CloseAsync race condition in MangedWebSocket
- Dominant language
- C#
- Stars
- 18.3k
- Forks
- 5.6k
- PR merge metrics
- PR metrics pending
Description
### Description
ReceiveAsync can receive and handle the close frame, after `CloseAsync` has started the internal receive task.
Then, when the `CloseAsync`->`ReceiveAsyncPrivate` enters the receiveMutex it will throw because the WebSocket has been disposed. This results in a "failed" `CloseAsync`, despite the closing handshake and connection termination being successful.
Here is an outline of the events (there are some slight variants in logging, its a race condition after all):
[i will use the name `ReceiveAsync` to indicate the user calls and `ReceiveAsyncPrivate` for the call from within `CloseAsync`, just for easier comprehension, also most of `CloseAsync` is technically `CloseAsyncPrivate`]
`ReceiveAsync` enters mutex and waits for message
`CloseAsync` send close message
`CloseAsync` waits for mutex to test whether a close frame has already been received
`ReceiveAsync` receives any message and returns it (unblocks mutex)
`CloseAsync` enters mutex
`ReceiveAsync` is called again by the receiveLoop (waits for mutex)
`CloseAsync` determines that the close frame is missing and internally calls `ReceiveAsyncPrivate`
`ReceiveAsyncPrivate` queues for the mutex as well
`CloseAsync` releases mutex
`ReceiveAsync` enters mutex
`ReceiveAsync` receives close frame and handles it (since the new state is Closed, this involves Abort() and therefore Dispose)
`ReceiveAsync` leaves mutex and completes (the state is now Closed, so the receiveLoop stops)
`ReceiveAsyncPrivate` enters mutex
`ReceiveAsyncPrivate` calls ThrowIfDisposed, which throws
`ReceiveAsyncPrivate` wraps the error in a WebSocketException (WRONGLY `ConnectionClosedPrematurely`). This wasted a lot of time during my initial debugging
`ReceiveAsyncPrivate` then leaves mutex
`CloseAsync` rethrows that WebSocketException, giving the wrong impression that CloseAsync failed
\
\
There are multiple issues here, that i think should be addressed:
1. `CloseAsync` should not throw after a successful closing handshake has taken place. I propose a check in the catch in `CloseAsyncPrivate` currently used for logging. If the _state is Closed, don't rethrow the error.
2. The logic for which WebSocketException to throw in ReceiveAsyncPrivate is faulty. It just always throws `ConnectionClosedPrematurely`. This should probably be a separate `WebSocketClosed` version. The first point might seem to address the wrong exception being propagated to the user. However during my many tests i also had an instance where this error was thrown in the receiveLoop `ReceiveAsync` instead. It is still incorrect here and it would be nice if the user does not receive wrong and misleading error messages. Some kind of `WebSocketClosed` would clearly indicate that ReceiveAsync threw because another thread (correctly) closed the connection while it was waiting for messages.
3. A very minor thing, but the internal documentation for `WaitForServerToCloseConnectionAsync` does not make it clear that the catch is the happy path. It would be nice for people like me who a re not super familiar with this websocket implementation to make it explicit. Either by moving the Abort in a finally or through a comment.
### Reproduction Steps
Attached is short no dependency script that can reproduce the error consistently. It also produces a log that can be used to understand the exact order of events outlined above that leads to the incorrect error.
This script was used to reproduce the error on an m1 mac, an M4 mac and windows, all with .net10.
.net 8 and before will not produce the log because NetEventSource was only added in .net9. But the error thrown in the one .net8 test i ran looked like it also has the same kind of issue.
[Program.cs](https://github.com/user-attachments/files/30830001/Program.cs)
I can provide my collected logs if desired/neddded.
### Expected behavior
CloseAsync should not throw an exception at all if the connection was closed successfully. Also ReceiveAsync(Private) should not throw a WebSocketException that indicates failure to complete the closing handshake, when the closing handshake was in fact successful.
### Actual behavior
ReceiveAsyncPrivate throws a WebSocketException(WebSocketError.ConnectionClosedPrematurely) if another receive handled the close frame. This leads to either CloseAsync wrongly failing or ReceiveAsync throwing. Both with the objectively false ConnectionClosedPrematurely error message.
### Regression?
Not since .net8 at least, but without the internal logs i did not want to try older versions.
### Known Workarounds
You can wrap the CloseAsync in a try catch and check whether the WebSocket.State is Closed. If yes then it can be ignored. The same can be done for ReceiveAsync in a receiveLoop.
### Configuration
Most of my tests were run on:
- M1 Macbook Pro, Tahoe 26.5.2, .net 10.0.8
I could easily reproduce on:
- M4 Macbook Pro, Sequoia 15.7.4, .net 10.0.3
- Windows (don't know details, i just asked a friend), .net 10.0.10
On windows the first test was with .net 8.0.13, without the internal logs i can only confirm that the exception thrown by CloseAsync is the same (and the State is still Closed, so almost certainly the same issue).
### Other information
_No response_
Contributor guide
Research direction
Start with the CloseAsyncPrivate, ReceiveAsyncPrivate, and WaitForServerToCloseConnectionAsync paths described in the issue, then run the attached Program.cs reproduction to observe the race and exception. Compare the state and exception behavior when another receive handles the close frame. Done means a successful closing handshake does not make CloseAsync fail, and ReceiveAsync reports an accurate outcome rather than ConnectionClosedPrematurely.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- networking
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100