-
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
Use static exceptions for closing websocket flushers and in ContentProducer #8155
Use static exceptions for closing websocket flushers and in ContentProducer #8155
Conversation
Signed-off-by: Lachlan Roberts <[email protected]>
// ======================================================================== | ||
// | ||
|
||
package org.eclipse.jetty.websocket.core.exception; |
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.
Not sure if this should be here or in an internal implementation package.
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.
LGTM, but I think we should align this PR with this other one: #8135 as we should use have a common suppressed-exceptions-safe exception class.
* meaning calling {@link #addSuppressed(Throwable)} has no effect. | ||
* This means instances of {@link SentinelWebSocketCloseException} are suitable to be kept as static fields. | ||
*/ | ||
public class SentinelWebSocketCloseException extends Exception |
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.
As @lorban suggested, perhaps create a util class called StaticException that forces the false,true args of the super constructor.
Signed-off-by: Lachlan Roberts <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Combined here the changes of #8135 as they're pretty trivial. |
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 is good, other than a tiny quibble.
Going foward, I still wonder if we should every throw/pass such a sentinel or if we should make a new exception when the sentinel is detected.... but for now, this is a good minimal change.
jetty-util/src/main/java/org/eclipse/jetty/util/StaticException.java
Outdated
Show resolved
Hide resolved
jetty-util/src/main/java/org/eclipse/jetty/util/StaticException.java
Outdated
Show resolved
Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Ludovic Orban <[email protected]>
jetty-util/src/main/java/org/eclipse/jetty/util/StaticException.java
Outdated
Show resolved
Hide resolved
…efault Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Lachlan Roberts [email protected]