Skip to content

IHttpRequestBodyDetectionFeature for HTTP/3 GET requests #62275

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ladeak
Copy link
Contributor

@ladeak ladeak commented Jun 7, 2025

IHttpRequestBodyDetectionFeature for HTTP/3 GET requests

IHttpRequestBodyDetectionFeature returns false for HTTP/3 (bodyless) GET requests

Description

  • Implementation follows the HTTP/2 structure.
  • Updated IHttpRequestBodyDetectionFeature to include HTTP/3 descriptions for the END_STREAM flag.
  • In Http3Stream modified request body handling logic to return zero length content body instance when there endstream flag is set. It also completes the message body for empty requests, so that the RequestBodyPipe.Reader is closed (needs to be closed, so that when the stream is reused by the pool, the Pipe is reset which requires closed reader) - not sure if this is the best place to do this.
  • Added unit test CanHaveBody_ReturnsFalseWithoutRequestBody to validate behavior when no request body is present.

Fixes #58753

- Updated `IHttpRequestBodyDetectionFeature` to include HTTP/3 descriptions for the `END_STREAM` flag.
- Modified request body handling logic to return zero length content body instance when there endstream flag is set.
  It also completes the message body for empty requests, so that the RequestBodyPipe.Reader is closed (needed when
  stream is reused by the pool and the Pipe is reset).
- Added unit test `CanHaveBody_ReturnsFalseWithoutRequestBody`
to validate behavior when no request body is present.
@github-actions github-actions bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Jun 7, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 7, 2025
@BrennanConroy
Copy link
Member

It also completes the message body for empty requests, so that the RequestBodyPipe.Reader is closed (needs to be closed, so that when the stream is reused by the pool, the Pipe is reset which requires closed reader) - not sure if this is the best place to do this.

Http2Stream doesn't do this. It relies on the calling code the cleanup the MessageBody. Is there a reason this is different from the Http2Stream impl?

@ladeak
Copy link
Contributor Author

ladeak commented Jun 10, 2025

@BrennanConroy - I will follow up on that.

@ladeak ladeak marked this pull request as draft June 10, 2025 06:25
…der was not closed in http3) causing Reset to throw when stream was re-used.
@ladeak
Copy link
Contributor Author

ladeak commented Jun 14, 2025

@BrennanConroy I addressed your concern.

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 community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IHttpRequestBodyDetectionFeature returns true for GET requests with HTTP/3
2 participants