Skip to content

fix h/3 ResponseContent with large buffer #56892

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
wfurt merged 1 commit into from
Aug 5, 2021
Merged

fix h/3 ResponseContent with large buffer #56892

wfurt merged 1 commit into from
Aug 5, 2021

Conversation

@wfurt
Copy link
Member

@wfurt wfurt commented Aug 5, 2021

I was originally suspecting Quic to not hand out data.
It seems like that part is OK, while there are some deficiencies IMHO. Filed #56891 to track that.
(adding BigWrite_SmallRead_Success test to outline some processing)

The real root cause lives in HttpClient.
The ReadResponseContent would run until it would fill the user provided buffer.
In case of #56115 it sends 11 bytes but it provide 1024 buffer to read to.

I added code to break out when we reach the frame boundary (similar to SslStream)
I could add some special case for Http3 but it feels like we should just update existing tests to be version agnostic.
Deferred work tracked by #56890

fixes #56115

note that ReadResponseContent and ReadResponseContentAsync are almost identical.

@wfurt wfurt requested a review from a team August 5, 2021 08:14
@wfurt wfurt self-assigned this Aug 5, 2021
@ghost
Copy link

ghost commented Aug 5, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

I was originally suspecting Quic to not hand out data.
It seems like that part is OK, while there are some deficiencies IMHO. Filed #56891 to track that.
(adding BigWrite_SmallRead_Success test to outline some processing)

The real root cause lives in HttpClient.
The ReadResponseContent would run until it would fill the user provided buffer.
In case of #56115 it sends 11 bytes but it provide 1024 buffer to read to.

I added code to break out when we reach the frame boundary (similar to SslStream)
I could add some special case for Http3 but it feels like we should just update existing tests to be version agnostic.
Deferred work tracked by #56890

fixes #56115

note that ReadResponseContent and ReadResponseContentAsync are almost identical.

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Http
Milestone: -

ManickaP
ManickaP reviewed Aug 5, 2021
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This fix probably breaks reading of trailing headers.

My bad, I figured that out, it's fine.

ManickaP
ManickaP approved these changes Aug 5, 2021
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

LGTM

@wfurt wfurt merged commit f61d5c8 into dotnet:main Aug 5, 2021
@wfurt wfurt deleted the read_56115 branch August 5, 2021 18:23
@geoffkizer
Copy link
Contributor

geoffkizer commented Aug 6, 2021

The ReadResponseContent would run until it would fill the user provided buffer.

This behavior seems really bad in general. Most importantly, it will break duplex usage, right?

Even for non-duplex we should give whatever data we have back to the user sooner rather than later.

@karelz karelz added this to the 6.0.0 milestone Aug 17, 2021
@CarnaViire CarnaViire mentioned this pull request Aug 27, 2021
Merged
@karelz karelz mentioned this pull request Sep 1, 2021
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Sep 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ManickaP ManickaP ManickaP approved these changes

Assignees

@wfurt wfurt

Labels

area-System.Net.Http

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

[HTTP/3] Response stream ReadAsync hang with large buffer size

4 participants

@wfurt @geoffkizer @ManickaP @karelz