[Java] Don't call onClose when WebSocket connection is not open #28004
Conversation
BrennanConroy
added
Servicing-consider
|
Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge. |
| } | ||
| checkStartFailure(); | ||
| checkStartFailure(null); | ||
| } | ||
|
|
||
| @Override | ||
| public void onFailure(WebSocket webSocket, Throwable t, Response response) { |
There was a problem hiding this comment.
Is it ever possible for both onClosing and onFailure to be called on the same WebSocketListener?
There was a problem hiding this comment.
I don't believe so
| @@ -30,7 +30,7 @@ | |||
| private WebSocketOnClosedCallback onClose; | |||
| private CompletableSubject startSubject = CompletableSubject.create(); | |||
| private CompletableSubject closeSubject = CompletableSubject.create(); | |||
| private final ReentrantLock closeLock = new ReentrantLock(); | |||
| private final ReentrantLock stateLock = new ReentrantLock(); | |||
There was a problem hiding this comment.
A little unrelated to this pr, but do you know why we use ReentrantLock instead of synchronized?
There was a problem hiding this comment.
I'm not sure there is a good reason. Maybe just oversight
...lR/clients/java/signalr/core/src/main/java/com/microsoft/signalr/OkHttpWebSocketWrapper.java
Outdated
Show resolved
Hide resolved
…oft/signalr/OkHttpWebSocketWrapper.java Co-authored-by: Stephen Halter <halter73@gmail.com>
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
| } | ||
| // Only call onClose if connection is open | ||
| if (isOpen) { | ||
| onClose.invoke(code, reason); |
There was a problem hiding this comment.
Now that we sometimes discard the onClosing code/reason, we should log it.
| } | ||
| checkStartFailure(); | ||
| checkStartFailure(null); | ||
| } | ||
|
|
||
| @Override | ||
| public void onFailure(WebSocket webSocket, Throwable t, Response response) { | ||
| logger.error("WebSocket closed from an error: {}.", t.getMessage()); |
There was a problem hiding this comment.
And while I'm thinking about logging. I assume the following will produce more helpful output.
| logger.error("WebSocket closed from an error: {}.", t.getMessage()); | |
| logger.error("WebSocket closed from an error.", t); |
BrennanConroy
added
Servicing-approved
|
Servicing approved over email. Still need to react to final feedback. |
Fixes #27438
Description
If an endpoint doesn't support WebSockets or some kind of connection issue occurs then the WebSockets
onFailuremethod will be called which will callonClose. TheonClosecallback is only expected to be called when a connection is already connected and will fail when in this state, this results inhubConnection.start()never completing or in some environments, the client crashes due to an unhandled exception.Customer Impact
Bug reported in #27438 as well as an internal partner.
The bug will cause
hubConnection.start()to not finish or client crashes.A workaround is to only use LongPolling for the client, WebSockets is the best transport so this isn't a great workaround.
Regression?
Yes, regressed from 3.1 to 5.0
Risk
Low.
Issue is easy to repro and we have prior art with a similar change #14114