diff --git a/src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketBase.cs b/src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketBase.cs index 982a4766538dfb..a574b3890bf844 100644 --- a/src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketBase.cs +++ b/src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketBase.cs @@ -636,6 +636,13 @@ private async Task CloseAsyncCore(WebSocketCloseStatus closeStatus, { _closeReceivedTaskCompletionSource ??= new TaskCompletionSource(); + // _thisLock MUST be released before starting the CloseOutput operation: it acquires the + // SessionHandle-lock, which MUST always be acquired before _thisLock (see TakeLocks). + // Acquiring the locks in the opposite order here would deadlock with a thread that is + // processing a received close frame, as that one holds the SessionHandle-lock while + // acquiring _thisLock in StartOnCloseReceived. + ReleaseLock(_thisLock, ref lockTaken); + closeOutputTask = CloseOutputAsync(closeStatus, statusDescription, linkedCancellationToken); @@ -716,9 +723,13 @@ private async Task CloseAsyncCore(WebSocketCloseStatus closeStatus, ArraySegment closeMessageBuffer = new ArraySegment(new byte[HttpWebSocket.MinReceiveBufferSize]); EnsureReceiveOperation(); + + // As above, _thisLock MUST be released before starting the receive operation, because + // WebSocketOperation.Process acquires the SessionHandle-lock. + ReleaseLock(_thisLock, ref lockTaken); + Task receiveAsyncTask = _receiveOperation!.Process(closeMessageBuffer, linkedCancellationToken); - ReleaseLock(_thisLock, ref lockTaken); WebSocketReceiveResult? receiveResult = null; try @@ -816,6 +827,11 @@ private async Task CloseAsyncCore(WebSocketCloseStatus closeStatus, catch (Exception exception) { bool aborted = linkedCancellationToken.IsCancellationRequested; + + // As above, _thisLock MUST be released before calling Abort, because it acquires the + // SessionHandle-lock. + ReleaseLock(_thisLock, ref lockTaken); + Abort(); ThrowIfConvertibleException(nameof(CloseAsync), exception, cancellationToken, aborted); throw; diff --git a/src/libraries/System.Net.HttpListener/tests/HttpListenerWebSocketTests.cs b/src/libraries/System.Net.HttpListener/tests/HttpListenerWebSocketTests.cs index d1ede4096ba0f8..80106810b4c648 100644 --- a/src/libraries/System.Net.HttpListener/tests/HttpListenerWebSocketTests.cs +++ b/src/libraries/System.Net.HttpListener/tests/HttpListenerWebSocketTests.cs @@ -360,6 +360,52 @@ public async Task Abort_CallAfterAborted_Nop() Assert.Equal(WebSocketState.Aborted, context.WebSocket.State); } + [ConditionalFact(typeof(Helpers), nameof(Helpers.IsWindowsImplementation))] + public async Task CloseAsync_ConcurrentWithCloseFrameFromClient_DoesNotDeadlock() + { + // Closing a WebSocket from both ends at the same time used to deadlock: the thread processing + // an incoming close frame holds the session handle lock while acquiring the state lock, while + // CloseAsync held the state lock while acquiring the session handle lock. + const int Iterations = 100; + Random random = new Random(42); + + for (int i = 0; i < Iterations; i++) + { + using ClientWebSocket client = new ClientWebSocket(); + HttpListenerWebSocketContext context = await GetWebSocketContext(client); + WebSocket server = context.WebSocket; + + // The pending receive makes a separate thread process the close frame sent by the client. + Task serverReceiveTask = IgnoreExpectedExceptionsAsync( + server.ReceiveAsync(new ArraySegment(new byte[16]), CancellationToken.None)); + + Task clientCloseTask = IgnoreExpectedExceptionsAsync( + client.CloseOutputAsync(WebSocketCloseStatus.NormalClosure, null, CancellationToken.None)); + + // Sweep the (very short) window in which the deadlock can happen. + Thread.SpinWait(random.Next(200_000)); + + Task serverCloseTask = IgnoreExpectedExceptionsAsync(Task.Run( + () => server.CloseAsync(WebSocketCloseStatus.NormalClosure, null, CancellationToken.None))); + + Task allTasks = Task.WhenAll(serverReceiveTask, clientCloseTask, serverCloseTask); + await allTasks.WaitAsync(TimeSpan.FromSeconds(30)); + + server.Dispose(); + } + + static async Task IgnoreExpectedExceptionsAsync(Task task) + { + try + { + await task; + } + catch (Exception e) when (e is WebSocketException or InvalidOperationException or ObjectDisposedException or OperationCanceledException) + { + } + } + } + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsWindows))] public async Task ReceiveAsync_ReadBuffer_WithWindowsAuthScheme_Success() { @@ -430,5 +476,23 @@ private async Task GetWebSocketContext(string[] su HttpListenerContext context = await serverContextTask; return await context.AcceptWebSocketAsync(null); } + + private async Task GetWebSocketContext(ClientWebSocket client) + { + var uriBuilder = new UriBuilder(Factory.ListeningUrl) { Scheme = "ws" }; + Task serverContextTask = Factory.GetListener().GetContextAsync(); + + Task clientConnectTask = client.ConnectAsync(uriBuilder.Uri, CancellationToken.None); + if (clientConnectTask == await Task.WhenAny(serverContextTask, clientConnectTask)) + { + await clientConnectTask; + Assert.Fail("Client should not have completed prior to server sending response"); + } + + HttpListenerContext context = await serverContextTask; + HttpListenerWebSocketContext webSocketContext = await context.AcceptWebSocketAsync(null); + await clientConnectTask; + return webSocketContext; + } } }