Issue #3379 - Add tracking of WebSocket Sessions to various WebSocket Container APIs #3383
Conversation
|
@lachlan-roberts with these additions, the testcases from |
| client.setIdleTimeout(Duration.ofMillis(timeout)); | ||
|
|
||
| ClientOpenSessionTracker clientSessionTracker = new ClientOpenSessionTracker(1); | ||
| clientSessionTracker.addTo(client); |
There was a problem hiding this comment.
So here to set a Connection.Listener we add the ClientOpenSessionTracker as a bean to the the WebSocketClient. But to successfully add the connection listener we need to set the Connection.Listener beans on the WebSocketCoreClient which is what we use to set the listeners on the WebSocketConnection in ClientUpgradeRequest.
There was a problem hiding this comment.
Yup, that's correct.
There was a problem hiding this comment.
maybe all the Connection.Listener beans could be added to the WebSocketCoreClient on the connect method of the WebSocketClient?
| // Verify that server socket got close event | ||
| AbstractCloseEndpoint serverEndpoint = serverEndpointCreator.pollLastCreated(); | ||
| assertThat("Fast Close Latch", serverEndpoint.closeLatch.await(5, SECONDS), is(true)); | ||
| assertThat("Fast Close.statusCode", serverEndpoint.closeStatusCode, is(StatusCode.ABNORMAL)); |
There was a problem hiding this comment.
why is this expecting ABNORMAL close status, the server sends receives NORMAL close and this is received by the client at line 146, what event is expected to happen to cause the abnormal close
There was a problem hiding this comment.
In this testcase, during the server onOpen() it issued a session.close().
Since the server's onOpen() never finished successfully, it the websocket connection was never opened for read on the server side, so it cannot read the response from the client. Hence the ABNORMAL closure.
There was a problem hiding this comment.
is closing in onOpen abnormal? I can imagine services that in onOpen send a few messages and then immediately do a normal close and then return. I think that is normal!
There was a problem hiding this comment.
closing in onOpen is abnormal & wasClean==false.
this is because the close handshake cannot be completed.
There was a problem hiding this comment.
the close handshake can be completed, we send the close frame, we return from onWebSocketConnect the onOpen callback is succeeded, so we can now read frames, we read the close response which is normal status code
| session = futSession.get(5, SECONDS); | ||
|
|
||
| // Verify that client got close | ||
| clientEndpoint.assertReceivedCloseEvent(5000, is(StatusCode.NORMAL), containsString("")); |
There was a problem hiding this comment.
reason should be "FastCloseServer"
| session = futSession.get(5, SECONDS); | ||
|
|
||
| // Verify that client got close indicating SERVER_ERROR | ||
| clientEndpoint.assertReceivedCloseEvent(5000, is(StatusCode.SERVER_ERROR), containsString("Intentional FastFail")); |
There was a problem hiding this comment.
the message given to WebSocketException created at line 153 of JettyWebSocketFrameHandler.onOpen() is too long to fit in a close frame so the "Intentional FastFail" part is trimmed out
There was a problem hiding this comment.
The Throwable.getMessage() is all that should be in the close reason.
If there is no Throwable.getMessage() (empty or null) then use the Throwable.getClass().getName().
There was a problem hiding this comment.
The @OnError gets the entire stacktrace anyway, so there's no purpose for the going nuts with the reason phrase.
There was a problem hiding this comment.
using .getSimpleName() instead leaves enough room for the exception message, maybe that would be better
but the same change should be made in the JavaxWebSocketFrameHandler as well to keep it consistent
|
|
||
| // Verify that server socket got close event | ||
| AbstractCloseEndpoint serverEndpoint = serverEndpointCreator.pollLastCreated(); | ||
| serverEndpoint.assertReceivedCloseEvent(5000, is(StatusCode.SERVER_ERROR), containsString("Intentional FastFail")); |
There was a problem hiding this comment.
I changed line 180 to use containsString("") to reach this line.
This is failing due to a timeout on the closeLatch, the problem is that in JettyWebSocketFrameHandler the closeHandle is only invoked when a close frame is received, this is incorrect and should be instead invoked in the onClosed(CloseStatus, Callback) method
There was a problem hiding this comment.
this failed for a different reason than above so its not the same, this is due to a problem with the implementation of the JettyWebSocketFrameHandler, and its probably the reason why a lot of the tests are failing
|
@joakime did you want me to make the changes to fix this up |
… Container APIs + Jetty WebSocket API now tracks Sessions and will close them on lifecycle stop + Javax WebSocket API now tracks Sessions and will close them on lifecycle stop + Adding Jetty WebSocket tests for proper close / session tracking + Disabling tests that need triage Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime
force-pushed
the
jetty-10-issue-3379-websocket-session-tracking
branch
from
0502e48
to
d8a9f26
Compare
|
@lachlan-roberts i've |
#3379
Signed-off-by: Joakim Erdfelt joakim@erdfelt.com