From e66ff5977b49a389b9eef917020063a4bd0a4190 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Wed, 9 Dec 2020 11:23:14 +0100 Subject: [PATCH 01/19] Set the WebSocketException to Faulted to fix tests --- .../src/System/Net/WebSockets/WebSocketHandle.Browser.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.Browser.cs b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.Browser.cs index cf91e6821db05e..9876fc126376f7 100644 --- a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.Browser.cs +++ b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.Browser.cs @@ -67,7 +67,7 @@ public async Task ConnectAsync(Uri uri, CancellationToken cancellationToken, Cli case OperationCanceledException _ when cancellationToken.IsCancellationRequested: throw; default: - throw new WebSocketException(SR.net_webstatus_ConnectFailure, exc); + throw new WebSocketException(WebSocketError.Faulted, SR.net_webstatus_ConnectFailure, exc); } } } From 1bd6fa6800bf287b6b535d58da3090a46b940694 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Wed, 9 Dec 2020 11:25:19 +0100 Subject: [PATCH 02/19] Fix the exceptions that are being thrown to coincide with test expectations --- .../Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs index 25cecaf3e3d9d6..ee9126e2907bf5 100644 --- a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs +++ b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs @@ -446,7 +446,13 @@ public override async Task ReceiveAsync(ArraySegment Date: Wed, 9 Dec 2020 11:27:55 +0100 Subject: [PATCH 03/19] Fix Dispose we are not processing it multiple times. --- .../BrowserWebSockets/BrowserWebSocket.cs | 29 +++++++++++-------- 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs index ee9126e2907bf5..77363558d11d6c 100644 --- a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs +++ b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs @@ -59,6 +59,7 @@ private enum InternalState Disposed = 3 } + private bool _disposed; /// /// Initializes a new instance of the class. @@ -318,23 +319,27 @@ private void NativeCleanup() public override void Dispose() { - int priorState = Interlocked.Exchange(ref _state, (int)InternalState.Disposed); - if (priorState == (int)InternalState.Disposed) + if (!_disposed) { - // No cleanup required. - return; - } + if (_state < (int)InternalState.Aborted) { + Interlocked.Exchange(ref _state, (int)InternalState.Disposed); + } + _disposed = true; - // registered by the CancellationTokenSource cts in the connect method - _cts.Cancel(false); - _cts.Dispose(); + if (!_cts.IsCancellationRequested) + { + // registered by the CancellationTokenSource cts in the connect method + _cts.Cancel(false); + _cts.Dispose(); + } - _writeBuffer?.Dispose(); - _receiveMessageQueue.Writer.Complete(); + _writeBuffer?.Dispose(); + _receiveMessageQueue.Writer.TryComplete(); - NativeCleanup(); + NativeCleanup(); - _innerWebSocket?.Dispose(); + _innerWebSocket?.Dispose(); + } } // This method is registered by the CancellationTokenSource cts in the connect method From 334a002ddd9975cfa708fbe00136bde841b62c82 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Wed, 9 Dec 2020 11:31:31 +0100 Subject: [PATCH 04/19] Fix Abort code so the messages align correctly with the tests. example from tests: ``` Assert.Equal() Failure info: Expected: Aborted info: Actual: Closed ``` --- .../BrowserWebSockets/BrowserWebSocket.cs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs index 77363558d11d6c..ef3c349f1e204c 100644 --- a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs +++ b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs @@ -56,7 +56,8 @@ private enum InternalState Created = 0, Connecting = 1, Connected = 2, - Disposed = 3 + Disposed = 3, + Aborted = 4 } private bool _disposed; @@ -87,6 +88,8 @@ public override WebSocketState State { InternalState.Created => WebSocketState.None, InternalState.Connecting => WebSocketState.Connecting, + InternalState.Aborted => WebSocketState.Aborted, + InternalState.Disposed => WebSocketState.Closed, _ => WebSocketState.Closed }; } @@ -164,8 +167,9 @@ internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellati _innerWebSocketCloseStatusDescription = closeEvt.GetObjectProperty("reason")?.ToString(); _receiveMessageQueue.Writer.TryWrite(new ReceivePayload(Array.Empty(), WebSocketMessageType.Close)); NativeCleanup(); - if ((InternalState)_state == InternalState.Connecting) + if ((InternalState)_state == InternalState.Connecting || (InternalState)_state == InternalState.Aborted) { + _state = (int)InternalState.Disposed; if (cancellationToken.IsCancellationRequested) { tcsConnect.TrySetCanceled(cancellationToken); @@ -440,10 +444,10 @@ public override async Task ReceiveAsync(ArraySegment ReceiveAsync(ArraySegment public override void Abort() { - if (_state == (int)InternalState.Disposed) + if (_state != (int)InternalState.Disposed) { - return; + if (Interlocked.Exchange(ref _state, (int)InternalState.Aborted) != (int)InternalState.Aborted) + { + _cts.Cancel(true); + } } - _state = (int)WebSocketState.Aborted; - Dispose(); } public override async Task CloseAsync(WebSocketCloseStatus closeStatus, string? statusDescription, CancellationToken cancellationToken) From 4bc9bc7b7b7284a7ab667f0b933163ca339130d1 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Wed, 9 Dec 2020 11:32:24 +0100 Subject: [PATCH 05/19] Set the WebSocketException to Faulted to fix test expectations --- .../System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs index ef3c349f1e204c..aff30d70093450 100644 --- a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs +++ b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs @@ -232,7 +232,7 @@ internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellati case OperationCanceledException: throw; default: - throw new WebSocketException(SR.net_webstatus_ConnectFailure, wse); + throw new WebSocketException(WebSocketError.Faulted, SR.net_webstatus_ConnectFailure, wse); } } finally From c13cb26c3bf6c891cf03c702bddad40ef14915a5 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Wed, 9 Dec 2020 11:33:40 +0100 Subject: [PATCH 06/19] Close the connections correctly. --- .../BrowserWebSockets/BrowserWebSocket.cs | 39 +++++++++++++------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs index aff30d70093450..c1daaddee65d72 100644 --- a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs +++ b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs @@ -479,24 +479,39 @@ public override void Abort() } } - public override async Task CloseAsync(WebSocketCloseStatus closeStatus, string? statusDescription, CancellationToken cancellationToken) + public override Task CloseAsync(WebSocketCloseStatus closeStatus, string? statusDescription, CancellationToken cancellationToken) { _writeBuffer = null; ThrowIfNotConnected(); - await CloseAsyncCore(closeStatus, statusDescription, cancellationToken).ConfigureAwait(continueOnCapturedContext: true); - } - - private async Task CloseAsyncCore(WebSocketCloseStatus closeStatus, string? statusDescription, CancellationToken cancellationToken) - { - ThrowOnInvalidState(State, WebSocketState.Connecting, WebSocketState.Open, WebSocketState.CloseReceived, WebSocketState.CloseSent); WebSocketValidate.ValidateCloseStatus(closeStatus, statusDescription); - _tcsClose = new TaskCompletionSource(); - _innerWebSocketCloseStatus = closeStatus; - _innerWebSocketCloseStatusDescription = statusDescription; - _innerWebSocket!.Invoke("close", (int)closeStatus, statusDescription); - await _tcsClose.Task.ConfigureAwait(continueOnCapturedContext: true); + try + { + ThrowOnInvalidState(State, WebSocketState.Open, WebSocketState.CloseReceived, WebSocketState.CloseSent); + } + catch (Exception exc) + { + return Task.FromException(exc); + } + + return CloseAsyncCore(closeStatus, statusDescription, cancellationToken); + } + + private Task CloseAsyncCore(WebSocketCloseStatus closeStatus, string? statusDescription, CancellationToken cancellationToken) + { + try + { + _tcsClose = new TaskCompletionSource(); + _innerWebSocketCloseStatus = closeStatus; + _innerWebSocketCloseStatusDescription = statusDescription; + _innerWebSocket!.Invoke("close", (int)closeStatus, statusDescription); + return _tcsClose.Task; + } + catch (Exception exc) + { + return Task.FromException(exc); + } } public override Task CloseOutputAsync(WebSocketCloseStatus closeStatus, string? statusDescription, CancellationToken cancellationToken) => throw new PlatformNotSupportedException(); From 00c6ef7ff3332043aa83158516e7612156269e7e Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Thu, 10 Dec 2020 08:12:21 +0100 Subject: [PATCH 07/19] Fix Abort test Abort_CloseAndAbort_Success - This was causing a never ending Task when aborted after a Close already executed. - Never ended which was a cause of memory errors after left running. - See also https://github.com/dotnet/runtime/issues/45586 --- .../Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs index c1daaddee65d72..cf771b0e2b7e80 100644 --- a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs +++ b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs @@ -181,7 +181,7 @@ internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellati } else { - _tcsClose?.SetResult(); + _tcsClose?.TrySetResult(); } } }; @@ -470,11 +470,13 @@ public override async Task ReceiveAsync(ArraySegment public override void Abort() { + System.Diagnostics.Debug.WriteLine($"BrowserWebSocket::Abort() {_tcsClose}"); if (_state != (int)InternalState.Disposed) { if (Interlocked.Exchange(ref _state, (int)InternalState.Aborted) != (int)InternalState.Aborted) { _cts.Cancel(true); + _tcsClose?.TrySetResult(); } } } From 2d138334263285bfc7357077cba062fafb896078 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Thu, 10 Dec 2020 10:55:22 +0100 Subject: [PATCH 08/19] - Fixes for ReceiveAsyncTest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Exception text not sent correctly. This test was expecting 'Aborted'. - Mismatched expected exception messages - Make sure the connection is torn down. - Multiple GC calls that end in running out of memory fixed. https://github.com/dotnet/runtime/issues/45586 ``` // [FAIL] System.Net.WebSockets.Client.Tests.CancelTest.ReceiveAsync_AfterCancellationDoReceiveAsync_ThrowsWebSocketException(server: ws://corefx-net-http11.azurewebsites.net/WebSocket/EchoWebSocket.ashx) // info: Assert.Equal() Failure // info: ↓ (pos 39) // info: Expected: ··· an invalid state ('Aborted') for this operation. Valid state··· // info: Actual: ··· an invalid state ('Open') for this operation. Valid states a··· // info: ↑ (pos 39) ``` --- .../BrowserWebSockets/BrowserWebSocket.cs | 33 +++++++++++++++---- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs index cf771b0e2b7e80..e2e718983f421f 100644 --- a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs +++ b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs @@ -432,6 +432,16 @@ public override Task SendAsync(ArraySegment buffer, WebSocketMessageType m return Task.CompletedTask; } + // This method is registered by the CancellationTokenSource in the receive async method + private async void CancelRequest() + { + _receiveMessageQueue.Writer.TryComplete(); + if (State == WebSocketState.Open || State == WebSocketState.Connecting) + { + await CloseAsyncCore(WebSocketCloseStatus.NormalClosure, SR.net_WebSockets_Connection_Aborted, CancellationToken.None).ConfigureAwait(continueOnCapturedContext: true); + } + } + /// /// Receives data on as an asynchronous operation. /// @@ -440,15 +450,19 @@ public override Task SendAsync(ArraySegment buffer, WebSocketMessageType m /// Cancellation token. public override async Task ReceiveAsync(ArraySegment buffer, CancellationToken cancellationToken) { - WebSocketValidate.ValidateArraySegment(buffer, nameof(buffer)); - ThrowIfDisposed(); - ThrowOnInvalidState(State, WebSocketState.Open, WebSocketState.CloseSent); + CancellationTokenSource _receiveCTS = new CancellationTokenSource(); + CancellationTokenRegistration receiveRegistration = cancellationToken.Register(cts => ((CancellationTokenSource)cts!).Cancel(), _receiveCTS); + _receiveCTS.Token.Register(s => ((BrowserWebSocket)s!).CancelRequest(), this); try { + WebSocketValidate.ValidateArraySegment(buffer, nameof(buffer)); + + ThrowIfDisposed(); + ThrowOnInvalidState(State, WebSocketState.Open, WebSocketState.CloseSent); _bufferedPayload ??= await _receiveMessageQueue.Reader.ReadAsync(cancellationToken).ConfigureAwait(continueOnCapturedContext: true); - bool endOfMessage = _bufferedPayload.BufferPayload(buffer, out WebSocketReceiveResult receiveResult); + bool endOfMessage = _bufferedPayload!.BufferPayload(buffer, out WebSocketReceiveResult receiveResult); if (endOfMessage) _bufferedPayload = null; return receiveResult; @@ -458,11 +472,17 @@ public override async Task ReceiveAsync(ArraySegment(exc).ConfigureAwait(continueOnCapturedContext: true); + case ChannelClosedException: + return await Task.FromException(new WebSocketException(WebSocketError.InvalidState, SR.Format(SR.net_WebSockets_InvalidState, "Aborted", "Open, CloseSent"))).ConfigureAwait(continueOnCapturedContext: true); default: - throw new WebSocketException(WebSocketError.InvalidState, exc); + return await Task.FromException(new WebSocketException(WebSocketError.InvalidState, SR.Format(SR.net_WebSockets_InvalidState, State, "Open, CloseSent"))).ConfigureAwait(continueOnCapturedContext: true); } } + finally + { + receiveRegistration.Unregister(); + } } /// @@ -470,7 +490,6 @@ public override async Task ReceiveAsync(ArraySegment public override void Abort() { - System.Diagnostics.Debug.WriteLine($"BrowserWebSocket::Abort() {_tcsClose}"); if (_state != (int)InternalState.Disposed) { if (Interlocked.Exchange(ref _state, (int)InternalState.Aborted) != (int)InternalState.Aborted) From b1c21c0c0e12541fc507cd76e3c7819ee904456d Mon Sep 17 00:00:00 2001 From: Larry Ewing Date: Tue, 5 Jan 2021 14:27:29 -0600 Subject: [PATCH 09/19] Remove ActiveIssue attributes that should be fixed --- src/libraries/System.Net.WebSockets.Client/tests/AbortTest.cs | 4 ---- .../System.Net.WebSockets.Client/tests/CancelTest.cs | 2 -- 2 files changed, 6 deletions(-) diff --git a/src/libraries/System.Net.WebSockets.Client/tests/AbortTest.cs b/src/libraries/System.Net.WebSockets.Client/tests/AbortTest.cs index 6b48fd285258af..565150cbe91a09 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/AbortTest.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/AbortTest.cs @@ -17,7 +17,6 @@ public AbortTest(ITestOutputHelper output) : base(output) { } [OuterLoop] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/45674", TestPlatforms.Browser)] public async Task Abort_ConnectAndAbort_ThrowsWebSocketExceptionWithmessage(Uri server) { using (var cws = new ClientWebSocket()) @@ -43,7 +42,6 @@ public async Task Abort_ConnectAndAbort_ThrowsWebSocketExceptionWithmessage(Uri [OuterLoop] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/45674", TestPlatforms.Browser)] public async Task Abort_SendAndAbort_Success(Uri server) { await TestCancellation(async (cws) => @@ -64,7 +62,6 @@ await TestCancellation(async (cws) => [OuterLoop] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/45674", TestPlatforms.Browser)] public async Task Abort_ReceiveAndAbort_Success(Uri server) { await TestCancellation(async (cws) => @@ -89,7 +86,6 @@ await cws.SendAsync( [OuterLoop] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/45674", TestPlatforms.Browser)] public async Task Abort_CloseAndAbort_Success(Uri server) { await TestCancellation(async (cws) => diff --git a/src/libraries/System.Net.WebSockets.Client/tests/CancelTest.cs b/src/libraries/System.Net.WebSockets.Client/tests/CancelTest.cs index 32780301a3644b..92c44a712378a1 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/CancelTest.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/CancelTest.cs @@ -131,7 +131,6 @@ public async Task ReceiveAsync_CancelThenReceive_ThrowsOperationCanceledExceptio [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/45674", TestPlatforms.Browser)] public async Task ReceiveAsync_ReceiveThenCancel_ThrowsOperationCanceledException(Uri server) { using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output)) @@ -148,7 +147,6 @@ public async Task ReceiveAsync_ReceiveThenCancel_ThrowsOperationCanceledExceptio [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/45674", TestPlatforms.Browser)] public async Task ReceiveAsync_AfterCancellationDoReceiveAsync_ThrowsWebSocketException(Uri server) { using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output)) From fc157017db63c9283908a5274dbb0ad42b7867d4 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Mon, 11 Jan 2021 08:12:42 +0100 Subject: [PATCH 10/19] Add back code after merge conflict - Update tests that are resolved. --- .../BrowserWebSockets/BrowserWebSocket.cs | 93 +++++++++++++------ .../tests/AbortTest.cs | 1 + .../tests/CancelTest.cs | 1 + .../tests/SendReceiveTest.cs | 6 +- 4 files changed, 69 insertions(+), 32 deletions(-) diff --git a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs index 4288444f2de37b..babf057df4e0b0 100644 --- a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs +++ b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs @@ -56,9 +56,11 @@ private enum InternalState Created = 0, Connecting = 1, Connected = 2, - Disposed = 3 + Disposed = 3, + Aborted = 4 } + private bool _disposed; /// /// Initializes a new instance of the class. @@ -78,7 +80,7 @@ public override WebSocketState State { get { - if (_innerWebSocket != null && !_innerWebSocket.IsDisposed) + if (_innerWebSocket != null && !_innerWebSocket.IsDisposed && _state != (int)InternalState.Aborted) { return ReadyStateToDotNetState((int)_innerWebSocket.GetObjectProperty("readyState")); } @@ -86,6 +88,8 @@ public override WebSocketState State { InternalState.Created => WebSocketState.None, InternalState.Connecting => WebSocketState.Connecting, + InternalState.Aborted => WebSocketState.Aborted, + InternalState.Disposed => WebSocketState.Closed, _ => WebSocketState.Closed }; } @@ -163,8 +167,9 @@ internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellati _innerWebSocketCloseStatusDescription = closeEvt.GetObjectProperty("reason")?.ToString(); _receiveMessageQueue.Writer.TryWrite(new ReceivePayload(Array.Empty(), WebSocketMessageType.Close)); NativeCleanup(); - if ((InternalState)_state == InternalState.Connecting) + if ((InternalState)_state == InternalState.Connecting || (InternalState)_state == InternalState.Aborted) { + _state = (int)InternalState.Disposed; if (cancellationToken.IsCancellationRequested) { tcsConnect.TrySetCanceled(cancellationToken); @@ -176,7 +181,7 @@ internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellati } else { - _tcsClose?.SetResult(); + _tcsClose?.TrySetResult(); } } }; @@ -227,7 +232,7 @@ internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellati case OperationCanceledException: throw; default: - throw new WebSocketException(SR.net_webstatus_ConnectFailure, wse); + throw new WebSocketException(WebSocketError.Faulted, SR.net_webstatus_ConnectFailure, wse); } } finally @@ -318,23 +323,27 @@ private void NativeCleanup() public override void Dispose() { - int priorState = Interlocked.Exchange(ref _state, (int)InternalState.Disposed); - if (priorState == (int)InternalState.Disposed) + if (!_disposed) { - // No cleanup required. - return; - } + if (_state < (int)InternalState.Aborted) { + Interlocked.Exchange(ref _state, (int)InternalState.Disposed); + } + _disposed = true; - // registered by the CancellationTokenSource cts in the connect method - _cts.Cancel(false); - _cts.Dispose(); + if (!_cts.IsCancellationRequested) + { + // registered by the CancellationTokenSource cts in the connect method + _cts.Cancel(false); + _cts.Dispose(); + } - _writeBuffer?.Dispose(); - _receiveMessageQueue.Writer.Complete(); + _writeBuffer?.Dispose(); + _receiveMessageQueue.Writer.TryComplete(); - NativeCleanup(); + NativeCleanup(); - _innerWebSocket?.Dispose(); + _innerWebSocket?.Dispose(); + } } // This method is registered by the CancellationTokenSource cts in the connect method @@ -423,6 +432,18 @@ public override Task SendAsync(ArraySegment buffer, WebSocketMessageType m return Task.CompletedTask; } + // This method is registered by the CancellationTokenSource in the receive async method + private async void CancelRequest() + { + int prevState = Interlocked.Exchange(ref _state, (int)InternalState.Aborted); + _receiveMessageQueue.Writer.TryComplete(); + if (prevState == (int)InternalState.Connected || prevState == (int)InternalState.Connecting) + { + await CloseAsyncCore(WebSocketCloseStatus.NormalClosure, SR.net_WebSockets_Connection_Aborted, CancellationToken.None).ConfigureAwait(continueOnCapturedContext: true); + } + + } + /// /// Receives data on as an asynchronous operation. /// @@ -431,22 +452,38 @@ public override Task SendAsync(ArraySegment buffer, WebSocketMessageType m /// Cancellation token. public override async Task ReceiveAsync(ArraySegment buffer, CancellationToken cancellationToken) { - WebSocketValidate.ValidateArraySegment(buffer, nameof(buffer)); - ThrowIfDisposed(); - ThrowOnInvalidState(State, WebSocketState.Open, WebSocketState.CloseSent); - _bufferedPayload ??= await _receiveMessageQueue.Reader.ReadAsync(cancellationToken).ConfigureAwait(continueOnCapturedContext: true); + CancellationTokenSource _receiveCTS = new CancellationTokenSource(); + CancellationTokenRegistration receiveRegistration = cancellationToken.Register(cts => ((CancellationTokenSource)cts!).Cancel(), _receiveCTS); + _receiveCTS.Token.Register(s => ((BrowserWebSocket)s!).CancelRequest(), this); try { - bool endOfMessage = _bufferedPayload.BufferPayload(buffer, out WebSocketReceiveResult receiveResult); + WebSocketValidate.ValidateArraySegment(buffer, nameof(buffer)); + + ThrowIfDisposed(); + ThrowOnInvalidState(State, WebSocketState.Open, WebSocketState.CloseSent); + _bufferedPayload ??= await _receiveMessageQueue.Reader.ReadAsync(cancellationToken).ConfigureAwait(continueOnCapturedContext: true); + bool endOfMessage = _bufferedPayload!.BufferPayload(buffer, out WebSocketReceiveResult receiveResult); if (endOfMessage) _bufferedPayload = null; return receiveResult; } catch (Exception exc) { - throw new WebSocketException(WebSocketError.NativeError, exc); + switch (exc) + { + case OperationCanceledException: + return await Task.FromException(exc).ConfigureAwait(continueOnCapturedContext: true); + case ChannelClosedException: + return await Task.FromException(new WebSocketException(WebSocketError.InvalidState, SR.Format(SR.net_WebSockets_InvalidState, State, "Open, CloseSent"))).ConfigureAwait(continueOnCapturedContext: true); + default: + return await Task.FromException(new WebSocketException(WebSocketError.InvalidState, SR.Format(SR.net_WebSockets_InvalidState, State, "Open, CloseSent"))).ConfigureAwait(continueOnCapturedContext: true); + } + } + finally + { + receiveRegistration.Unregister(); } } @@ -455,12 +492,14 @@ public override async Task ReceiveAsync(ArraySegment public override void Abort() { - if (_state == (int)InternalState.Disposed) + if (_state != (int)InternalState.Disposed) { - return; + if (Interlocked.Exchange(ref _state, (int)InternalState.Aborted) != (int)InternalState.Aborted) + { + _cts.Cancel(true); + _tcsClose?.TrySetResult(); + } } - _state = (int)WebSocketState.Aborted; - Dispose(); } public override Task CloseAsync(WebSocketCloseStatus closeStatus, string? statusDescription, CancellationToken cancellationToken) diff --git a/src/libraries/System.Net.WebSockets.Client/tests/AbortTest.cs b/src/libraries/System.Net.WebSockets.Client/tests/AbortTest.cs index 565150cbe91a09..b53ecef3f43737 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/AbortTest.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/AbortTest.cs @@ -17,6 +17,7 @@ public AbortTest(ITestOutputHelper output) : base(output) { } [OuterLoop] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] + [PlatformSpecific(~TestPlatforms.Browser)] // Failure !!!!!! Never ends public async Task Abort_ConnectAndAbort_ThrowsWebSocketExceptionWithmessage(Uri server) { using (var cws = new ClientWebSocket()) diff --git a/src/libraries/System.Net.WebSockets.Client/tests/CancelTest.cs b/src/libraries/System.Net.WebSockets.Client/tests/CancelTest.cs index 92c44a712378a1..7744bd8913a3e5 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/CancelTest.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/CancelTest.cs @@ -147,6 +147,7 @@ public async Task ReceiveAsync_ReceiveThenCancel_ThrowsOperationCanceledExceptio [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] + [PlatformSpecific(~TestPlatforms.Browser)] // Failure !!!!!! Never ends public async Task ReceiveAsync_AfterCancellationDoReceiveAsync_ThrowsWebSocketException(Uri server) { using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output)) diff --git a/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs b/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs index 7999e4d636c295..5986af0221e92d 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs @@ -88,7 +88,7 @@ public async Task SendReceive_PartialMessageDueToSmallReceiveBuffer_Success(Uri [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/45586", TestPlatforms.Browser)] + [ActiveIssue("https://github.com/dotnet/runtime/issues/45586", TestPlatforms.Browser)] // Failure public async Task SendReceive_PartialMessageBeforeCompleteMessageArrives_Success(Uri server) { var rand = new Random(); @@ -131,7 +131,6 @@ public async Task SendReceive_PartialMessageBeforeCompleteMessageArrives_Success [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/45586", TestPlatforms.Browser)] public async Task SendAsync_SendCloseMessageType_ThrowsArgumentExceptionWithMessage(Uri server) { using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output)) @@ -219,7 +218,6 @@ public async Task SendAsync_MultipleOutstandingSendOperations_Throws(Uri server) [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/45586", TestPlatforms.Browser)] public async Task ReceiveAsync_MultipleOutstandingReceiveOperations_Throws(Uri server) { using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output)) @@ -284,7 +282,6 @@ await SendAsync( [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/45586", TestPlatforms.Browser)] public async Task SendAsync_SendZeroLengthPayloadAsEndOfMessage_Success(Uri server) { using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output)) @@ -463,7 +460,6 @@ await LoopbackServer.CreateServerAsync(async (server, url) => [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/45586", TestPlatforms.Browser)] public async Task ZeroByteReceive_CompletesWhenDataAvailable(Uri server) { using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output)) From db403cd72d114b88677b08308033090c2269c72a Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Tue, 12 Jan 2021 09:48:01 +0100 Subject: [PATCH 11/19] Fix tests that were failing when expecting CloseSent as a valid state. ``` // fail: [FAIL] System.Net.WebSockets.Client.Tests(server: ws://corefx-net-http11.azurewebsites.net/WebSocket/EchoWebSocket.ashx) // info: Assert.Throws() Failure // info: Expected: typeof(System.OperationCanceledException) // info: Actual: typeof(System.Net.WebSockets.WebSocketException): The WebSocket is in an invalid state ('Aborted') for this operation. Valid states are: 'Open, CloseSent' ``` --- .../WebSockets/BrowserWebSockets/BrowserWebSocket.cs | 10 +++++++--- .../tests/SendReceiveTest.cs | 2 ++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs index babf057df4e0b0..7ea61befdf7d8f 100644 --- a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs +++ b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs @@ -56,8 +56,9 @@ private enum InternalState Created = 0, Connecting = 1, Connected = 2, - Disposed = 3, - Aborted = 4 + CloseSent = 3, + Disposed = 4, + Aborted = 5, } private bool _disposed; @@ -90,6 +91,7 @@ public override WebSocketState State InternalState.Connecting => WebSocketState.Connecting, InternalState.Aborted => WebSocketState.Aborted, InternalState.Disposed => WebSocketState.Closed, + InternalState.CloseSent => WebSocketState.CloseSent, _ => WebSocketState.Closed }; } @@ -326,7 +328,7 @@ public override void Dispose() if (!_disposed) { if (_state < (int)InternalState.Aborted) { - Interlocked.Exchange(ref _state, (int)InternalState.Disposed); + _state = (int)InternalState.Disposed; } _disposed = true; @@ -529,6 +531,8 @@ private Task CloseAsyncCore(WebSocketCloseStatus closeStatus, string? statusDesc _innerWebSocketCloseStatus = closeStatus; _innerWebSocketCloseStatusDescription = statusDescription; _innerWebSocket!.Invoke("close", (int)closeStatus, statusDescription); + if (_state != (int)InternalState.Connecting) + _state = (int)InternalState.CloseSent; return _tcsClose.Task; } catch (Exception exc) diff --git a/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs b/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs index 5986af0221e92d..d592f7cb154e4f 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs @@ -218,6 +218,7 @@ public async Task SendAsync_MultipleOutstandingSendOperations_Throws(Uri server) [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] + [PlatformSpecific(~TestPlatforms.Browser)] // Failure !!!!!! public async Task ReceiveAsync_MultipleOutstandingReceiveOperations_Throws(Uri server) { using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output)) @@ -321,6 +322,7 @@ await SendAsync( [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] + [PlatformSpecific(~TestPlatforms.Browser)] // Failure !!!!!! public async Task SendReceive_VaryingLengthBuffers_Success(Uri server) { using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output)) From a321d6e620ae2c992983cc5851015d54b353b8e5 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Tue, 12 Jan 2021 14:37:15 +0100 Subject: [PATCH 12/19] Fix the Abort and Cancel never ending tests. - Fix the clean up of the task and set or cancel the task cleanly. --- .../BrowserWebSockets/BrowserWebSocket.cs | 22 ++++++++++++++----- .../tests/AbortTest.cs | 1 - .../tests/CancelTest.cs | 1 - 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs index 7ea61befdf7d8f..e711a835ccc3a4 100644 --- a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs +++ b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs @@ -437,13 +437,15 @@ public override Task SendAsync(ArraySegment buffer, WebSocketMessageType m // This method is registered by the CancellationTokenSource in the receive async method private async void CancelRequest() { - int prevState = Interlocked.Exchange(ref _state, (int)InternalState.Aborted); + int prevState = _state; + _state = (int)InternalState.Aborted; _receiveMessageQueue.Writer.TryComplete(); if (prevState == (int)InternalState.Connected || prevState == (int)InternalState.Connecting) { + if (prevState == (int)InternalState.Connecting) + _state = (int)InternalState.CloseSent; await CloseAsyncCore(WebSocketCloseStatus.NormalClosure, SR.net_WebSockets_Connection_Aborted, CancellationToken.None).ConfigureAwait(continueOnCapturedContext: true); } - } /// @@ -455,6 +457,11 @@ private async void CancelRequest() public override async Task ReceiveAsync(ArraySegment buffer, CancellationToken cancellationToken) { + if (cancellationToken.IsCancellationRequested) + { + return await Task.FromException(new OperationCanceledException()).ConfigureAwait(continueOnCapturedContext: true); + } + CancellationTokenSource _receiveCTS = new CancellationTokenSource(); CancellationTokenRegistration receiveRegistration = cancellationToken.Register(cts => ((CancellationTokenSource)cts!).Cancel(), _receiveCTS); _receiveCTS.Token.Register(s => ((BrowserWebSocket)s!).CancelRequest(), this); @@ -496,7 +503,13 @@ public override void Abort() { if (_state != (int)InternalState.Disposed) { - if (Interlocked.Exchange(ref _state, (int)InternalState.Aborted) != (int)InternalState.Aborted) + int prevState = _state; + if (prevState != (int)InternalState.Connecting) + { + _state = (int)InternalState.Aborted; + } + + if (prevState < (int)InternalState.Aborted) { _cts.Cancel(true); _tcsClose?.TrySetResult(); @@ -519,7 +532,6 @@ public override Task CloseAsync(WebSocketCloseStatus closeStatus, string? status { return Task.FromException(exc); } - return CloseAsyncCore(closeStatus, statusDescription, cancellationToken); } @@ -531,8 +543,6 @@ private Task CloseAsyncCore(WebSocketCloseStatus closeStatus, string? statusDesc _innerWebSocketCloseStatus = closeStatus; _innerWebSocketCloseStatusDescription = statusDescription; _innerWebSocket!.Invoke("close", (int)closeStatus, statusDescription); - if (_state != (int)InternalState.Connecting) - _state = (int)InternalState.CloseSent; return _tcsClose.Task; } catch (Exception exc) diff --git a/src/libraries/System.Net.WebSockets.Client/tests/AbortTest.cs b/src/libraries/System.Net.WebSockets.Client/tests/AbortTest.cs index b53ecef3f43737..565150cbe91a09 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/AbortTest.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/AbortTest.cs @@ -17,7 +17,6 @@ public AbortTest(ITestOutputHelper output) : base(output) { } [OuterLoop] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [PlatformSpecific(~TestPlatforms.Browser)] // Failure !!!!!! Never ends public async Task Abort_ConnectAndAbort_ThrowsWebSocketExceptionWithmessage(Uri server) { using (var cws = new ClientWebSocket()) diff --git a/src/libraries/System.Net.WebSockets.Client/tests/CancelTest.cs b/src/libraries/System.Net.WebSockets.Client/tests/CancelTest.cs index 7744bd8913a3e5..92c44a712378a1 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/CancelTest.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/CancelTest.cs @@ -147,7 +147,6 @@ public async Task ReceiveAsync_ReceiveThenCancel_ThrowsOperationCanceledExceptio [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [PlatformSpecific(~TestPlatforms.Browser)] // Failure !!!!!! Never ends public async Task ReceiveAsync_AfterCancellationDoReceiveAsync_ThrowsWebSocketException(Uri server) { using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output)) From 8754b33beb97bd185c3fc7185c34dc8baf4a0338 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Wed, 13 Jan 2021 10:22:29 +0100 Subject: [PATCH 13/19] Add ActiveIssue to websocket client SendRecieve tests --- .../BrowserWebSockets/BrowserWebSocket.cs | 18 ++++++++++-------- .../Net/WebSockets/WebSocketHandle.Browser.cs | 1 + .../tests/SendReceiveTest.cs | 2 +- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs index e711a835ccc3a4..4efc68df468cda 100644 --- a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs +++ b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs @@ -118,15 +118,17 @@ private static WebSocketState ReadyStateToDotNetState(int readyState) => internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellationToken, List? requestedSubProtocols) { - // Check that we have not started already - int priorState = Interlocked.CompareExchange(ref _state, (int)InternalState.Connecting, (int)InternalState.Created); - if (priorState == (int)InternalState.Disposed) + // Check that we have not started already. + switch ((InternalState)Interlocked.CompareExchange(ref _state, (int)InternalState.Connecting, (int)InternalState.Created)) { - throw new ObjectDisposedException(GetType().FullName); - } - else if (priorState != (int)InternalState.Created) - { - throw new InvalidOperationException(SR.net_WebSockets_AlreadyStarted); + case InternalState.Disposed: + throw new ObjectDisposedException(GetType().FullName); + + case InternalState.Created: + break; + + default: + throw new InvalidOperationException(SR.net_WebSockets_AlreadyStarted); } CancellationTokenRegistration connectRegistration = cancellationToken.Register(cts => ((CancellationTokenSource)cts!).Cancel(), _cts); diff --git a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.Browser.cs b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.Browser.cs index 9876fc126376f7..1dde3894c8dc29 100644 --- a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.Browser.cs +++ b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.Browser.cs @@ -24,6 +24,7 @@ public void Dispose() public void Abort() { + _abortSource.Cancel(); WebSocket?.Abort(); } diff --git a/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs b/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs index d592f7cb154e4f..d918734fec662d 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs @@ -322,7 +322,7 @@ await SendAsync( [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [PlatformSpecific(~TestPlatforms.Browser)] // Failure !!!!!! + [ActiveIssue("https://github.com/dotnet/runtime/issues/46909", TestPlatforms.Browser)] public async Task SendReceive_VaryingLengthBuffers_Success(Uri server) { using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output)) From e254ac0ee7408c05544218b3c59302c3950669fa Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Wed, 13 Jan 2021 10:31:40 +0100 Subject: [PATCH 14/19] Add ActiveIssue to websocket client SendRecieve tests - intermittently failing with System.OperationCanceledException : The operation was canceled. --- .../System.Net.WebSockets.Client/tests/SendReceiveTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs b/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs index d918734fec662d..9dd7e6fcb51f79 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs @@ -218,7 +218,7 @@ public async Task SendAsync_MultipleOutstandingSendOperations_Throws(Uri server) [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [PlatformSpecific(~TestPlatforms.Browser)] // Failure !!!!!! + [ActiveIssue("https://github.com/dotnet/runtime/issues/46909", TestPlatforms.Browser)] public async Task ReceiveAsync_MultipleOutstandingReceiveOperations_Throws(Uri server) { using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output)) From 6586e25986c810bf720052bac4a9d25882276def Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Wed, 13 Jan 2021 11:00:25 +0100 Subject: [PATCH 15/19] Add extra time to timeout for a couple of tests that were intermittently failing with System.OperationCanceledException : The operation was canceled. - This was an ActiveIssue https://github.com/dotnet/runtime/issues/46909 but extending the time seems to do the job. --- .../tests/ClientWebSocketTestBase.cs | 1 + .../tests/SendReceiveTest.cs | 11 +++++++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.WebSockets.Client/tests/ClientWebSocketTestBase.cs b/src/libraries/System.Net.WebSockets.Client/tests/ClientWebSocketTestBase.cs index ca24740d69cd1e..f521a8a4769544 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/ClientWebSocketTestBase.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/ClientWebSocketTestBase.cs @@ -26,6 +26,7 @@ public class ClientWebSocketTestBase }).ToArray(); public const int TimeOutMilliseconds = 20000; + public const int BrowserTimeOutMilliseconds = 30000; public const int CloseDescriptionMaxLength = 123; public readonly ITestOutputHelper _output; diff --git a/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs b/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs index 9dd7e6fcb51f79..441e147823b06c 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs @@ -218,12 +218,13 @@ public async Task SendAsync_MultipleOutstandingSendOperations_Throws(Uri server) [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/46909", TestPlatforms.Browser)] public async Task ReceiveAsync_MultipleOutstandingReceiveOperations_Throws(Uri server) { using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output)) { - var cts = new CancellationTokenSource(TimeOutMilliseconds); + // It seems that sometimes the default timeout is not enough for browser so we will extend it + // See issue https://github.com/dotnet/runtime/issues/46909 + var cts = PlatformDetection.IsBrowser ? new CancellationTokenSource(BrowserTimeOutMilliseconds) : new CancellationTokenSource(TimeOutMilliseconds); Task[] tasks = new Task[2]; @@ -322,13 +323,15 @@ await SendAsync( [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/46909", TestPlatforms.Browser)] public async Task SendReceive_VaryingLengthBuffers_Success(Uri server) { using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output)) { var rand = new Random(); - var ctsDefault = new CancellationTokenSource(TimeOutMilliseconds); + + // It seems that sometimes the default timeout is not enough for browser so we will extend it + // See issue https://github.com/dotnet/runtime/issues/46909 + var ctsDefault = PlatformDetection.IsBrowser ? new CancellationTokenSource(BrowserTimeOutMilliseconds) : new CancellationTokenSource(TimeOutMilliseconds); // Values chosen close to boundaries in websockets message length handling as well // as in vectors used in mask application. From 11fdc7dd48418bd008269f076769663351eca37a Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Thu, 14 Jan 2021 14:40:20 +0100 Subject: [PATCH 16/19] Add ActiveIssue to websocket client SendRecieve tests - [browser][websocket] Hang with responses without ever signaling "end of message" - [ActiveIssue("https://github.com/dotnet/runtime/issues/46983", TestPlatforms.Browser)] // JS Fetch does not support see issue --- .../System.Net.WebSockets.Client/tests/SendReceiveTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs b/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs index 441e147823b06c..e6a16507ae4f6b 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs @@ -88,7 +88,7 @@ public async Task SendReceive_PartialMessageDueToSmallReceiveBuffer_Success(Uri [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/45586", TestPlatforms.Browser)] // Failure + [ActiveIssue("https://github.com/dotnet/runtime/issues/46983", TestPlatforms.Browser)] // JS Fetch does not support see issue public async Task SendReceive_PartialMessageBeforeCompleteMessageArrives_Success(Uri server) { var rand = new Random(); From 90e7b79a2ee82ba1ba2dd8714a2cf551aa896515 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Mon, 18 Jan 2021 10:12:56 +0100 Subject: [PATCH 17/19] Remove `Interlocked` code as per review comment. --- .../Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs index 4efc68df468cda..b07012fc642db0 100644 --- a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs +++ b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs @@ -119,7 +119,10 @@ private static WebSocketState ReadyStateToDotNetState(int readyState) => internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellationToken, List? requestedSubProtocols) { // Check that we have not started already. - switch ((InternalState)Interlocked.CompareExchange(ref _state, (int)InternalState.Connecting, (int)InternalState.Created)) + int prevState = _state; + _state = _state == (int)InternalState.Created ? (int)InternalState.Connecting : _state; + + switch ((InternalState)prevState) { case InternalState.Disposed: throw new ObjectDisposedException(GetType().FullName); @@ -201,7 +204,9 @@ internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellati if (!cancellationToken.IsCancellationRequested) { // Change internal _state to 'Connected' to enable the other methods - if (Interlocked.CompareExchange(ref _state, (int)InternalState.Connected, (int)InternalState.Connecting) != (int)InternalState.Connecting) + int prevState = _state; + _state = _state == (int)InternalState.Connecting ? (int)InternalState.Connected : _state; + if (prevState != (int)InternalState.Connecting) { // Aborted/Disposed during connect. tcsConnect.TrySetException(new ObjectDisposedException(GetType().FullName)); From 188bdc1c8122736ba9b112d8145ec53d57c4e3b7 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Mon, 18 Jan 2021 11:17:43 +0100 Subject: [PATCH 18/19] Fix comment --- .../System.Net.WebSockets.Client/tests/SendReceiveTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs b/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs index e6a16507ae4f6b..49a4e3cb84fb7a 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs @@ -88,7 +88,7 @@ public async Task SendReceive_PartialMessageDueToSmallReceiveBuffer_Success(Uri [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/46983", TestPlatforms.Browser)] // JS Fetch does not support see issue + [ActiveIssue("https://github.com/dotnet/runtime/issues/46983", TestPlatforms.Browser)] // JS Websocket does not support see issue public async Task SendReceive_PartialMessageBeforeCompleteMessageArrives_Success(Uri server) { var rand = new Random(); From 011832288eaeac0a4b5f3548427de4a41763ca51 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Mon, 25 Jan 2021 07:19:58 +0100 Subject: [PATCH 19/19] Fix Abort tests on non chrome browsers. - Fix for those browsers that do not set Close and send an onClose event in certain instances i.e. firefox and safari. - Chrome will send an onClose event and we tear down the websocket there. Other browsers need to be handled differently. --- .../BrowserWebSockets/BrowserWebSocket.cs | 36 ++++++++++++++----- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs index b07012fc642db0..6df638d5b429d7 100644 --- a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs +++ b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs @@ -34,6 +34,7 @@ internal sealed class BrowserWebSocket : WebSocket }); private TaskCompletionSource? _tcsClose; + private TaskCompletionSource? _tcsConnect; private WebSocketCloseStatus? _innerWebSocketCloseStatus; private string? _innerWebSocketCloseStatusDescription; @@ -47,6 +48,7 @@ internal sealed class BrowserWebSocket : WebSocket private MemoryStream? _writeBuffer; private ReceivePayload? _bufferedPayload; private readonly CancellationTokenSource _cts; + private int _closeStatus; // variable to track the close status after a close is sent. // Stages of this class. private int _state; @@ -135,7 +137,7 @@ internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellati } CancellationTokenRegistration connectRegistration = cancellationToken.Register(cts => ((CancellationTokenSource)cts!).Cancel(), _cts); - TaskCompletionSource tcsConnect = new TaskCompletionSource(); + _tcsConnect = new TaskCompletionSource(); // For Abort/Dispose. Calling Abort on the request at any point will close the connection. _cts.Token.Register(s => ((BrowserWebSocket)s!).AbortRequest(), this); @@ -179,11 +181,11 @@ internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellati _state = (int)InternalState.Disposed; if (cancellationToken.IsCancellationRequested) { - tcsConnect.TrySetCanceled(cancellationToken); + _tcsConnect.TrySetCanceled(cancellationToken); } else { - tcsConnect.TrySetException(new WebSocketException(WebSocketError.NativeError)); + _tcsConnect.TrySetException(new WebSocketException(WebSocketError.NativeError)); } } else @@ -209,16 +211,16 @@ internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellati if (prevState != (int)InternalState.Connecting) { // Aborted/Disposed during connect. - tcsConnect.TrySetException(new ObjectDisposedException(GetType().FullName)); + _tcsConnect.TrySetException(new ObjectDisposedException(GetType().FullName)); } else { - tcsConnect.SetResult(); + _tcsConnect.SetResult(); } } else { - tcsConnect.SetCanceled(cancellationToken); + _tcsConnect.SetCanceled(cancellationToken); } } }; @@ -231,7 +233,7 @@ internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellati // Attach the onMessage callaback _innerWebSocket.SetObjectProperty("onmessage", _onMessage); - await tcsConnect.Task.ConfigureAwait(continueOnCapturedContext: true); + await _tcsConnect.Task.ConfigureAwait(continueOnCapturedContext: true); } catch (Exception wse) { @@ -359,9 +361,24 @@ public override void Dispose() // and called by Dispose or Abort so that any open websocket connection can be closed. private async void AbortRequest() { - if (State == WebSocketState.Open || State == WebSocketState.Connecting) + switch (State) { - await CloseAsyncCore(WebSocketCloseStatus.NormalClosure, SR.net_WebSockets_Connection_Aborted, CancellationToken.None).ConfigureAwait(continueOnCapturedContext: true); + case WebSocketState.Open: + case WebSocketState.Connecting: + { + await CloseAsyncCore(WebSocketCloseStatus.NormalClosure, SR.net_WebSockets_Connection_Aborted, CancellationToken.None).ConfigureAwait(continueOnCapturedContext: true); + // The following code is for those browsers that do not set Close and send an onClose event in certain instances i.e. firefox and safari. + // chrome will send an onClose event and we tear down the websocket there. + if (ReadyStateToDotNetState(_closeStatus) == WebSocketState.CloseSent) + { + _writeBuffer?.Dispose(); + _receiveMessageQueue.Writer.TryWrite(new ReceivePayload(Array.Empty(), WebSocketMessageType.Close)); + _receiveMessageQueue.Writer.TryComplete(); + NativeCleanup(); + _tcsConnect?.TrySetCanceled(); + } + } + break; } } @@ -550,6 +567,7 @@ private Task CloseAsyncCore(WebSocketCloseStatus closeStatus, string? statusDesc _innerWebSocketCloseStatus = closeStatus; _innerWebSocketCloseStatusDescription = statusDescription; _innerWebSocket!.Invoke("close", (int)closeStatus, statusDescription); + _closeStatus = (int)_innerWebSocket.GetObjectProperty("readyState"); return _tcsClose.Task; } catch (Exception exc)