From: Stephen Toub <stoub@microsoft.com>
Date: Fri, 29 Jan 2021 14:25:39 +0000 (-0500)
Subject: Dispose of ClientWebSocket when it fails to connect (#47616)
X-Git-Tag: submit/tizen/20210909.063632~3548
X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=5fc66480fe47ff8f4b2620901e2d33177af2dd4c;p=platform%2Fupstream%2Fdotnet%2Fruntime.git

Dispose of ClientWebSocket when it fails to connect (#47616)

Somehow when it was ported to .NET Core, the transition from connecting to connected was switched to being done before the connect rather than after.  As a result, if the connection fails and the websocket is never initialized, subsequent use (misuse) ends up null ref'ing.  This restores the .NET Framework behavior, which was to dispose of the instance if the connect fails; subsequent operations then throw ObjectDisposedException.
---

diff --git a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/ClientWebSocket.cs b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/ClientWebSocket.cs
index 4d71e31..7159283 100644
--- a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/ClientWebSocket.cs
+++ b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/ClientWebSocket.cs
@@ -82,17 +82,25 @@ namespace System.Net.WebSockets
             return ConnectAsyncCore(uri, cancellationToken);
         }
 
-        private Task ConnectAsyncCore(Uri uri, CancellationToken cancellationToken)
+        private async Task ConnectAsyncCore(Uri uri, CancellationToken cancellationToken)
         {
             _innerWebSocket = new WebSocketHandle();
 
-            // Change internal state to 'connected' to enable the other methods
-            if ((InternalState)Interlocked.CompareExchange(ref _state, (int)InternalState.Connected, (int)InternalState.Connecting) != InternalState.Connecting)
+            try
+            {
+                await _innerWebSocket.ConnectAsync(uri, cancellationToken, Options).ConfigureAwait(false);
+            }
+            catch
             {
-                return Task.FromException(new ObjectDisposedException(nameof(ClientWebSocket))); // Aborted/Disposed during connect.
+                Dispose();
+                throw;
             }
 
-            return _innerWebSocket.ConnectAsync(uri, cancellationToken, Options);
+            if ((InternalState)Interlocked.CompareExchange(ref _state, (int)InternalState.Connected, (int)InternalState.Connecting) != InternalState.Connecting)
+            {
+                Debug.Assert(_state == (int)InternalState.Disposed);
+                throw new ObjectDisposedException(GetType().FullName);
+            }
         }
 
         public override Task SendAsync(ArraySegment<byte> buffer, WebSocketMessageType messageType, bool endOfMessage, CancellationToken cancellationToken) =>
diff --git a/src/libraries/System.Net.WebSockets.Client/tests/ConnectTest.cs b/src/libraries/System.Net.WebSockets.Client/tests/ConnectTest.cs
index 68faae7..d6f9512 100644
--- a/src/libraries/System.Net.WebSockets.Client/tests/ConnectTest.cs
+++ b/src/libraries/System.Net.WebSockets.Client/tests/ConnectTest.cs
@@ -33,6 +33,12 @@ namespace System.Net.WebSockets.Client.Tests
                 }
                 Assert.Equal(WebSocketState.Closed, cws.State);
                 Assert.Equal(exceptionMessage, ex.Message);
+
+                // Other operations throw after failed connect
+                await Assert.ThrowsAsync<ObjectDisposedException>(() => cws.ReceiveAsync(new byte[1], default));
+                await Assert.ThrowsAsync<ObjectDisposedException>(() => cws.SendAsync(new byte[1], WebSocketMessageType.Binary, true, default));
+                await Assert.ThrowsAsync<ObjectDisposedException>(() => cws.CloseAsync(WebSocketCloseStatus.NormalClosure, null, default));
+                await Assert.ThrowsAsync<ObjectDisposedException>(() => cws.CloseOutputAsync(WebSocketCloseStatus.NormalClosure, null, default));
             }
         }