Fix ClientWebSocket close handshake when using proxy (#36928)

When the client websocket was going through a proxy, it would sometimes transition to the
'Aborted' state and not the 'Closed' state after a successful closing handshake.

I first opened this bug a year ago but finally was able to get back to it and root cause the
problem. The problem only occured with the managed websocket. The .NET Framework did not have
this problem. And some proxies didn't cause a problem with managed websocket (such as Fiddler).

The root cause is a misinterpretation of RFC 6455, Section 7.1.1. This describes the behavior
of how the websocket's TCP connection should be closed. The most graceful way is to wait for
the server to initiate the close of the socket. In cases where it is taking too long to wait
for the server to start the TCP close, then the client can start the TCP close of the socket.

But no matter how the socket is finally closed, the websocket state should transition to 'Closed'
as soon as a valid closing handshake was performed (i.e. close frames both sent and received).
And this occurs before any logic for closing the TCP connection.

The code in managed websocket has a timer to wait 1 second for the server to start a close. If
the timer finishes, then the managed websocket calls an Abort() method to close the socket. This
ends up transitioning the websocket to an 'Aborted' state which is incorrect. The state should
only be moved to the 'Aborted' state if a closing handshake was not completed.

I added a new test to support this change and moved the LoopbackProxyServer code to Common.

Fixes #28027

src/Common/src/System/Net/WebSockets/ManagedWebSocket.cs

@@ -782,7 +782,11 @@ private async Task HandleReceivedCloseAsync(MessageHeader header, CancellationTo
             lock (StateUpdateLock)
             {
                 _receivedCloseFrame = true;
-                if (_state < WebSocketState.CloseReceived)
+                if (_sentCloseFrame && _state < WebSocketState.Closed)
+                {
+                    _state = WebSocketState.Closed;
+                }
+                else if (_state < WebSocketState.CloseReceived)
                 {
                     _state = WebSocketState.CloseReceived;
                 }
@@ -1153,7 +1157,11 @@ private async Task SendCloseFrameAsync(WebSocketCloseStatus closeStatus, string
             lock (StateUpdateLock)
             {
                 _sentCloseFrame = true;
-                if (_state <= WebSocketState.CloseReceived)
+                if (_receivedCloseFrame && _state < WebSocketState.Closed)
+                {
+                    _state = WebSocketState.Closed;
+                }
+                else if (_state < WebSocketState.CloseSent)
                 {
                     _state = WebSocketState.CloseSent;
                 }

src/System.Net.Http/tests/FunctionalTests/LoopbackProxyServer.cs → src/Common/tests/System/Net/Http/LoopbackProxyServer.cs

@@ -12,7 +12,7 @@
 using System.Threading;
 using System.Threading.Tasks;
 
-namespace System.Net.Http.Functional.Tests
+namespace System.Net.Test.Common
 {
     /// <summary>
     /// Provides a test-only HTTP proxy. Handles multiple connections/requests and CONNECT tunneling for HTTPS
@@ -210,7 +210,7 @@ private async Task ProcessConnectMethod(NetworkStream clientStream, string remot
                 {
                     byte[] buffer = new byte[8000];
                     int bytesRead;
-                    while ((bytesRead = await clientStream.ReadAsync(buffer)) > 0)
+                    while ((bytesRead = await clientStream.ReadAsync(buffer, 0, buffer.Length)) > 0)
                     {
                         await serverStream.WriteAsync(buffer, 0, bytesRead);
                     }
@@ -228,7 +228,7 @@ private async Task ProcessConnectMethod(NetworkStream clientStream, string remot
                 {
                     byte[] buffer = new byte[8000];
                     int bytesRead;
-                    while ((bytesRead = await serverStream.ReadAsync(buffer)) > 0)
+                    while ((bytesRead = await serverStream.ReadAsync(buffer, 0, buffer.Length)) > 0)
                     {
                         await clientStream.WriteAsync(buffer, 0, bytesRead);
                     }

src/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj

@@ -65,6 +65,9 @@
     <Compile Include="$(CommonTestPath)\System\Net\VerboseTestLogging.cs">
       <Link>Common\System\Net\VerboseTestLogging.cs</Link>
     </Compile>
+    <Compile Include="$(CommonTestPath)\System\Net\Http\LoopbackProxyServer.cs">
+      <Link>Common\System\Net\Http\LoopbackProxyServer.cs</Link>
+    </Compile>
     <Compile Include="$(CommonTestPath)\System\Net\Http\LoopbackServer.cs">
       <Link>Common\System\Net\Http\LoopbackServer.cs</Link>
     </Compile>
@@ -120,7 +123,6 @@
     <Compile Include="HttpProtocolTests.cs" />
     <Compile Include="HttpRequestMessageTest.cs" />
     <Compile Include="HttpResponseMessageTest.cs" />
-    <Compile Include="LoopbackProxyServer.cs" />
     <Compile Include="MessageProcessingHandlerTest.cs" />
     <Compile Include="MultipartContentTest.cs" />
     <Compile Include="MultipartFormDataContentTest.cs" />

src/System.Net.WebSockets.Client/tests/ConnectTest.cs

@@ -16,7 +16,7 @@ public class ConnectTest : ClientWebSocketTestBase
     {
         public ConnectTest(ITestOutputHelper output) : base(output) { }
 
-        [OuterLoop] // TODO: Issue #11345
+        [OuterLoop("Uses external servers")]
         [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(UnavailableWebSocketServers))]
         public async Task ConnectAsync_NotWebSocketServer_ThrowsWebSocketExceptionWithMessage(Uri server, string exceptionMessage, WebSocketError errorCode)
         {
@@ -40,21 +40,21 @@ public async Task ConnectAsync_NotWebSocketServer_ThrowsWebSocketExceptionWithMe
             }
         }
 
-        [OuterLoop] // TODO: Issue #11345
+        [OuterLoop("Uses external servers")]
         [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
         public async Task EchoBinaryMessage_Success(Uri server)
         {
             await WebSocketHelper.TestEcho(server, WebSocketMessageType.Binary, TimeOutMilliseconds, _output);
         }
 
-        [OuterLoop] // TODO: Issue #11345
+        [OuterLoop("Uses external servers")]
         [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
         public async Task EchoTextMessage_Success(Uri server)
         {
             await WebSocketHelper.TestEcho(server, WebSocketMessageType.Text, TimeOutMilliseconds, _output);
         }
 
-        [OuterLoop] // TODO: Issue #11345
+        [OuterLoop("Uses external servers")]
         [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoHeadersServers))]
         public async Task ConnectAsync_AddCustomHeaders_Success(Uri server)
         {
@@ -92,7 +92,7 @@ public async Task ConnectAsync_AddCustomHeaders_Success(Uri server)
         }
 
         [ActiveIssue(18784, TargetFrameworkMonikers.NetFramework)]
-        [OuterLoop]
+        [OuterLoop("Uses external servers")]
         [ConditionalTheory(nameof(WebSocketsSupported))]
         public async Task ConnectAsync_AddHostHeader_Success()
         {
@@ -136,7 +136,7 @@ public async Task ConnectAsync_AddHostHeader_Success()
             }
         }
 
-        [OuterLoop] // TODO: Issue #11345
+        [OuterLoop("Uses external servers")]
         [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoHeadersServers))]
         public async Task ConnectAsync_CookieHeaders_Success(Uri server)
         {
@@ -185,7 +185,7 @@ public async Task ConnectAsync_CookieHeaders_Success(Uri server)
             }
         }
 
-        [OuterLoop] // TODO: Issue #11345
+        [OuterLoop("Uses external servers")]
         [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
         public async Task ConnectAsync_PassNoSubProtocol_ServerRequires_ThrowsWebSocketException(Uri server)
         {
@@ -210,7 +210,7 @@ public async Task ConnectAsync_PassNoSubProtocol_ServerRequires_ThrowsWebSocketE
             }
         }
 
-        [OuterLoop] // TODO: Issue #11345
+        [OuterLoop("Uses external servers")]
         [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
         public async Task ConnectAsync_PassMultipleSubProtocols_ServerRequires_ConnectionUsesAgreedSubProtocol(Uri server)
         {
@@ -248,5 +248,25 @@ await LoopbackServer.CreateClientAndServerAsync(async uri =>
                 Assert.True(await LoopbackHelper.WebSocketHandshakeAsync(connection));
             }), new LoopbackServer.Options { WebSocketEndpoint = true });
         }
+
+        [OuterLoop("Uses external servers")]
+        [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))]
+        public async Task ConnectAndCloseAsync_UseProxyServer_ExpectedClosedState(Uri server)
+        {
+            using (var cws = new ClientWebSocket())
+            using (var cts = new CancellationTokenSource(TimeOutMilliseconds))
+            using (LoopbackProxyServer proxyServer = LoopbackProxyServer.Create())
+            {
+                cws.Options.Proxy = new WebProxy(proxyServer.Uri);
+                await cws.ConnectAsync(server, cts.Token);
+
+                string expectedCloseStatusDescription = "Client close status";
+                await cws.CloseAsync(WebSocketCloseStatus.NormalClosure, expectedCloseStatusDescription, cts.Token);
+
+                Assert.Equal(WebSocketState.Closed, cws.State);
+                Assert.Equal(WebSocketCloseStatus.NormalClosure, cws.CloseStatus);
+                Assert.Equal(expectedCloseStatusDescription, cws.CloseStatusDescription);
+            }
+        }
     }
 }

src/System.Net.WebSockets.Client/tests/System.Net.WebSockets.Client.Tests.csproj

@@ -28,6 +28,9 @@
     <Compile Include="$(CommonTestPath)\System\Net\Configuration.WebSockets.cs">
       <Link>Common\System\Net\Configuration.WebSockets.cs</Link>
     </Compile>
+    <Compile Include="$(CommonTestPath)\System\Net\Http\LoopbackProxyServer.cs">
+      <Link>Common\System\Net\Http\LoopbackProxyServer.cs</Link>
+    </Compile>
     <Compile Include="$(CommonTestPath)\System\Net\Http\LoopbackServer.cs">
       <Link>Common\System\Net\Http\LoopbackServer.cs</Link>
     </Compile>