Skip to content

Commit

Permalink
fix: DefaultWebSocket can now correctly close
Browse files Browse the repository at this point in the history
This resolves a bug where disconnecting the socket client would not
actually close the websocket. Bots would appear to remain online in the
discord client until their connection to discord eventually timed out.

The underlying cause of this issue sourced from the cancellation token
passed into the websocket's ReceiveAsync method - when entering the
disconnect process, the first step is to cancel out all of the
connection tokens. Unfortunately, the standard ClientWebSocket handles a
token cancellation by aborting the socket, rendering it inoperable for a
safe closure.

This change removes the inner cancellation token passed into
ReceiveAsync. The cancellation token is still retained for use in the
receive loop, so the receive task should gracefully complete once some
event satisfies the ClientWebSocket's blocking receive.

To ensure that all clients succesfully close, regardless of their
traffic, the disconnect procedure was rearranged such that awaiting the
receive task now occurs last, after the socket has been closed. Closing
the socket will propagate an event up to the ClientWebSocket's receive
method, which will allow the loop to iterate and gracefully complete.

So far, I have validated this change against basic connection opening
and closing, for both the gateway and voice clients. I have not yet
validated against unplanned connection interruptions, though I believe
that this change might actually improve some of those connection bugs,
since the ClientWebSocket should never find itself in an aborted state.
  • Loading branch information
foxbot committed Dec 22, 2018
1 parent aec7105 commit ac389f5
Showing 1 changed file with 10 additions and 8 deletions.
18 changes: 10 additions & 8 deletions src/Discord.Net.WebSocket/Net/DefaultWebSocketClient.cs
Expand Up @@ -108,15 +108,10 @@ public async Task DisconnectAsync()
}
private async Task DisconnectInternalAsync(bool isDisposing = false)
{
try { _disconnectTokenSource.Cancel(false); } catch { }
try { _disconnectTokenSource.Cancel(false); }
catch { }

_isDisconnecting = true;
try
{
await (_task ?? Task.Delay(0)).ConfigureAwait(false);
_task = null;
}
finally { _isDisconnecting = false; }

if (_client != null)
{
Expand All @@ -130,6 +125,13 @@ private async Task DisconnectInternalAsync(bool isDisposing = false)

_client = null;
}

try
{
await (_task ?? Task.Delay(0)).ConfigureAwait(false);
_task = null;
}
finally { _isDisconnecting = false; }
}
private async Task OnClosed(Exception ex)
{
Expand Down Expand Up @@ -198,7 +200,7 @@ private async Task RunAsync(CancellationToken cancelToken)
{
while (!cancelToken.IsCancellationRequested)
{
WebSocketReceiveResult socketResult = await _client.ReceiveAsync(buffer, cancelToken).ConfigureAwait(false);
WebSocketReceiveResult socketResult = await _client.ReceiveAsync(buffer, CancellationToken.None).ConfigureAwait(false);
byte[] result;
int resultCount;

Expand Down

0 comments on commit ac389f5

Please sign in to comment.