Skip to content

HTTP/3: Support canceling requests that aren't reading a body #36017

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
JamesNK merged 3 commits into from
Sep 2, 2021
Merged

HTTP/3: Support canceling requests that aren't reading a body #36017

JamesNK merged 3 commits into from
Sep 2, 2021

Conversation

@JamesNK
Copy link
Member

@JamesNK JamesNK commented Sep 1, 2021

  • Fix HTTP/3 layer not being notified of client abort after it has finished reads. Subscribe to a cancellation token for the notification. Driven by new QuicStream.WaitForSendCompleteAsync API.
  • Fix unexpected errors in Http3OutputProducer.WriteDataFrames loop from QuicStream writes being aborted.
  • IProtocolErrorCodeFeature.Error defaults to -1 when unset.
  • Aborts throw IOException to app in Kestrel in more scenarios. This matches HTTP/2 behavior.
  • Different log messages for aborting a streams reads and aborting writes.

Build fails because QuicStream.WaitForSendCompleteAsync is not available yet.

@JamesNK JamesNK added blocked The work on this issue is blocked due to some dependency area-runtime labels Sep 1, 2021
@JamesNK
Copy link
Member Author

JamesNK commented Sep 1, 2021

@Tratcher @halter73 This PR has some changes to Http3OutputProducer and Http3FrameWriter that I'd like you to take a closer look at. When the underlying QUIC stream was aborted, unexpected errors were being logged in Http3OutputProducer.ProcessDataWrites. My updates change it so that ProcessDataWrites and TimingPipeFlusher could expect ConnectionResetException.

@JamesNK JamesNK force-pushed the jamesnk/http3-waitwritescomplete-main branch from 67fc2f8 to 66f5e26 Compare September 1, 2021 20:36
@JamesNK JamesNK removed the blocked The work on this issue is blocked due to some dependency label Sep 1, 2021
Tratcher
Tratcher approved these changes Sep 1, 2021
View reviewed changes
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

It is a bit circular but I think it makes sense.

halter73
halter73 reviewed Sep 2, 2021
View reviewed changes
@JamesNK JamesNK force-pushed the jamesnk/http3-waitwritescomplete-main branch from 2e4b1bb to 3b4cc27 Compare September 2, 2021 03:06
@dotnet dotnet deleted a comment from github-actions bot Sep 2, 2021
@dotnet dotnet deleted a comment from github-actions bot Sep 2, 2021
@JamesNK JamesNK mentioned this pull request Sep 2, 2021
Merged
@JamesNK JamesNK merged commit 5e1244e into main Sep 2, 2021
@JamesNK JamesNK deleted the jamesnk/http3-waitwritescomplete-main branch September 2, 2021 22:43
@ghost ghost added this to the 7.0-preview1 milestone Sep 2, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@halter73 halter73 halter73 approved these changes

@Tratcher Tratcher Tratcher approved these changes

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy 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

7.0-preview1

Development

Successfully merging this pull request may close these issues.

5 participants

@JamesNK @halter73 @Tratcher @amcasey