[browser][websocket] Throw OperationCanceledException on connect

[kjpou1]

if cancel was requested before.

This fixes the test

        public async Task ConnectAsync_CancellationRequestedBeforeConnect_ThrowsOperationCanceledException()
        {
            using (var clientSocket = new ClientWebSocket())
            {
                var cts = new CancellationTokenSource();
                cts.Cancel();
                Task t = clientSocket.ConnectAsync(new Uri("ws://" + Guid.NewGuid().ToString("N")), cts.Token);
                await Assert.ThrowsAnyAsync<OperationCanceledException>(() => t);
            }
        }

https://github.com/dotnet/runtime/blob/master/src/libraries/System.Net.WebSockets.Client/tests/ConnectTest.cs#L249

Fix #44720

Part of #44666

Test was failing with the following error:

[FAIL] System.Net.WebSockets.Client.Tests.ConnectTest.ConnectAsync_CancellationRequestedBeforeConnect_ThrowsOperationCanceledException
  info: Assert.Throws() Failure
  info: Expected: typeof(System.OperationCanceledException)
  info: Actual:   typeof(System.Net.WebSockets.WebSocketException): Unable to connect to the remote server
  info: ---- System.Net.WebSockets.WebSocketException : Unable to connect to the remote server
  info: -------- System.Net.WebSockets.WebSocketException : An internal WebSocket error occurred. Please see the innerException, if present, for more details.
  info:    at System.Net.WebSockets.BrowserWebSocket.ConnectAsyncJavaScript(Uri uri, CancellationToken cancellationToken, List`1 requestedSubProtocols)
  info:    at System.Net.WebSockets.WebSocketHandle.ConnectAsync(Uri uri, CancellationToken cancellationToken, ClientWebSocketOptions options)
  info: ----- Inner Stack Trace -----
  info:    at System.Net.WebSockets.BrowserWebSocket.ConnectAsyncJavaScript(Uri uri, CancellationToken cancellationToken, List`1 requestedSubProtocols)
  info:    at System.Net.WebSockets.WebSocketHandle.ConnectAsync(Uri uri, CancellationToken cancellationToken, ClientWebSocketOptions options)
  info: ----- Inner Stack Trace -----
  info:    at System.Net.WebSockets.BrowserWebSocket.ConnectAsyncJavaScript(Uri uri, CancellationToken cancellationToken, List`1 requestedSubProtocols)

See #45470 for more tests

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Description:	if cancel was requested before. This fixes the test public async Task ConnectAsync_CancellationRequestedBeforeConnect_ThrowsOperationCanceledException() { using (var clientSocket = new ClientWebSocket()) { var cts = new CancellationTokenSource(); cts.Cancel(); Task t = clientSocket.ConnectAsync(new Uri("ws://" + Guid.NewGuid().ToString("N")), cts.Token); await Assert.ThrowsAnyAsync<OperationCanceledException>(() => t); } } https://github.com/dotnet/runtime/blob/master/src/libraries/System.Net.WebSockets.Client/tests/ConnectTest.cs#L249 Fix #44720
Author:	kjpou1
Assignees:	-
Labels:	area-System.Net
Milestone:	-

[stephentoub]

This fixes the test

I understand that this change avoids doing extra work in case cancellation was already requested, but can you elaborate on how it fixes the test? What was happening before this change?

[kjpou1]

Updated the PR comments to include the error message.

[stephentoub]

Test was failing with the following error:

But doesn't that suggest there's still a problem here? What happens if cancellation is requested while the connect is in flight? That should still produce an OperationCanceledException, but the error you pasted suggests it isn't.

[lewing]

The test change is a little racey, so it should probably be reverted until we can test each stage but it was needed to get around the first change.

[kjpou1]

I am not finding it racey. I will take a closer look but it seems to be working fine on my local machine.

[kjpou1]

Will need to setup a websocket server locally to test some of the other stages.

[lewing]

I meant in theory it might be able to fail on the host lookup before the cancellation, upon reflection that is unlikely, I'm fine either way.

[kjpou1]

I have made multiple tests with these modifications against a local server as well as against a public websocket test echo server with no problems.

The new test also works as expected. The original tests tests against a server that was not found but testing against a live server where the connection actually exists and succeeds also works.

            using (var clientSocket = new ClientWebSocket())
            {
                var cts = new CancellationTokenSource();
                //Task t = clientSocket.ConnectAsync(new Uri("ws://" + Guid.NewGuid().ToString("N")), cts.Token);
                Task t = clientSocket.ConnectAsync(new Uri("wss://echo.websocket.org"), cts.Token);
                cts.Cancel();
                await Assert.ThrowsAnyAsync<OperationCanceledException>(() => t);
            }

[lewing]

The fact checks need work.

[lewing]

@kjpou1 I've pushed a rough pass at what I was suggesting because I realized we've discussed the complications before and I might have given contradictory information then.

[lewing]

And now I'm realizing the platform check was probably written that way because of the old options throwing. If browser is the only unsupported case we can simplify it a lot more.

[kjpou1]

Agree with the changes above. The reason we did not throw if WebSocket was not found was so that we could do rudimentary tests with the javascript engines. You are right that now that we have the tests stood up via browser we can now do it this way. The testing platform checks become a lot simpler.

[kjpou1]

@lewing ping. Can you review this again?

[kjpou1]

/cc @marek-safar @stephentoub

[marek-safar]

Looks good to me but will wait on @lewing to approve