From e66ff5977b49a389b9eef917020063a4bd0a4190 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Wed, 9 Dec 2020 11:23:14 +0100 Subject: [PATCH 01/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] - 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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) From 4cf2b1daa37ba0e0618b4c9b81c8ba6e17178e09 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Fri, 5 Feb 2021 14:34:56 +0100 Subject: [PATCH 20/30] We should not throw here. - Closing an already-closed ClientWebSocket should be a no-op. --- .../System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs | 1 - 1 file changed, 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 6df638d5b429d7..197431408a4d72 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 @@ -544,7 +544,6 @@ public override void Abort() public override Task CloseAsync(WebSocketCloseStatus closeStatus, string? statusDescription, CancellationToken cancellationToken) { _writeBuffer = null; - ThrowIfNotConnected(); WebSocketValidate.ValidateCloseStatus(closeStatus, statusDescription); From a93d76e28b8b00cfc1ae62708e52b89b6eb9bb02 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Fri, 5 Feb 2021 14:36:16 +0100 Subject: [PATCH 21/30] Add `CloseOutputAsync` implementation - The browser websocket implementation does not support this so we fake it the best we can. --- .../BrowserWebSockets/BrowserWebSocket.cs | 33 ++++++++++++++++++- 1 file changed, 32 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 197431408a4d72..ec1e894619bad1 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 @@ -575,7 +575,38 @@ private Task CloseAsyncCore(WebSocketCloseStatus closeStatus, string? statusDesc } } - public override Task CloseOutputAsync(WebSocketCloseStatus closeStatus, string? statusDescription, CancellationToken cancellationToken) => throw new PlatformNotSupportedException(); + public override Task CloseOutputAsync(WebSocketCloseStatus closeStatus, string? statusDescription, CancellationToken cancellationToken) + { + _writeBuffer = null; + + WebSocketValidate.ValidateCloseStatus(closeStatus, statusDescription); + + try + { + ThrowOnInvalidState(State, WebSocketState.Connecting, WebSocketState.Open, WebSocketState.CloseReceived, WebSocketState.CloseSent); + } + catch (Exception exc) + { + return Task.FromException(exc); + } + return CloseOutputAsyncCore(closeStatus, statusDescription, cancellationToken); + } + + private Task CloseOutputAsyncCore(WebSocketCloseStatus closeStatus, string? statusDescription, CancellationToken cancellationToken) + { + try + { + _innerWebSocketCloseStatus = closeStatus; + _innerWebSocketCloseStatusDescription = statusDescription; + _innerWebSocket!.Invoke("close", (int)closeStatus, statusDescription); + _closeStatus = (int)_innerWebSocket.GetObjectProperty("readyState"); + return Task.CompletedTask; + } + catch (Exception exc) + { + return Task.FromException(exc); + } + } private void ThrowIfNotConnected() { From 1ccb6ead692646623c8fb2ed5380507da5fc24d4 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Fri, 5 Feb 2021 14:37:49 +0100 Subject: [PATCH 22/30] Remove `ActiveIssue` and add more tests - Remove `[ActiveIssue("https://github.com/dotnet/runtime/issues/45468", TestPlatforms.Browser)]` - Add more tests --- .../tests/AbortTest.cs | 1 - .../tests/CancelTest.cs | 1 - .../tests/CloseTest.cs | 50 +++++++++++++++++-- 3 files changed, 46 insertions(+), 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 565150cbe91a09..6e9a60f0647935 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/AbortTest.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/AbortTest.cs @@ -110,7 +110,6 @@ await cws.SendAsync( [OuterLoop] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/45468", TestPlatforms.Browser)] public async Task ClientWebSocket_Abort_CloseOutputAsync(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 92c44a712378a1..8c74513e0a0c8a 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/CancelTest.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/CancelTest.cs @@ -91,7 +91,6 @@ await cws.SendAsync( [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/45468", TestPlatforms.Browser)] public async Task CloseOutputAsync_Cancel_Success(Uri server) { await TestCancellation(async (cws) => diff --git a/src/libraries/System.Net.WebSockets.Client/tests/CloseTest.cs b/src/libraries/System.Net.WebSockets.Client/tests/CloseTest.cs index 0e50e787d58732..1f3875f8fefe98 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/CloseTest.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/CloseTest.cs @@ -158,13 +158,55 @@ public async Task CloseAsync_CloseDescriptionIsNull_Success(Uri server) await cws.CloseAsync(closeStatus, closeDescription, cts.Token); Assert.Equal(closeStatus, cws.CloseStatus); + } + } + + [OuterLoop("Uses external server")] + [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] + public async Task CloseOutputAsync_ExpectedStates(Uri server) + { + using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output)) + { + var cts = new CancellationTokenSource(TimeOutMilliseconds); + + var closeStatus = WebSocketCloseStatus.NormalClosure; + string closeDescription = null; + + await cws.CloseOutputAsync(closeStatus, closeDescription, cts.Token); + Assert.True( + cws.State == WebSocketState.CloseSent || cws.State == WebSocketState.Closed, + $"Expected CloseSent or Closed, got {cws.State}"); + Assert.True(string.IsNullOrEmpty(cws.CloseStatusDescription)); + } + } + + [OuterLoop("Uses external server")] + [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] + public async Task CloseAsync_CloseOutputAsync_Throws(Uri server) + { + using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output)) + { + var cts = new CancellationTokenSource(TimeOutMilliseconds); + + var closeStatus = WebSocketCloseStatus.NormalClosure; + string closeDescription = null; + + await cws.CloseAsync(closeStatus, closeDescription, cts.Token); + Assert.True( + cws.State == WebSocketState.CloseSent || cws.State == WebSocketState.Closed, + $"Expected CloseSent or Closed, got {cws.State}"); + Assert.True(string.IsNullOrEmpty(cws.CloseStatusDescription)); + await Assert.ThrowsAnyAsync(async () => + { await cws.CloseOutputAsync(closeStatus, closeDescription, cts.Token); }); + Assert.True( + cws.State == WebSocketState.CloseSent || cws.State == WebSocketState.Closed, + $"Expected CloseSent or Closed, got {cws.State}"); Assert.True(string.IsNullOrEmpty(cws.CloseStatusDescription)); } } [OuterLoop("Uses external server")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/45468", TestPlatforms.Browser)] public async Task CloseOutputAsync_ClientInitiated_CanReceive_CanClose(Uri server) { string message = "Hello WebSockets!"; @@ -173,7 +215,8 @@ public async Task CloseOutputAsync_ClientInitiated_CanReceive_CanClose(Uri serve { var cts = new CancellationTokenSource(TimeOutMilliseconds); - var closeStatus = WebSocketCloseStatus.InvalidPayloadData; + // See issue for Browser websocket differences https://github.com/dotnet/runtime/issues/45538 + var closeStatus = PlatformDetection.IsBrowser ? WebSocketCloseStatus.NormalClosure : WebSocketCloseStatus.InvalidPayloadData; string closeDescription = "CloseOutputAsync_Client_InvalidPayloadData"; await cws.SendAsync(WebSocketData.GetBufferFromText(message), WebSocketMessageType.Text, true, cts.Token); @@ -181,7 +224,7 @@ public async Task CloseOutputAsync_ClientInitiated_CanReceive_CanClose(Uri serve // data fragments after a close has been sent. The delay allows the received data fragment to be // available before calling close. The WinRT MessageWebSocket implementation doesn't allow receiving // after a call to Close. - await Task.Delay(100); + await Task.Delay(PlatformDetection.IsBrowser ? 500 : 100); // default timeout is not enough for browser sometimes so we will extend it await cws.CloseOutputAsync(closeStatus, closeDescription, cts.Token); // Should be able to receive the message echoed by the server. @@ -251,7 +294,6 @@ await cws.SendAsync( [OuterLoop("Uses external server")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/45468", TestPlatforms.Browser)] public async Task CloseOutputAsync_CloseDescriptionIsNull_Success(Uri server) { using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output)) From fb8596a2b1ad8afab54fb9c9c539b019a5b5cf2c Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Tue, 9 Feb 2021 07:20:34 +0100 Subject: [PATCH 23/30] Address timeout implementation review --- .../tests/ClientWebSocketTestBase.cs | 3 +-- .../System.Net.WebSockets.Client/tests/CloseTest.cs | 2 +- .../System.Net.WebSockets.Client/tests/SendReceiveTest.cs | 8 ++------ 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Net.WebSockets.Client/tests/ClientWebSocketTestBase.cs b/src/libraries/System.Net.WebSockets.Client/tests/ClientWebSocketTestBase.cs index f521a8a4769544..ab8ebe20f32fe1 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/ClientWebSocketTestBase.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/ClientWebSocketTestBase.cs @@ -25,8 +25,7 @@ public class ClientWebSocketTestBase new object[] { o[0], true } }).ToArray(); - public const int TimeOutMilliseconds = 20000; - public const int BrowserTimeOutMilliseconds = 30000; + public const int TimeOutMilliseconds = 30000; public const int CloseDescriptionMaxLength = 123; public readonly ITestOutputHelper _output; diff --git a/src/libraries/System.Net.WebSockets.Client/tests/CloseTest.cs b/src/libraries/System.Net.WebSockets.Client/tests/CloseTest.cs index 1f3875f8fefe98..f4dc4bbbe48fc8 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/CloseTest.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/CloseTest.cs @@ -224,7 +224,7 @@ public async Task CloseOutputAsync_ClientInitiated_CanReceive_CanClose(Uri serve // data fragments after a close has been sent. The delay allows the received data fragment to be // available before calling close. The WinRT MessageWebSocket implementation doesn't allow receiving // after a call to Close. - await Task.Delay(PlatformDetection.IsBrowser ? 500 : 100); // default timeout is not enough for browser sometimes so we will extend it + await Task.Delay(500); await cws.CloseOutputAsync(closeStatus, closeDescription, cts.Token); // Should be able to receive the message echoed by the server. diff --git a/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs b/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs index 49a4e3cb84fb7a..d716c089be58ef 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs @@ -222,9 +222,7 @@ public async Task ReceiveAsync_MultipleOutstandingReceiveOperations_Throws(Uri s { using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output)) { - // 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); + var cts = new CancellationTokenSource(TimeOutMilliseconds); Task[] tasks = new Task[2]; @@ -329,9 +327,7 @@ public async Task SendReceive_VaryingLengthBuffers_Success(Uri server) { var rand = new Random(); - // 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); + var ctsDefault = new CancellationTokenSource(TimeOutMilliseconds); // Values chosen close to boundaries in websockets message length handling as well // as in vectors used in mask application. From 59d03836ab7dbe079008f2e9d4c5a0b412e1190f Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Tue, 9 Feb 2021 12:04:50 +0100 Subject: [PATCH 24/30] Add code as per SignalR comments to prevent long wait times in some cases As per comments - We clear all events on the websocket (including onClose), - call websocket.close() - then call the user provided onClose method manually. --- .../BrowserWebSockets/BrowserWebSocket.cs | 68 +++++++++++-------- 1 file changed, 40 insertions(+), 28 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 ec1e894619bad1..38594ab99df2c0 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 @@ -42,7 +42,7 @@ internal sealed class BrowserWebSocket : WebSocket private Action? _onOpen; private Action? _onError; - private Action? _onClose; + private Action? _onClose; private Action? _onMessage; private MemoryStream? _writeBuffer; @@ -168,32 +168,7 @@ internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellati _innerWebSocket.SetObjectProperty("onerror", _onError); // Setup the onClose callback - _onClose = (closeEvt) => - { - using (closeEvt) - { - _innerWebSocketCloseStatus = (WebSocketCloseStatus)closeEvt.GetObjectProperty("code"); - _innerWebSocketCloseStatusDescription = closeEvt.GetObjectProperty("reason")?.ToString(); - _receiveMessageQueue.Writer.TryWrite(new ReceivePayload(Array.Empty(), WebSocketMessageType.Close)); - NativeCleanup(); - if ((InternalState)_state == InternalState.Connecting || (InternalState)_state == InternalState.Aborted) - { - _state = (int)InternalState.Disposed; - if (cancellationToken.IsCancellationRequested) - { - _tcsConnect.TrySetCanceled(cancellationToken); - } - else - { - _tcsConnect.TrySetException(new WebSocketException(WebSocketError.NativeError)); - } - } - else - { - _tcsClose?.TrySetResult(); - } - } - }; + _onClose = (closeEvent) => onCloseCallback(closeEvent, cancellationToken); // Attach the onClose callback _innerWebSocket.SetObjectProperty("onclose", _onClose); @@ -252,6 +227,37 @@ internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellati } } + + private void onCloseCallback(JSObject? closeEvt, CancellationToken cancellationToken) + { + if (closeEvt != null) + { + using (closeEvt) + { + _innerWebSocketCloseStatus = (WebSocketCloseStatus)closeEvt.GetObjectProperty("code"); + _innerWebSocketCloseStatusDescription = closeEvt.GetObjectProperty("reason")?.ToString(); + } + } + _receiveMessageQueue.Writer.TryWrite(new ReceivePayload(Array.Empty(), WebSocketMessageType.Close)); + NativeCleanup(); + if ((InternalState)_state == InternalState.Connecting || (InternalState)_state == InternalState.Aborted) + { + _state = (int)InternalState.Disposed; + if (cancellationToken.IsCancellationRequested) + { + _tcsConnect?.TrySetCanceled(cancellationToken); + } + else + { + _tcsConnect?.TrySetException(new WebSocketException(WebSocketError.NativeError)); + } + } + else + { + _tcsClose?.TrySetResult(); + } + } + private void onMessageCallback(JSObject messageEvent) { // get the events "data" @@ -555,7 +561,7 @@ public override Task CloseAsync(WebSocketCloseStatus closeStatus, string? status { return Task.FromException(exc); } - return CloseAsyncCore(closeStatus, statusDescription, cancellationToken); + return State == WebSocketState.CloseSent ? Task.CompletedTask : CloseAsyncCore(closeStatus, statusDescription, cancellationToken); } private Task CloseAsyncCore(WebSocketCloseStatus closeStatus, string? statusDescription, CancellationToken cancellationToken) @@ -596,10 +602,16 @@ private Task CloseOutputAsyncCore(WebSocketCloseStatus closeStatus, string? stat { try { + // as per comments + // - We clear all events on the websocket (including onClose), + // - call websocket.close() + // - then call the user provided onClose method manually. + NativeCleanup(); _innerWebSocketCloseStatus = closeStatus; _innerWebSocketCloseStatusDescription = statusDescription; _innerWebSocket!.Invoke("close", (int)closeStatus, statusDescription); _closeStatus = (int)_innerWebSocket.GetObjectProperty("readyState"); + onCloseCallback(null, cancellationToken); return Task.CompletedTask; } catch (Exception exc) From 0fb4017a25ca1cd2a339c5f37d8579d3ba114d22 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Wed, 10 Feb 2021 17:42:38 +0100 Subject: [PATCH 25/30] Update src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs Co-authored-by: Larry Ewing --- .../System.Net.WebSockets.Client/tests/SendReceiveTest.cs | 1 - 1 file changed, 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 d716c089be58ef..358f6649240b0e 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs @@ -326,7 +326,6 @@ 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); // Values chosen close to boundaries in websockets message length handling as well From ea5a787d919be1df674851dbc4411adafa98c755 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Thu, 11 Feb 2021 05:40:15 +0100 Subject: [PATCH 26/30] Update src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs Co-authored-by: Stephen Toub --- .../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 38594ab99df2c0..a22cd08406bc85 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 @@ -350,7 +350,7 @@ public override void Dispose() if (!_cts.IsCancellationRequested) { // registered by the CancellationTokenSource cts in the connect method - _cts.Cancel(false); + _cts.Cancel(); _cts.Dispose(); } From 543b31aa9f411875f07f4b8578ef6b60f844bba4 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Thu, 11 Feb 2021 05:41:14 +0100 Subject: [PATCH 27/30] Update src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs Co-authored-by: Stephen Toub --- .../Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs | 5 ++++- 1 file changed, 4 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 a22cd08406bc85..e2b2ff63d29daf 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 @@ -122,7 +122,10 @@ internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellati { // Check that we have not started already. int prevState = _state; - _state = _state == (int)InternalState.Created ? (int)InternalState.Connecting : _state; + if (prevState == (int)InternalState.Created) + { + _state = (int)InternalState.Connecting; + } switch ((InternalState)prevState) { From a50f11a135d4c426dec561259f993f08a9795f87 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Thu, 11 Feb 2021 06:44:09 +0100 Subject: [PATCH 28/30] Address review comments for using `TrySet*` variants --- .../Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs | 4 ++-- 1 file changed, 2 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 e2b2ff63d29daf..c39241d4fbf959 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 @@ -193,12 +193,12 @@ internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellati } else { - _tcsConnect.SetResult(); + _tcsConnect.TrySetResult(); } } else { - _tcsConnect.SetCanceled(cancellationToken); + _tcsConnect.TrySetCanceled(cancellationToken); } } }; From baddb82905148a73940af841787d825c73d0ee86 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Thu, 11 Feb 2021 06:56:53 +0100 Subject: [PATCH 29/30] Address review about using PascalCasing --- .../WebSockets/BrowserWebSockets/BrowserWebSocket.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 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 c39241d4fbf959..ac05989f6ea862 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 @@ -171,7 +171,7 @@ internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellati _innerWebSocket.SetObjectProperty("onerror", _onError); // Setup the onClose callback - _onClose = (closeEvent) => onCloseCallback(closeEvent, cancellationToken); + _onClose = (closeEvent) => OnCloseCallback(closeEvent, cancellationToken); // Attach the onClose callback _innerWebSocket.SetObjectProperty("onclose", _onClose); @@ -207,7 +207,7 @@ internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellati _innerWebSocket.SetObjectProperty("onopen", _onOpen); // Setup the onMessage callback - _onMessage = (messageEvent) => onMessageCallback(messageEvent); + _onMessage = (messageEvent) => OnMessageCallback(messageEvent); // Attach the onMessage callaback _innerWebSocket.SetObjectProperty("onmessage", _onMessage); @@ -231,7 +231,7 @@ internal async Task ConnectAsyncJavaScript(Uri uri, CancellationToken cancellati } - private void onCloseCallback(JSObject? closeEvt, CancellationToken cancellationToken) + private void OnCloseCallback(JSObject? closeEvt, CancellationToken cancellationToken) { if (closeEvt != null) { @@ -261,7 +261,7 @@ private void onCloseCallback(JSObject? closeEvt, CancellationToken cancellationT } } - private void onMessageCallback(JSObject messageEvent) + private void OnMessageCallback(JSObject messageEvent) { // get the events "data" using (messageEvent) @@ -614,7 +614,7 @@ private Task CloseOutputAsyncCore(WebSocketCloseStatus closeStatus, string? stat _innerWebSocketCloseStatusDescription = statusDescription; _innerWebSocket!.Invoke("close", (int)closeStatus, statusDescription); _closeStatus = (int)_innerWebSocket.GetObjectProperty("readyState"); - onCloseCallback(null, cancellationToken); + OnCloseCallback(null, cancellationToken); return Task.CompletedTask; } catch (Exception exc) From a6622b416491820badf6ee0f74aaab957ffd2b48 Mon Sep 17 00:00:00 2001 From: Kenneth Pouncey Date: Thu, 11 Feb 2021 07:04:32 +0100 Subject: [PATCH 30/30] Change test to be a platform check and not an ActiveIssue as per review --- .../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 358f6649240b0e..5e8a1adb5547e7 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 Websocket does not support see issue + [PlatformSpecific(~TestPlatforms.Browser)] // JS Websocket does not support see issue https://github.com/dotnet/runtime/issues/46983 public async Task SendReceive_PartialMessageBeforeCompleteMessageArrives_Success(Uri server) { var rand = new Random();