-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
fix h/3 ResponseContent with large buffer #56892
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsI was originally suspecting Quic to not hand out data. The real root cause lives in HttpClient. I added code to break out when we reach the frame boundary (similar to SslStream) fixes #56115 note that
|
There was a problem hiding this comment.
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.
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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. |
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
andReadResponseContentAsync
are almost identical.