Skip to content

Commit 42009d1

Browse files
kjpou1lewingstephentoub
authored
[browser][websockets] Support for CloseOutputAsync (#47906)
* Set the WebSocketException to Faulted to fix tests * Fix the exceptions that are being thrown to coincide with test expectations * Fix Dispose we are not processing it multiple times. * Fix Abort code so the messages align correctly with the tests. example from tests: ``` Assert.Equal() Failure info: Expected: Aborted info: Actual: Closed ``` * Set the WebSocketException to Faulted to fix test expectations * Close the connections correctly. * 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 #45586 * - Fixes for ReceiveAsyncTest - 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. #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) ``` * Remove ActiveIssue attributes that should be fixed * Add back code after merge conflict - Update tests that are resolved. * 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' ``` * Fix the Abort and Cancel never ending tests. - Fix the clean up of the task and set or cancel the task cleanly. * Add ActiveIssue to websocket client SendRecieve tests * Add ActiveIssue to websocket client SendRecieve tests - intermittently failing with System.OperationCanceledException : The operation was canceled. * 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 #46909 but extending the time seems to do the job. * Add ActiveIssue to websocket client SendRecieve tests - [browser][websocket] Hang with responses without ever signaling "end of message" - [ActiveIssue("#46983", TestPlatforms.Browser)] // JS Fetch does not support see issue * Remove `Interlocked` code as per review comment. * Fix comment * 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. * We should not throw here. - Closing an already-closed ClientWebSocket should be a no-op. * Add `CloseOutputAsync` implementation - The browser websocket implementation does not support this so we fake it the best we can. * Remove `ActiveIssue` and add more tests - Remove `[ActiveIssue("#45468", TestPlatforms.Browser)]` - Add more tests * Address timeout implementation review * 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. * Update src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs Co-authored-by: Larry Ewing <[email protected]> * Update src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs Co-authored-by: Stephen Toub <[email protected]> * Update src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs Co-authored-by: Stephen Toub <[email protected]> * Address review comments for using `TrySet*` variants * Address review about using PascalCasing * Change test to be a platform check and not an ActiveIssue as per review Co-authored-by: Larry Ewing <[email protected]> Co-authored-by: Stephen Toub <[email protected]>
1 parent aa660d5 commit 42009d1

File tree

7 files changed

+244
-90
lines changed

7 files changed

+244
-90
lines changed

‎src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs‎

Lines changed: 194 additions & 71 deletions
Large diffs are not rendered by default.

‎src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.Browser.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ public void Dispose()
2424

2525
public void Abort()
2626
{
27+
_abortSource.Cancel();
2728
WebSocket?.Abort();
2829
}
2930

@@ -67,7 +68,7 @@ public async Task ConnectAsync(Uri uri, CancellationToken cancellationToken, Cli
6768
case OperationCanceledException _ when cancellationToken.IsCancellationRequested:
6869
throw;
6970
default:
70-
throw new WebSocketException(SR.net_webstatus_ConnectFailure, exc);
71+
throw new WebSocketException(WebSocketError.Faulted, SR.net_webstatus_ConnectFailure, exc);
7172
}
7273
}
7374
}

‎src/libraries/System.Net.WebSockets.Client/tests/AbortTest.cs‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ public AbortTest(ITestOutputHelper output) : base(output) { }
1717

1818
[OuterLoop]
1919
[ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
20-
[ActiveIssue("https://github.com/dotnet/runtime/issues/45674", TestPlatforms.Browser)]
2120
public async Task Abort_ConnectAndAbort_ThrowsWebSocketExceptionWithmessage(Uri server)
2221
{
2322
using (var cws = new ClientWebSocket())
@@ -43,7 +42,6 @@ public async Task Abort_ConnectAndAbort_ThrowsWebSocketExceptionWithmessage(Uri
4342

4443
[OuterLoop]
4544
[ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
46-
[ActiveIssue("https://github.com/dotnet/runtime/issues/45674", TestPlatforms.Browser)]
4745
public async Task Abort_SendAndAbort_Success(Uri server)
4846
{
4947
await TestCancellation(async (cws) =>
@@ -64,7 +62,6 @@ await TestCancellation(async (cws) =>
6462

6563
[OuterLoop]
6664
[ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
67-
[ActiveIssue("https://github.com/dotnet/runtime/issues/45674", TestPlatforms.Browser)]
6865
public async Task Abort_ReceiveAndAbort_Success(Uri server)
6966
{
7067
await TestCancellation(async (cws) =>
@@ -89,7 +86,6 @@ await cws.SendAsync(
8986

9087
[OuterLoop]
9188
[ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
92-
[ActiveIssue("https://github.com/dotnet/runtime/issues/45674", TestPlatforms.Browser)]
9389
public async Task Abort_CloseAndAbort_Success(Uri server)
9490
{
9591
await TestCancellation(async (cws) =>
@@ -114,7 +110,6 @@ await cws.SendAsync(
114110

115111
[OuterLoop]
116112
[ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
117-
[ActiveIssue("https://github.com/dotnet/runtime/issues/45468", TestPlatforms.Browser)]
118113
public async Task ClientWebSocket_Abort_CloseOutputAsync(Uri server)
119114
{
120115
await TestCancellation(async (cws) =>

‎src/libraries/System.Net.WebSockets.Client/tests/CancelTest.cs‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,6 @@ await cws.SendAsync(
9191

9292
[OuterLoop("Uses external servers")]
9393
[ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
94-
[ActiveIssue("https://github.com/dotnet/runtime/issues/45468", TestPlatforms.Browser)]
9594
public async Task CloseOutputAsync_Cancel_Success(Uri server)
9695
{
9796
await TestCancellation(async (cws) =>
@@ -131,7 +130,6 @@ public async Task ReceiveAsync_CancelThenReceive_ThrowsOperationCanceledExceptio
131130

132131
[OuterLoop("Uses external servers")]
133132
[ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
134-
[ActiveIssue("https://github.com/dotnet/runtime/issues/45674", TestPlatforms.Browser)]
135133
public async Task ReceiveAsync_ReceiveThenCancel_ThrowsOperationCanceledException(Uri server)
136134
{
137135
using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output))
@@ -148,7 +146,6 @@ public async Task ReceiveAsync_ReceiveThenCancel_ThrowsOperationCanceledExceptio
148146

149147
[OuterLoop("Uses external servers")]
150148
[ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
151-
[ActiveIssue("https://github.com/dotnet/runtime/issues/45674", TestPlatforms.Browser)]
152149
public async Task ReceiveAsync_AfterCancellationDoReceiveAsync_ThrowsWebSocketException(Uri server)
153150
{
154151
using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output))

‎src/libraries/System.Net.WebSockets.Client/tests/ClientWebSocketTestBase.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ public class ClientWebSocketTestBase
2525
new object[] { o[0], true }
2626
}).ToArray();
2727

28-
public const int TimeOutMilliseconds = 20000;
28+
public const int TimeOutMilliseconds = 30000;
2929
public const int CloseDescriptionMaxLength = 123;
3030
public readonly ITestOutputHelper _output;
3131

‎src/libraries/System.Net.WebSockets.Client/tests/CloseTest.cs‎

Lines changed: 46 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -158,13 +158,55 @@ public async Task CloseAsync_CloseDescriptionIsNull_Success(Uri server)
158158

159159
await cws.CloseAsync(closeStatus, closeDescription, cts.Token);
160160
Assert.Equal(closeStatus, cws.CloseStatus);
161+
}
162+
}
163+
164+
[OuterLoop("Uses external server")]
165+
[ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
166+
public async Task CloseOutputAsync_ExpectedStates(Uri server)
167+
{
168+
using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output))
169+
{
170+
var cts = new CancellationTokenSource(TimeOutMilliseconds);
171+
172+
var closeStatus = WebSocketCloseStatus.NormalClosure;
173+
string closeDescription = null;
174+
175+
await cws.CloseOutputAsync(closeStatus, closeDescription, cts.Token);
176+
Assert.True(
177+
cws.State == WebSocketState.CloseSent || cws.State == WebSocketState.Closed,
178+
$"Expected CloseSent or Closed, got {cws.State}");
179+
Assert.True(string.IsNullOrEmpty(cws.CloseStatusDescription));
180+
}
181+
}
182+
183+
[OuterLoop("Uses external server")]
184+
[ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
185+
public async Task CloseAsync_CloseOutputAsync_Throws(Uri server)
186+
{
187+
using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output))
188+
{
189+
var cts = new CancellationTokenSource(TimeOutMilliseconds);
190+
191+
var closeStatus = WebSocketCloseStatus.NormalClosure;
192+
string closeDescription = null;
193+
194+
await cws.CloseAsync(closeStatus, closeDescription, cts.Token);
195+
Assert.True(
196+
cws.State == WebSocketState.CloseSent || cws.State == WebSocketState.Closed,
197+
$"Expected CloseSent or Closed, got {cws.State}");
198+
Assert.True(string.IsNullOrEmpty(cws.CloseStatusDescription));
199+
await Assert.ThrowsAnyAsync<WebSocketException>(async () =>
200+
{ await cws.CloseOutputAsync(closeStatus, closeDescription, cts.Token); });
201+
Assert.True(
202+
cws.State == WebSocketState.CloseSent || cws.State == WebSocketState.Closed,
203+
$"Expected CloseSent or Closed, got {cws.State}");
161204
Assert.True(string.IsNullOrEmpty(cws.CloseStatusDescription));
162205
}
163206
}
164207

165208
[OuterLoop("Uses external server")]
166209
[ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
167-
[ActiveIssue("https://github.com/dotnet/runtime/issues/45468", TestPlatforms.Browser)]
168210
public async Task CloseOutputAsync_ClientInitiated_CanReceive_CanClose(Uri server)
169211
{
170212
string message = "Hello WebSockets!";
@@ -173,15 +215,16 @@ public async Task CloseOutputAsync_ClientInitiated_CanReceive_CanClose(Uri serve
173215
{
174216
var cts = new CancellationTokenSource(TimeOutMilliseconds);
175217

176-
var closeStatus = WebSocketCloseStatus.InvalidPayloadData;
218+
// See issue for Browser websocket differences https://github.com/dotnet/runtime/issues/45538
219+
var closeStatus = PlatformDetection.IsBrowser ? WebSocketCloseStatus.NormalClosure : WebSocketCloseStatus.InvalidPayloadData;
177220
string closeDescription = "CloseOutputAsync_Client_InvalidPayloadData";
178221

179222
await cws.SendAsync(WebSocketData.GetBufferFromText(message), WebSocketMessageType.Text, true, cts.Token);
180223
// Need a short delay as per WebSocket rfc6455 section 5.5.1 there isn't a requirement to receive any
181224
// data fragments after a close has been sent. The delay allows the received data fragment to be
182225
// available before calling close. The WinRT MessageWebSocket implementation doesn't allow receiving
183226
// after a call to Close.
184-
await Task.Delay(100);
227+
await Task.Delay(500);
185228
await cws.CloseOutputAsync(closeStatus, closeDescription, cts.Token);
186229

187230
// Should be able to receive the message echoed by the server.
@@ -251,7 +294,6 @@ await cws.SendAsync(
251294

252295
[OuterLoop("Uses external server")]
253296
[ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
254-
[ActiveIssue("https://github.com/dotnet/runtime/issues/45468", TestPlatforms.Browser)]
255297
public async Task CloseOutputAsync_CloseDescriptionIsNull_Success(Uri server)
256298
{
257299
using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output))

‎src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ public async Task SendReceive_PartialMessageDueToSmallReceiveBuffer_Success(Uri
8888

8989
[OuterLoop("Uses external servers")]
9090
[ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
91-
[ActiveIssue("https://github.com/dotnet/runtime/issues/45586", TestPlatforms.Browser)]
91+
[PlatformSpecific(~TestPlatforms.Browser)] // JS Websocket does not support see issue https://github.com/dotnet/runtime/issues/46983
9292
public async Task SendReceive_PartialMessageBeforeCompleteMessageArrives_Success(Uri server)
9393
{
9494
var rand = new Random();
@@ -131,7 +131,6 @@ public async Task SendReceive_PartialMessageBeforeCompleteMessageArrives_Success
131131

132132
[OuterLoop("Uses external servers")]
133133
[ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
134-
[ActiveIssue("https://github.com/dotnet/runtime/issues/45586", TestPlatforms.Browser)]
135134
public async Task SendAsync_SendCloseMessageType_ThrowsArgumentExceptionWithMessage(Uri server)
136135
{
137136
using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output))
@@ -219,7 +218,6 @@ public async Task SendAsync_MultipleOutstandingSendOperations_Throws(Uri server)
219218

220219
[OuterLoop("Uses external servers")]
221220
[ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
222-
[ActiveIssue("https://github.com/dotnet/runtime/issues/45586", TestPlatforms.Browser)]
223221
public async Task ReceiveAsync_MultipleOutstandingReceiveOperations_Throws(Uri server)
224222
{
225223
using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output))
@@ -284,7 +282,6 @@ await SendAsync(
284282

285283
[OuterLoop("Uses external servers")]
286284
[ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
287-
[ActiveIssue("https://github.com/dotnet/runtime/issues/45586", TestPlatforms.Browser)]
288285
public async Task SendAsync_SendZeroLengthPayloadAsEndOfMessage_Success(Uri server)
289286
{
290287
using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output))
@@ -463,7 +460,6 @@ await LoopbackServer.CreateServerAsync(async (server, url) =>
463460

464461
[OuterLoop("Uses external servers")]
465462
[ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
466-
[ActiveIssue("https://github.com/dotnet/runtime/issues/45586", TestPlatforms.Browser)]
467463
public async Task ZeroByteReceive_CompletesWhenDataAvailable(Uri server)
468464
{
469465
using (ClientWebSocket cws = await WebSocketHelper.GetConnectedWebSocket(server, TimeOutMilliseconds, _output))

0 commit comments

Comments
 (0)