Skip to content

Prevent WebSockets from throwing during graceful shutdown #27123

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

Merged
merged 1 commit into from
Oct 23, 2020

Conversation

halter73
Copy link
Member

The same as #26914, but for 5.0.

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 for 5.0

- Fix canceled ReadResult handling in Http1UpgradeMessageBody
- Add UpgradeTests.DoesNotThrowGivenCanceledReadResult
@halter73 halter73 requested review from Tratcher and jkotalik October 22, 2020 21:48
@ghost ghost added the area-servers label Oct 22, 2020
@halter73 halter73 added this to the 5.0.0 milestone Oct 22, 2020
@halter73 halter73 added feature-kestrel Servicing-consider Shiproom approval is required for the issue labels Oct 22, 2020
@ghost
Copy link

ghost commented Oct 22, 2020

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.

@halter73
Copy link
Member Author

Description

This 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 Impact

Without 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.

Risk

Low. 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 (and 5.0 for that matter) in the more commonly used Http1ContentLengthMessageBody. See #26914 (comment).

@Pilchie Pilchie merged commit c15cd69 into release/5.0 Oct 23, 2020
@Pilchie Pilchie deleted the halter73/26482-5.0 branch October 23, 2020 18:06
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions feature-kestrel Servicing-consider Shiproom approval is required for the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants