Skip to content
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

Merged
merged 5 commits into from Jun 13, 2022

Conversation

lachlan-roberts
Copy link
Contributor

Signed-off-by: Lachlan Roberts lachlan@webtide.com

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts lachlan-roberts requested review from gregw, sbordet and lorban and removed request for gregw June 10, 2022 07:45
// ========================================================================
//

package org.eclipse.jetty.websocket.core.exception;
Copy link
Contributor Author

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.

Copy link
Contributor

@lorban lorban left a 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
Copy link
Contributor

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 <lachlan@webtide.com>
lorban
lorban previously approved these changes Jun 10, 2022
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban lorban changed the title Use static exceptions for closing websocket flushers. Use static exceptions for closing websocket flushers and in ContentProducer Jun 10, 2022
@lorban
Copy link
Contributor

lorban commented Jun 10, 2022

Combined here the changes of #8135 as they're pretty trivial.

@joakime joakime added this to In progress in Jetty 10.0.10/11.0.10 - 🧊 FROZEN 🥶 via automation Jun 10, 2022
@joakime joakime moved this from In progress to Review in progress in Jetty 10.0.10/11.0.10 - 🧊 FROZEN 🥶 Jun 10, 2022
gregw
gregw previously approved these changes Jun 11, 2022
Copy link
Contributor

@gregw gregw left a 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 10.0.10/11.0.10 - 🧊 FROZEN 🥶 automation moved this from Review in progress to Reviewer approved Jun 11, 2022
Jetty 10.0.10/11.0.10 - 🧊 FROZEN 🥶 automation moved this from Reviewer approved to Review in progress Jun 12, 2022
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
…efault

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Jetty 10.0.10/11.0.10 - 🧊 FROZEN 🥶 automation moved this from Review in progress to Reviewer approved Jun 13, 2022
@sbordet sbordet merged commit 0699bc5 into jetty-10.0.x Jun 13, 2022
Jetty 10.0.10/11.0.10 - 🧊 FROZEN 🥶 automation moved this from Reviewer approved to Done Jun 13, 2022
@sbordet sbordet deleted the jetty-10.0.x-websocketFlusherExceptions branch June 13, 2022 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants