Skip to content

HTTP/3: Connection shutdown improvements #34968

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Aug 7, 2021
Merged

HTTP/3: Connection shutdown improvements #34968

merged 13 commits into from
Aug 7, 2021

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Aug 3, 2021 •
edited
Loading

Fixes #35026

This PR kind of just grew and now fixes a lot of things:

  • Fallback to multiplex features
  • Fix stream ID in GOAWAY. It doesn't signal rejection of stream IDs greater than this value, it signals rejection of stream IDs greater or equal to this value. Basically, we need to add 4 to the value sent.
  • Many graceful shutdown fixes
    • Send GOAWAY at correct times
    • Abort incoming requests with H3_REQUEST_REJECTED error code
    • Correctly listen to IConnectionLifetimeNotificationFeature
  • Fix race between pooling a stream and aborting its remaining reads
  • Log error codes for stream and connection aborts
  • Log GOAWAY stream ID
  • Lock connection shutdown and throw passed in abortReason from AcceptAsync.

@JamesNK JamesNK mentioned this pull request Aug 3, 2021
Closed
@JamesNK JamesNK marked this pull request as draft August 3, 2021 05:48
@JamesNK JamesNK force-pushed the jamesnk/goaway-tests branch from 002dd72 to ff947fb Compare August 4, 2021 10:10
@JamesNK JamesNK changed the title HTTP/3: Add GOAWAY tests HTTP/3: Connection shutdown improvements, fallback to multiplex features Aug 4, 2021
@JamesNK JamesNK force-pushed the jamesnk/goaway-tests branch from ff947fb to 14d4b2c Compare August 4, 2021 10:17
@JamesNK JamesNK marked this pull request as ready for review August 4, 2021 10:18
Tratcher
Tratcher approved these changes Aug 4, 2021
View reviewed changes
@Tratcher
Copy link
Member

Tratcher commented Aug 4, 2021

Looks like a flaky test:
https://dev.azure.com/dnceng/public/_build/results?buildId=1275543&view=ms.vss-test-web.build-test-results-tab&runId=37726684&resultId=107567&paneView=debug

@JamesNK JamesNK force-pushed the jamesnk/goaway-tests branch from d3c9f68 to edcd274 Compare August 5, 2021 11:55
@JamesNK JamesNK requested a review from Tratcher August 5, 2021 11:56
@JamesNK
Copy link
Member Author

JamesNK commented Aug 5, 2021

@Tratcher Update with a big refactor of graceful shutdown. Could you take another look?

@JamesNK JamesNK mentioned this pull request Aug 6, 2021
Closed
Tratcher
Tratcher reviewed Aug 6, 2021
View reviewed changes
src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs
Comment on lines +346 to +354
// TODO should this message say the connection is shutting down or closing rather than faulted?
// Faulted has a negative meaning and we can get here by the host stopping or ConnectionLifeTimeNotification.RequestClose.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

@JamesNK JamesNK force-pushed the jamesnk/goaway-tests branch from 5e3e6fe to 8595e2d Compare August 7, 2021 00:57
Tratcher
Tratcher approved these changes Aug 7, 2021
View reviewed changes
@JamesNK JamesNK changed the title HTTP/3: Connection shutdown improvements, fallback to multiplex features HTTP/3: Connection shutdown improvements Aug 7, 2021
@JamesNK JamesNK merged commit ec504ed into main Aug 7, 2021
@JamesNK JamesNK deleted the jamesnk/goaway-tests branch August 7, 2021 05:30
@ghost ghost added this to the 6.0-rc1 milestone Aug 7, 2021
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Tratcher Tratcher Tratcher approved these changes

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

Assignees
No one assigned
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Milestone
6.0-rc1
Development

Successfully merging this pull request may close these issues.

HTTP/3: Connection level features vs stream level features
3 participants
@JamesNK @Tratcher @amcasey