-
Notifications
You must be signed in to change notification settings - Fork 134
Fix bugs in Reconnect logic #157
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
Changes from all commits
465577d
d8125c9
dab027c
34a147c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,12 @@ public partial class WebsocketClient | |
|
|
||
| private static readonly byte[] _emptyArray = { }; | ||
|
|
||
| /// <inheritdoc /> | ||
| public bool TextSenderRunning { get; private set; } | ||
|
|
||
| /// <inheritdoc /> | ||
| public bool BinarySenderRunning { get; private set; } | ||
|
|
||
| /// <summary> | ||
| /// Send text message to the websocket channel. | ||
| /// It inserts the message to the queue and actual sending is done on another thread | ||
|
|
@@ -159,6 +165,7 @@ public void StreamFakeMessage(ResponseMessage message) | |
|
|
||
| private async Task SendTextFromQueue() | ||
| { | ||
| TextSenderRunning = true; | ||
| try | ||
| { | ||
| while (await _messagesTextToSendQueue.Reader.WaitToReadAsync()) | ||
|
|
@@ -176,30 +183,35 @@ private async Task SendTextFromQueue() | |
| } | ||
| } | ||
| } | ||
| catch (TaskCanceledException) | ||
| catch (TaskCanceledException e) | ||
| { | ||
| // task was canceled, ignore | ||
| _logger.LogDebug(e, L("Sending text thread failed, error: {error}. Shutting down."), Name, e.Message); | ||
| } | ||
| catch (OperationCanceledException) | ||
| catch (OperationCanceledException e) | ||
| { | ||
| // operation was canceled, ignore | ||
| _logger.LogDebug(e, L("Sending text thread failed, error: {error}. Shutting down."), Name, e.Message); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| if (_cancellationTotal?.IsCancellationRequested == true || _disposing) | ||
| { | ||
| // disposing/canceling, do nothing and exit | ||
| _logger.LogDebug(e, L("Sending text thread failed, error: {error}. Shutting down."), Name, e.Message); | ||
| TextSenderRunning = false; | ||
| return; | ||
| } | ||
|
|
||
| _logger.LogTrace(L("Sending text thread failed, error: {error}. Creating a new sending thread."), Name, e.Message); | ||
| _logger.LogDebug(e, L("Sending text thread failed, error: {error}. Creating a new sending thread."), Name, e.Message); | ||
| StartBackgroundThreadForSendingText(); | ||
| } | ||
|
|
||
| TextSenderRunning = false; | ||
| } | ||
|
|
||
| private async Task SendBinaryFromQueue() | ||
| { | ||
| BinarySenderRunning = true; | ||
| try | ||
| { | ||
| while (await _messagesBinaryToSendQueue.Reader.WaitToReadAsync()) | ||
|
|
@@ -217,26 +229,30 @@ private async Task SendBinaryFromQueue() | |
| } | ||
| } | ||
| } | ||
| catch (TaskCanceledException) | ||
| catch (TaskCanceledException e) | ||
| { | ||
| // task was canceled, ignore | ||
| _logger.LogDebug(e, L("Sending binary thread failed, error: {error}. Shutting down."), Name, e.Message); | ||
| } | ||
| catch (OperationCanceledException) | ||
| catch (OperationCanceledException e) | ||
| { | ||
| // operation was canceled, ignore | ||
| _logger.LogDebug(e, L("Sending binary thread failed, error: {error}. Shutting down."), Name, e.Message); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| if (_cancellationTotal?.IsCancellationRequested == true || _disposing) | ||
| { | ||
| // disposing/canceling, do nothing and exit | ||
| _logger.LogDebug(e, L("Sending binary thread failed, error: {error}. Shutting down."), Name, e.Message); | ||
| BinarySenderRunning = false; | ||
| return; | ||
| } | ||
|
|
||
| _logger.LogTrace(L("Sending binary thread failed, error: {error}. Creating a new sending thread."), Name, e.Message); | ||
| _logger.LogDebug(e, L("Sending binary thread failed, error: {error}. Creating a new sending thread."), Name, e.Message); | ||
| StartBackgroundThreadForSendingBinary(); | ||
| } | ||
|
|
||
| BinarySenderRunning = false; | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably better put it into finally block
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, but you already merged it now.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, np, I will update it |
||
| } | ||
|
|
||
| private void StartBackgroundThreadForSendingText() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,7 @@ public partial class WebsocketClient : IWebsocketClient | |
| private bool _isReconnectionEnabled = true; | ||
| private WebSocket? _client; | ||
| private CancellationTokenSource? _cancellation; | ||
| private CancellationTokenSource? _cancellationConnection; | ||
| private CancellationTokenSource? _cancellationTotal; | ||
|
|
||
| private readonly Subject<ResponseMessage> _messageReceivedSubject = new Subject<ResponseMessage>(); | ||
|
|
@@ -123,6 +124,12 @@ public Uri Url | |
| /// </summary> | ||
| public IObservable<DisconnectionInfo> DisconnectionHappened => _disconnectedSubject.AsObservable(); | ||
|
|
||
| /// <summary> | ||
| /// Time range for how long to wait while connecting a new client. | ||
| /// Default: 2 seconds | ||
| /// </summary> | ||
| public TimeSpan ConnectTimeout { get; set; } = TimeSpan.FromSeconds(2); | ||
|
|
||
| /// <summary> | ||
| /// Time range for how long to wait before reconnecting if no message comes from server. | ||
| /// Set null to disable this feature. | ||
|
|
@@ -189,6 +196,9 @@ public bool IsReconnectionEnabled | |
| /// Default: true | ||
| /// </summary> | ||
| public bool IsTextMessageConversionEnabled { get; set; } = true; | ||
|
|
||
| /// <inheritdoc /> | ||
| public bool IsInsideLock => _locker.IsLocked; | ||
|
|
||
| /// <summary> | ||
| /// Enable or disable automatic <see cref="MemoryStream.Dispose(bool)"/> of the <see cref="MemoryStream"/> | ||
|
|
@@ -210,19 +220,24 @@ public bool IsReconnectionEnabled | |
| /// </summary> | ||
| public void Dispose() | ||
| { | ||
| if (_disposing) | ||
| return; | ||
|
|
||
| _disposing = true; | ||
| _logger.LogDebug(L("Disposing.."), Name); | ||
| try | ||
| { | ||
| _messagesTextToSendQueue.Writer.Complete(); | ||
| _messagesBinaryToSendQueue.Writer.Complete(); | ||
| _messagesTextToSendQueue.Writer.TryComplete(); | ||
| _messagesBinaryToSendQueue.Writer.TryComplete(); | ||
| _lastChanceTimer?.Dispose(); | ||
| _errorReconnectTimer?.Dispose(); | ||
| _cancellation?.Cancel(); | ||
| _cancellationConnection?.Cancel(); | ||
| _cancellationTotal?.Cancel(); | ||
| _client?.Abort(); | ||
| _client?.Dispose(); | ||
| _cancellation?.Dispose(); | ||
| _cancellationConnection?.Dispose(); | ||
| _cancellationTotal?.Dispose(); | ||
| _messageReceivedSubject.OnCompleted(); | ||
| _reconnectionSubject.OnCompleted(); | ||
|
|
@@ -402,7 +417,9 @@ private async Task StartClient(Uri uri, CancellationToken token, ReconnectionTyp | |
|
|
||
| try | ||
| { | ||
| _client = await _connectionFactory(uri, token).ConfigureAwait(false); | ||
| _cancellationConnection = CancellationTokenSource.CreateLinkedTokenSource(token); | ||
| _cancellationConnection.CancelAfter(ConnectTimeout); | ||
| _client = await _connectionFactory(uri, _cancellationConnection.Token).ConfigureAwait(false); | ||
| _ = Listen(_client, token); | ||
| IsRunning = true; | ||
| IsStarted = true; | ||
|
|
@@ -412,6 +429,7 @@ private async Task StartClient(Uri uri, CancellationToken token, ReconnectionTyp | |
| } | ||
| catch (Exception e) | ||
| { | ||
| IsRunning = _client?.State == WebSocketState.Open; | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is this possible to happen?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| var info = DisconnectionInfo.Create(DisconnectionType.Error, _client, e); | ||
| _disconnectedSubject.OnNext(info); | ||
|
|
||
|
|
@@ -447,7 +465,9 @@ private async Task StartClient(Uri uri, CancellationToken token, ReconnectionTyp | |
|
|
||
| private void ReconnectOnError(object? state) | ||
| { | ||
| // await Task.Delay(timeout, token).ConfigureAwait(false); | ||
| if (_client != null && ShouldIgnoreReconnection(_client)) | ||
| return; | ||
|
|
||
| _ = Reconnect(ReconnectionType.Error, false, state as Exception).ConfigureAwait(false); | ||
| } | ||
|
|
||
|
|
@@ -491,7 +511,7 @@ private async Task Listen(WebSocket client, CancellationToken token) | |
| } | ||
| else if (result.MessageType == WebSocketMessageType.Close) | ||
| { | ||
| _logger.LogTrace(L("Received close message"), Name); | ||
| _logger.LogDebug(L("Received close message"), Name); | ||
|
|
||
| if (!IsStarted || _stopping) | ||
| { | ||
|
|
@@ -512,13 +532,12 @@ private async Task Listen(WebSocket client, CancellationToken token) | |
| continue; | ||
| } | ||
|
|
||
| await StopInternal(client, WebSocketCloseStatus.NormalClosure, "Closing", | ||
| token, false, true); | ||
| await StopInternal(client, WebSocketCloseStatus.NormalClosure, "Closing", token, false, true); | ||
|
|
||
| // reconnect if enabled | ||
| if (IsReconnectionEnabled && !ShouldIgnoreReconnection(client)) | ||
| { | ||
| _ = ReconnectSynchronized(ReconnectionType.Lost, false, null); | ||
| _ = ReconnectSynchronized(ReconnectionType.ByServer, false, null); | ||
| } | ||
|
|
||
| return; | ||
|
|
||
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this, the unit test
Starting_WithServerDelay_RetriesAfterConnectionTimeoutfails. With it, all the unit test pass.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 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