-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Unobserved Task Exception after closing ClientWebSocket #80116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Tagging subscribers to this area: @dotnet/ncl Issue DetailsDescriptionWe randomly get the following unobserved task exception in our logs:
I managed to narrow this down to the ClientWebSocket close handshake and can reproduce this (see Reproduction Steps below). It happens when the closing of the connection takes too long after the close handshake finished. I think the issue is here: runtime/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs Line 985 in ac2ffdf
When Reproduction Steps
Expected behaviorNo unobserved task exception. Actual behaviorUnobserved task exception:
Regression?No response Known WorkaroundsNo response Configuration.NET 6.0.11 on Windows 11 x64. Other informationNo response
|
Triage: |
@MihaZupan But the await would need to wrapped in another try/catch, right? Otherwise we would throw here which currently cant' happen. Would this change require a test case? If not, I can make a PR. I'm not sure If I could write a consistent test case for this (especially since I assume I can't use ASP .NET Core in runtime tests). |
I think the cleanest option would be to revert to how this was written before #56282, something like using var finalCts = new CancellationTokenSource(WaitForCloseTimeoutMs);
using (finalCts.Token.UnsafeRegister(static s => ((ManagedWebSocket)s!).Abort(), this))
{
try
{
await finalReadTask;
}
catch { }
} Re: tests, it's generally harder to write ones that verify that all task exceptions are observed. While having a regression test to make sure we don't reintroduce the same mistake in the future would be nice, we'd take a PR even without it. We don't use ASP.NET in our tests, but we have our own "loopback server". You can see some examples of how that might look like here: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs. |
finalReadTask is a ValueTask. Wouldn't your suggestion result in it possibly being consume twice? Or have I misunderstood what you're suggesting? |
I think a PR needs a test. If it's worth fixing this to ensure we're not raising the event, then it's worth having a test for it. |
I meant we'd replace the current logic ( |
Ah, replacing it would be fine from the perspective of waiting for it twice. However, there is no guarantee that Dispose'ing of the Stream will result in the task returned from ReadAsync completing. That's why it's written the way it is. |
We're relying on this being the case in HttpConnection though, no? |
Yes, but only because at the time it was written we had no other option, e.g. there was no cancellation story for NetworkStream / Socket. I believe there's an open issue to switch HttpConnection over to using CancellationTokens threaded through all the calls instead of this approach with disposing streams, which is not thread-safe and not guaranteed to work, especially with ConnectCallback able to supply an arbitrary Stream. |
triage: not critical for 9.0, moving to future |
We're seeing the same issue in our application and our Sentry error reporting is picking up these unobserved task exceptions. Took forever to determine that this is not a problem in our own code, but in the framework 😬 |
Same issue with Udp/TcpClient. After closing it, every pending receive operation started with BeginReceive will emit the aforementioned unobserved task exception. Just ran into this on maui ios where the unobserved task handler was attempting to break if a debugger was attached, which due to some other bug, just hangs 95%+ of the time - what fun that was tracking down. This bug should have been fixed by now...having the runtime generate these kinds of errors just wastes everyone's time and needlessly increases SNR. We rely on the UnobservedTaskException to point out where we are being idiotic, but if the runtime is being an idiot too, then we really can't reliably use it as a safety check. |
…otnet/runtime#80116), so let's not forcefully terminate JL just because an UnobservedTaskException is thrown
Same problem with simple UnobservedTaskException System.Net.Sockets.SocketException (995): The I/O operation has been aborted because of either a thread exit or an application request.
at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.ThrowException(SocketError error, CancellationToken cancellationToken)
at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.System.Threading.Tasks.Sources.IValueTaskSource<System.Int32>.GetResult(Int16 token)
at System.Threading.Tasks.ValueTask`1.ValueTaskSourceAsTask.<>c.<.cctor>b__4_0(Object state) Took some time to investigate the root cause :( ... And documentation clearly says that all must be ok:
|
Description
We randomly get the following unobserved task exception in our logs:
I managed to narrow this down to the ClientWebSocket close handshake and can reproduce this (see Reproduction Steps below). It happens when the closing of the connection takes too long after the close handshake finished. I think the issue is here:
runtime/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs
Line 985 in ac2ffdf
When
WaitAsync()
throws aTimeoutException
, it gets ignored, but the exception from the originalfinalReadTask
(which may happen later) is never observed in this case.Reproduction Steps
Expected behavior
No unobserved task exception.
Actual behavior
Unobserved task exception:
Regression?
No response
Known Workarounds
No response
Configuration
.NET 6.0.11 on Windows 11 x64.
Other information
No response
The text was updated successfully, but these errors were encountered: