Fix ClientWebSocket close handshake when using proxy (dotnet/corefx#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 dotnet/corefx#28027

Commit migrated from https://github.com/dotnet/corefx/commit/0c9684c2c4057908cf697a366a3b1527326e507d

diff --git a/src/libraries/Common/src/System/Net/WebSockets/ManagedWebSocket.cs b/src/libraries/Common/src/System/Net/WebSockets/ManagedWebSocket.cs
index 86c5ba9..93a24c2 100644 (file)
--- a/src/libraries/Common/src/System/Net/WebSockets/ManagedWebSocket.cs
+++ b/src/libraries/Common/src/System/Net/WebSockets/ManagedWebSocket.cs
@@ -782,7 +782,11 @@ namespace System.Net.WebSockets
             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 @@ namespace System.Net.WebSockets
             lock (StateUpdateLock)
             {
                 _sentCloseFrame = true;
-                if (_state <= WebSocketState.CloseReceived)
+                if (_receivedCloseFrame && _state < WebSocketState.Closed)
+                {
+                    _state = WebSocketState.Closed;
+                }
+                else if (_state < WebSocketState.CloseSent)
                 {
                     _state = WebSocketState.CloseSent;
                 }

diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/LoopbackProxyServer.cs b/src/libraries/Common/tests/System/Net/Http/LoopbackProxyServer.cs
similarity index 99%
rename from src/libraries/System.Net.Http/tests/FunctionalTests/LoopbackProxyServer.cs
rename to src/libraries/Common/tests/System/Net/Http/LoopbackProxyServer.cs
index 979f3be..e278908 100644 (file)
--- a/src/libraries/System.Net.Http/tests/FunctionalTests/LoopbackProxyServer.cs
+++ b/src/libraries/Common/tests/System/Net/Http/LoopbackProxyServer.cs
@@ -12,7 +12,7 @@ using System.Text;
 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 @@ namespace System.Net.Http.Functional.Tests
                 {
                     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 @@ namespace System.Net.Http.Functional.Tests
                 {
                     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);
                     }

diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj b/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj
index 6003b5e..ba1aeac 100644 (file)
--- a/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj
+++ b/src/libraries/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" />

diff --git a/src/libraries/System.Net.WebSockets.Client/tests/ConnectTest.cs b/src/libraries/System.Net.WebSockets.Client/tests/ConnectTest.cs
index 2d83ecf..37d7e46 100644 (file)
--- a/src/libraries/System.Net.WebSockets.Client/tests/ConnectTest.cs
+++ b/src/libraries/System.Net.WebSockets.Client/tests/ConnectTest.cs
@@ -16,7 +16,7 @@ namespace System.Net.WebSockets.Client.Tests
     {
         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 @@ namespace System.Net.WebSockets.Client.Tests
             }
         }
 
-        [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 @@ namespace System.Net.WebSockets.Client.Tests
         }
 
         [ActiveIssue(18784, TargetFrameworkMonikers.NetFramework)]
-        [OuterLoop]
+        [OuterLoop("Uses external servers")]
         [ConditionalTheory(nameof(WebSocketsSupported))]
         public async Task ConnectAsync_AddHostHeader_Success()
         {
@@ -136,7 +136,7 @@ namespace System.Net.WebSockets.Client.Tests
             }
         }
 
-        [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 @@ namespace System.Net.WebSockets.Client.Tests
             }
         }
 
-        [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 @@ namespace System.Net.WebSockets.Client.Tests
             }
         }
 
-        [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 @@ namespace System.Net.WebSockets.Client.Tests
                 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);
+            }
+        }
     }
 }

diff --git a/src/libraries/System.Net.WebSockets.Client/tests/System.Net.WebSockets.Client.Tests.csproj b/src/libraries/System.Net.WebSockets.Client/tests/System.Net.WebSockets.Client.Tests.csproj
index 8849fdb..c47fa52 100644 (file)
--- a/src/libraries/System.Net.WebSockets.Client/tests/System.Net.WebSockets.Client.Tests.csproj
+++ b/src/libraries/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>