-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
WinHttpHandler: Add support for bidirectional streaming #51094
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes #44784 TODO:
|
bd053f6
to
b7d4993
Compare
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.
Initial pass looks good.
src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/BidirectionStreamingTest.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/BidirectionStreamingTest.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/BidirectionStreamingTest.cs
Outdated
Show resolved
Hide resolved
ThrowOnInvalidHandle(requestHandle, nameof(Interop.WinHttp.WinHttpOpenRequest)); | ||
} | ||
|
||
// Platform doesn't support WINHTTP_FLAG_AUTOMATIC_CHUNKING. Revert to manual chunking. |
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.
How expensive is the failing call? Worth caching so we don't try again?
@karelz @geoffkizer @antonfirsov I'd like to get this reviewed with the aim to merge and include with .NET 6. Please take a look 🙏 An outstanding question that should be looked at is whether it is worth having the feature detection and fallback to current behavior. The current behavior is pretty weird and unexpected. Current behavior: WinHttpHandler + a HTTP/2 request + a request body + no length = a HTTP/1.1 request with chunking Is this worth preserving, or should the fallback be removed? If it is removed then making a request like this on an unsupported Windows version will error instead of doing HTTP/1.1. |
Personally, I'm not confident enough about my knowledge of the space to approve this without doing a hands-on try of the code on a test machine, but I'm more than happy to defer to @scalablecory, if he thinks the code is good to take in :)
Do we think there is a noticeable number of HTTP/2 adaptors out there, who may have (unintentionally?) taken dependency on the old, slow, unexpected behavior, being broken with such a change on current production OS-es? @scalablecory @geoffkizer thoughts? |
I don't think we should worry about this. If the user requests HTTP2 then they are willing to have us do HTTP2. |
I think it's reasonable to use actual HTTP/2, falling back to the current behavior if it's not supported by the OS. If WinHTTP backports the feature to all the OSes WinHttpHandler supports, then we can remove the fallback in the future. |
Triage: If WinHttp supports it, use HTTP/2. If not, use the old fallback -- which is current status of the PR. |
@JamesNK since we decided to keep the fallback behavior, I recommend to add a minimal smoke test for that. Otherwise the PR looks good to me. |
4ee0489
to
c074259
Compare
Co-authored-by: Cory Nelson <phrosty@gmail.com>
c074259
to
d1d65df
Compare
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs
Outdated
Show resolved
Hide resolved
…tion.cs Co-authored-by: Anton Firszov <antonfir@gmail.com>
Thanks! |
Fixes #44784
Fixes #46413
TODO: