Prevent WebSockets from throwing during graceful shutdown #26914
Conversation
| readResult = await _context.Input.ReadAsync(cancellationToken); | ||
| } | ||
|
|
||
| return readResult; |
There was a problem hiding this comment.
@stephentoub Is this acceptable ValueTask usage?
I think we're okay because this always either awaits the ValueTask or calls GetAwaiter().GetResult(). And it only calls GetAwaiter().GetResult() if we've verified ValueTask.IsCompletedSuccessfully. I know we don't pool ValueTasks yet, but I don't want to introduce more bad code like what you highlighted in #16876.
There was a problem hiding this comment.
We should turn on the analyzer
There was a problem hiding this comment.
This looks fine. And the analyzer shouldn't warn about it; if it does, we should fix the analysis.
halter73
changed the title
| // can wake up and shutdown gracefully. We manually call CancelPendingRead() to simulate this and | ||
| // ensure the Stream returned by UpgradeAsync doesn't throw in this case. | ||
| // https://github.com/dotnet/aspnetcore/issues/26482 | ||
| var connectionTransportFeature = context.Features.Get<IConnectionTransportFeature>(); |
| // Ignore the canceled readResult unless it was canceled by the user. | ||
| do | ||
| { | ||
| if (!_context.Input.TryRead(out readResult)) |
There was a problem hiding this comment.
Could this end up in a tight loop until cancellation is consumed by the request processing loop?
There was a problem hiding this comment.
Yes, if the PipeReader repeatedly returns a canceled ReadResult. Same goes ReadAsync. It should still be safe, because PipeReaders are supposed to only return at most 1 canceled ReadResult per call to CancelPendingRead(). And this logic isn't entirely new, even in 3.1. Http1ContentLengthMessageBody already has this behavior.
One difference you'll notice though between this and Http1ContentLengthMessageBody is that it doesn't call AdvanceTo before reading again after a canceled ReadResult (Remember #25799). This isn't necessary for the DefaultPipeReader and simplifies the logic quite a bit, but I could easily seeing this breaking custom ones. That said, if it breaks because of this, it would wind up just throwing an InvalidOperationException instead of the OperationCanceledException it throws today.
I understand being nervous though. Given severity of the issue (which is basically throwing from WebSockets 5 seconds earlier than we should during shutdown), do you still think this is worth backporting to 3.1?
There was a problem hiding this comment.
Yes we should still back port it.
|
Should you make a PR to 5.0 as well? |
halter73
added
the
Servicing-consider
|
Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge. |
DescriptionThis prevents WebSockets from throwing during graceful shutdown. Kestrel calls Transport.Input.CancelPendingRead() during shutdown so idle connections can wake up and close gracefully. This PR ignores canceled ReadResults in Streams returned by UpgradeAsync, so they don't throw early during shutdown. Customer ImpactWithout this fix, WebSocket read operations throw an OperationCanceledException at the beginning graceful shutdown (which lasts 5 seconds by default). WebSocket read operations should only throw if the graceful shutdown period expires to give customers the opportunity to use the WebSocket during the graceful shutdown period. This was reported by a customer on GitHub. #26482 Regression?No. RiskLow. It might seem a bit scary because we're adding a loop with a non-trivial exit condition, but we already have loops exactly like this in 3.1 in the more commonly used Http1ContentLengthMessageBody. See #26914 (comment). |
Pilchie
added
Servicing-approved
|
This was approved for 3.1.10 over email. Tagging @wtgodbe for merge. |
|
@wtgodbe the merge bot is still running right? In that case, I think we should just merge it, and treat this as implicitly approved for 5.0. |
|
The merge bot was still running when I was on build ops a few days ago. @dougbu just mentioned in ASP.NET Build Team that the merge bot isn't adding much value, but I think it should work for this change. |
|
Yeah, the bot is still merging 3.1 -> 5.0 |
amcasey
added
the
area-networking
This stops WebSockets from throwing as soon as Kestrel shutdown starts instead of after the ShutdownTimeout. This copies logic we already have in other places like Http1ContentLengthMessageBody.
Kestrel will call Transport.Input.CancelPendingRead() during shutdown so idle connections can wake up and shutdown gracefully. We manually call CancelPendingRead() to simulate this and ensure the Stream returned by UpgradeAsync doesn't throw in this case.
Addresses #26482