From ff3af167359d46be2c43a07f2d0749e609f606a3 Mon Sep 17 00:00:00 2001 From: Tommaso Cittadino Date: Mon, 20 Apr 2026 22:55:40 +0200 Subject: [PATCH] Fix Http2Stream single-consumer violation on WebSocket close MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a user has a pending ReceiveAsync and calls CloseAsync over an HTTP/2 WebSocket (RFC 8441), two code paths could both reach WaitForServerToCloseConnectionAsync and each issue a ReadAsync on the underlying Http2Stream: 1. HandleReceivedCloseAsync (line 1122) — inside the receive loop; holds _receiveMutex. 2. SendCloseFrameAsync (line 1566) — after sending the close frame; does not hold _receiveMutex. Both paths gate on _sentCloseFrame and _receivedCloseFrame respectively. In rare concurrent Close + Receive scenarios the two flags are set near-simultaneously, both checks pass, and both paths try to read from the stream. Http2Stream enforces a single-consumer invariant and trips Debug.Assert(!_hasWaiter), crashing the test process. The scenario is explicitly listed as supported in the class-level thread-safety contract: /// - It's acceptable to have a pending ReceiveAsync while /// CloseOutputAsync or CloseAsync is called. Guard WaitForServerToCloseConnectionAsync with an Interlocked flag so that only the first caller performs the wait; the other is a no-op. This preserves RFC 6455 section 7.1.1 behavior (the wait still runs exactly once), introduces no lock acquisition, and has no reentrancy or lock-order implications. Also clarify the long-stale comment in the test that exercises this contract — the original attributed the OperationCanceledException path to CloseAsync "receiving" the close frame, which is imprecise. The exception originates from ReceiveAsyncPrivate's catch block when the socket becomes Aborted during the close handshake (e.g. a 1s timeout in WaitForServerToCloseConnectionAsync triggers Abort). --- .../System.Net.WebSockets.Client/tests/CloseTest.cs | 10 +++++++--- .../src/System/Net/WebSockets/ManagedWebSocket.cs | 13 ++++++++++++- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.WebSockets.Client/tests/CloseTest.cs b/src/libraries/System.Net.WebSockets.Client/tests/CloseTest.cs index 9a8f33299de93e..dbcc997e932e36 100644 --- a/src/libraries/System.Net.WebSockets.Client/tests/CloseTest.cs +++ b/src/libraries/System.Net.WebSockets.Client/tests/CloseTest.cs @@ -422,9 +422,13 @@ protected async Task RunClient_CloseAsync_DuringConcurrentReceiveAsync_ExpectedS await cws.CloseAsync(WebSocketCloseStatus.NormalClosure, "", CancellationToken.None); - // There is a race condition in the above. If the ReceiveAsync receives the sent close message from the server, - // then it will complete successfully and the socket will close successfully. If the CloseAsync receive the sent - // close message from the server, then the receive async will end up getting aborted along with the socket. + // The outcome depends on timing. Two outcomes are acceptable: + // 1. The pending ReceiveAsync picks up the server's close frame cleanly: it completes with a Close + // message and the socket transitions to Closed. + // 2. The close handshake triggers an internal Abort (e.g. WaitForServerToCloseConnectionAsync + // times out because the server doesn't close TCP within its 1s window): state becomes Aborted, + // the parked receive's stream read fails, and ReceiveAsyncPrivate translates the exception to + // OperationCanceledException because of the Aborted state. try { await t; diff --git a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs index a90ed4ccf38498..69ca835f149287 100644 --- a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs +++ b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs @@ -82,6 +82,8 @@ internal sealed partial class ManagedWebSocket : WebSocket private bool _sentCloseFrame; /// Whether we've ever received a close frame. private bool _receivedCloseFrame; + /// 0 if has not been invoked yet; 1 otherwise. + private int _waitedForServerClose; /// The reason for the close, as sent by the server, or null if not yet closed. private WebSocketCloseStatus? _closeStatus; /// A description of the close reason as sent by the server, or null if not yet closed. @@ -1123,9 +1125,18 @@ private async ValueTask HandleReceivedCloseAsync(MessageHeader header, Cancellat } } - /// Issues a read on the stream to wait for EOF. + /// Issues a read on the stream to wait for EOF. Idempotent across call sites. private async ValueTask WaitForServerToCloseConnectionAsync(CancellationToken cancellationToken) { + // Called from both the receive and the send path. In rare concurrent Close + Receive + // scenarios both paths can observe the close handshake as complete and would otherwise + // both issue a stream read (violating most Stream implementations' single-consumer invariant). + // The flag guarantees that only the first invocation performs the wait; the other is a no-op. + if (Interlocked.CompareExchange(ref _waitedForServerClose, 1, 0) != 0) + { + return; + } + if (NetEventSource.Log.IsEnabled()) NetEventSource.Trace(this); // Per RFC 6455 7.1.1, try to let the server close the connection. We give it up to a second.