Repository navigation
Fix lock inversion in HttpListener's Windows WebSocket impl - #132314
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @karelz, @dotnet/ncl |
There was a problem hiding this comment.
Pull request overview
This PR addresses a potential deadlock in the Windows HttpListener WebSocket implementation by enforcing a consistent lock acquisition order between the state lock (_thisLock) and the SessionHandle lock, and adds a regression test intended to exercise the problematic concurrent-close interleaving.
Changes:
- Update
WebSocketBase.CloseAsyncCoreto release_thisLockbefore starting operations that acquire theSessionHandlelock (CloseOutput, receive processing, Abort). - Add a Windows-only regression test that concurrently triggers a client close frame and a server
CloseAsyncto detect deadlocks. - Add a test helper overload to create a
HttpListenerWebSocketContextusing a providedClientWebSocket.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketBase.cs | Adjusts locking behavior in CloseAsyncCore to avoid lock inversion with SessionHandle-locked receive processing. |
| src/libraries/System.Net.HttpListener/tests/HttpListenerWebSocketTests.cs | Adds a concurrency regression test and a GetWebSocketContext(ClientWebSocket) helper for setup. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketBase.cs:648
CloseAsyncCorenow releases_thisLockand then callsCloseOutputAsync(...). If another thread progresses the close handshake in the small window after the lock is released (e.g., state becomesCloseSent),CloseOutputAsynccan throw aWebSocketExceptionsynchronously (viaThrowOnInvalidStatebefore its firstawait). That synchronous throw skips the existingcloseOutputTaskexception handling and flows into the outercatch, which callsAbort()and rethrows. Consider catchingWebSocketError.InvalidStatearound theCloseOutputAsynccall and treating it as a benign race when the state has already advanced toCloseSent/terminal, while ensuring_thisLockis re-taken before proceeding.
ReleaseLock(_thisLock, ref lockTaken);
closeOutputTask = CloseOutputAsync(closeStatus,
statusDescription,
linkedCancellationToken);
src/libraries/System.Net.HttpListener/tests/HttpListenerWebSocketTests.cs:405
- This test currently swallows all
WebSocketException/InvalidOperationExceptionfrom the three concurrent tasks, which can make the test pass even if the operations fail immediately for an unexpected reason (i.e., it only fails on timeout). Tightening the catch filter to only ignore cancellation/disposal would make the test assert more than just “no deadlock.”
catch (Exception e) when (e is WebSocketException or InvalidOperationException or ObjectDisposedException or OperationCanceledException)
{
}
Closes #115559
The
TakeLockshelper used elsewhere first takes the sessionHandle lock, then the thisLock.In these cases, we would call the helper while only holding thisLock, potentially deadlocking with other calls.
runtime/src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketBase.cs
Lines 881 to 888 in cdcc43c