-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework static exceptions in ContentProducer #8135
Conversation
6baf0c1
to
5edfef1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying hard to think of why this is going to bite us.... but this all looks very straight forward and how it probably should have been done in the first place.
jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to make so many changes now that you've discovered the constructor that allows us to avoid suppressed exceptions for static sentinel Throwables.
Perhaps it is better for use to create a utility StaticThrowable class that has no stack trace and doesn't keep suppressed exceptions. We can then use that as the base for our existing static exceptions rather than replacing them.
|
||
public UnconsumedContentException() | ||
{ | ||
super("Unconsumed content", null, false, LOG.isDebugEnabled()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this constructor is key!
Seeing that this constructor allows us to disable suppressed exceptions, there is no real reason for us to avoid static Throwables used as sentinels.
I'm not keen on using the LOG to hide stack traces. I think if it is worth creating an exception of the fly, it is worth generating a stack trace. I think we should avoid stack traces only for static sentinel Throwables.
@lachlan-roberts can you review your recent change in light of this constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. While this code might be worth some refactorings, that's clearly outside the scope of the original issue which is just to avoid leaking stack traces in static exceptions.
I'm going to revert all these changes, introduce a new StaticException
and keep the changes to a minimum.
…les. Signed-off-by: Ludovic Orban <[email protected]>
ac2c862
to
e175625
Compare
Changes combined with #8155 |
As this is a potential memory leak if
Throwable.addSuppressed()
is ever called on the static exception, or on the exception contained in the staticErrorContent
.