-
Notifications
You must be signed in to change notification settings - Fork 2k
Issue #13525 - potential fix for flakiness in JakartaWebSocketFrameHandlerOnMessageBinaryStreamTest #13550
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
base: jetty-12.1.x
Are you sure you want to change the base?
Issue #13525 - potential fix for flakiness in JakartaWebSocketFrameHandlerOnMessageBinaryStreamTest #13550
Conversation
…ndlerOnMessageBinaryStreamTest Signed-off-by: Lachlan Roberts <[email protected]>
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.
When you are having lifecycle start/stop order issues, the solution is never to start doing explicit start calls to sub components. Build a correct containment tree and let the infrastructure code do its work. I.e. follow the pattern rather than fight it. Don't try to work around it, adopt it fully!
components = new WebSocketComponents(); | ||
container = new DummyContainer(components); | ||
components.start(); | ||
container.start(); |
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.
Starting both should not be required. Just start the container
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 don't think we want to do this.
Because in the non-test usage we have the WebSocketComponents
managed by the server lifecycle so it can be shared between multiple WebSocket containers. For example if both Jakarta and Jetty websocket containers were used, or WebSocketContainers with different EE versions.
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.
Jetty's component mechanism can handle this. If the components are managed by the server, then it will be started by server and the auto beans in the containers will be unmanaged.
Embrace the pattern!
{ | ||
LifeCycle.stop(components); | ||
LifeCycle.stop(container); | ||
LifeCycle.stop(components); |
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.
just stop the container
public DummyContainer(WebSocketComponents components) | ||
{ | ||
super(new WebSocketComponents()); | ||
super(components); |
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.
The super constructor should installBean(components), but it does not
container = new DummyContainer(components); | ||
components.start(); | ||
container.start(); |
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.
ditto
…13525-FlakeyTest-testInvokeMessageStream
Closes #13525
I'm not sure if this will actually fix #13525, but it cleans up some of the usage around the
WebSocketComponents
lifecycle and tests that theWebSocketComponents
is actually started when executing the test.I think it is most likely that the
RejectedExecutionException
is resulting from aWebSocketComponents
which has not been started yet.