Avoid unobserved tasks in WebSocketsTransport #12315
Conversation
BrennanConroy
requested review from
analogrelay,
halter73 and
mikaelm12
as code owners
|
|
||
| // We re-throw here so we can communicate that there was an error when sending | ||
| // the close frame | ||
| throw; |
There was a problem hiding this comment.
This is the main culprit for unobserved tasks.
An alternative solution would be to keep this and actually observe the exception and set a private field to let the send loop know to send an internal server error instead of possibly a normal exit code.
https://github.com/aspnet/AspNetCore/blob/010ffe612150d9a3f5b78f2ad3ffcee3aedef369/src/SignalR/common/Http.Connections/src/Internal/Transports/WebSocketsTransport.cs#L272
|
This comment was made automatically. If there is a problem contact aspnetcore-build@microsoft.com. I've triaged the above build. I've created/commented on the following issue(s) |
|
This comment was made automatically. If there is a problem contact aspnetcore-build@microsoft.com. I've triaged the above build. I've created/commented on the following issue(s) |
BrennanConroy
force-pushed
the
brecon/unobserved
branch
from
c3019cf to
8e54457
Compare
| if (trigger == receiving) | ||
| { | ||
| // Observe exception if there is one to avoid unobserved tasks | ||
| _ = receiving.Exception; |
There was a problem hiding this comment.
Why don't these exceptions matter? Should we at least be logging them?
There was a problem hiding this comment.
We probably should, but to no higher than Debug since they will include client disconnects.
There was a problem hiding this comment.
These should never actually have exceptions in them, I was just being paranoid. We have a try catch around both sending and receiving so the only exceptions that could escape would be Pipe.Complete throwing, logger throwing, or socket.CloseOutputAsync (this would be the one most likely to throw).
I'll just wrap socket.CloseOutputAsync and log it if it throws.
There was a problem hiding this comment.
Can we get rid of all the _ = blah.Exception lines then?
The mere existence of these lines communicates to other developers that these tasks could have completed with an exception. I don't want to risk seeing this pattern copied more widely. The more places this pattern is used, the more likely it will be copied somewhere else.
There was a problem hiding this comment.
Yeah, I'm good with removing them
There was a problem hiding this comment.
Cool. If for whatever reason we are still concerned about something unexpected throwing, we could also replace the _ = blah.Exception with await trigger. That way if there was an unexpected error, WebSocketTransport.StopAsync() would rethrow it.
analogrelay
added
the
tell-mode
|
🆙 📅 |
|
This comment was made automatically. If there is a problem contact aspnetcore-build@microsoft.com. I've triaged the above build. I've created/commented on the following issue(s) |
ghost
deleted the
Fixing an instance of unobserved tasks that was causing clients in UWP apps to crash.
Not sure if this 100% fixes #12193 as one of the stack traces showed
HubConnection.ReceiveLoopcausing a crash (which looks impossible IMO).