Immediately set connection state to disconnected due to a pong timeout #118
Conversation
The underlying WebSocketClient connection will attempt to perform a WebSocket compliant graceful shutdown. But in the case where a disconnect was called due to the client not receiving a pong, this will likely not succeed, which would delay the change to DISCONNECTED.
|
/cc @jpatel531 |
|
I'm happy to merge this - pinging @mdpye for a 2nd opinion |
| // Sometimes expected when we disconnected due to pong timeout | ||
| log.error("Received close from underlying socket when already disconnected. " + "Close code [" | ||
| + code + "], Reason [" + reason + "], Remote [" + remote + "]"); | ||
| } |
There was a problem hiding this comment.
I think a short circuit exit at the top would be much clearer than wrapping the entire effective body of the method in an if.
Also, we should not log at error level in expected circumstances.
|
The change as proposed skips the proper close procedure not just in the case of pong timeouts, but in all cases where the user elects to disconnect. I can see why we might want an un-graceful shutdown in the pong timeout case, but we should restrict the hack to that case and respect the underlying state machine in the general case if you ask me. We should at least remove ourselves as a listener from the outgoing |
|
Ok, indeed it doesn't make sense to ungracefully shutdown connections that weren't timed out. I'll change this. Not sure calling I'd propose exposing the |
Yes, I wasn't proposing that we call
Cleanest would probably to be to create a |
|
Made some improvements in 44701cd. I think I ruined the |
|
Looks good.
Just add the |
Yes, but that's not directly under our control as we are using another library for that. I could make a PR to it though. |
|
Ah, sorry, I didn't realise. |
|
Actually, I'm not going to bother with a PR for the WebSocket client. Last two accepted there were in Feb 2016 and another in mid-August 2015. And the Feb one was this. |
The underlying WebSocketClient connection will attempt to perform a WebSocket compliant graceful shutdown.
But in the case where a disconnect was called due to the client not receiving a pong, this will likely not succeed, which would delay the change to DISCONNECTED.