From 5f3d25be72c0c92a72355ed5ce52239272c53357 Mon Sep 17 00:00:00 2001 From: David Shulman Date: Tue, 16 Apr 2019 17:15:30 -0700 Subject: [PATCH] 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 --- .../src/System/Net/WebSockets/ManagedWebSocket.cs | 12 ++++++-- .../tests/System/Net/Http}/LoopbackProxyServer.cs | 6 ++-- .../System.Net.Http.Functional.Tests.csproj | 4 ++- .../tests/ConnectTest.cs | 36 +++++++++++++++++----- .../System.Net.WebSockets.Client.Tests.csproj | 3 ++ 5 files changed, 47 insertions(+), 14 deletions(-) rename src/libraries/{System.Net.Http/tests/FunctionalTests => Common/tests/System/Net/Http}/LoopbackProxyServer.cs (99%) 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 --- 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 --- 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 { /// /// 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 --- 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 @@ Common\System\Net\VerboseTestLogging.cs + + Common\System\Net\Http\LoopbackProxyServer.cs + Common\System\Net\Http\LoopbackServer.cs @@ -120,7 +123,6 @@ - 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 --- 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 --- 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 @@ Common\System\Net\Configuration.WebSockets.cs + + Common\System\Net\Http\LoopbackProxyServer.cs + Common\System\Net\Http\LoopbackServer.cs -- 2.7.4