Fix bugs in Reconnect logic #157
Conversation
* Expose new properties on IWebsocketClient (TextSenderRunning, BinarySenderRunning, IsInsideLock) * Use Debug log level when receiving a close message and creating new send threads * Write a Debug log message when the sending threads are shutting down
… events * Use ReconnectionType.ByServer when reconnecting in response to receiving a close message from the server * Actually start the client a 2nd time in the test for that
* Fixes an issue where the connection factory occasionally just hangs forever * Expose a new property for configuring how long to wait before timing out (defaulted to 2 seconds) * Ignore duplicate calls to Dispose * Use try methods for closing channel writers in Dispose * Update IsRunning flag immediately within exception handler in StartClient * Skip reconnecting on error after waiting if the client is already reconnecting by user request
Marfusios
left a comment
Marfusios
left a comment
There was a problem hiding this comment.
LGTM, thanks, I will test this locally before deploying
| StartBackgroundThreadForSendingBinary(); | ||
| } | ||
|
|
||
| BinarySenderRunning = false; |
There was a problem hiding this comment.
Probably better put it into finally block
There was a problem hiding this comment.
True, but you already merged it now.
| _client?.Dispose(); | ||
|
|
||
| if (!IsReconnectionEnabled || disInfo.CancelReconnection) | ||
| if (type != ReconnectionType.Error && (!IsReconnectionEnabled || disInfo.CancelReconnection)) |
There was a problem hiding this comment.
Not sure about this change, it could affect existing implementations that rely on auto reconnection. I will test it locally
There was a problem hiding this comment.
Without this, the unit test Starting_WithServerDelay_RetriesAfterConnectionTimeout fails. With it, all the unit test pass.
There was a problem hiding this comment.
I was a little worried it could stop the reconnection, but actually, it is the opposite; in case of error, it will always reconnect.
Looks good to me
| } | ||
| catch (Exception e) | ||
| { | ||
| IsRunning = _client?.State == WebSocketState.Open; |
There was a problem hiding this comment.
How is this possible to happen?
There was a problem hiding this comment.
Well, the exception could get thrown unexpectedly (perhaps even in future after other changes happen to this code). This is just here to be extra safe and make sure that we set the IsRunning property to false in case it is true at this point. I was just being pessimistic with this line of code. It can be removed if you want.
2 participants
Fixes #156
This PR fixes a few problems raised in the above issue:
ReconnectionTypeafter receivingClosemessage from serverDisconnectionHappenedstreamI have split the changes into separate commits, for easier reviewing. I've also taken the opportunity to improve the tests, by making them faster and more debuggable. I've also added some public members for better diagnotics/debugging support.