Skip to content
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

Merged
merged 10 commits into from
May 18, 2021

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Apr 12, 2021

Fixes #44784
Fixes #46413

TODO:

  • OS detection
  • Verify that waiting for headers before fully writing request works on current OS

@ghost
Copy link

ghost commented Apr 12, 2021

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

Issue Details

Fixes #44784

TODO:

  • OS detection
  • Verify that waiting for headers before fully writing request works on current OS
Author: JamesNK
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@JamesNK JamesNK force-pushed the jamesnk/winhttp-bidi branch 2 times, most recently from bd053f6 to b7d4993 Compare April 27, 2021 08:53
Copy link
Contributor

@scalablecory scalablecory left a 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.

ThrowOnInvalidHandle(requestHandle, nameof(Interop.WinHttp.WinHttpOpenRequest));
}

// Platform doesn't support WINHTTP_FLAG_AUTOMATIC_CHUNKING. Revert to manual chunking.
Copy link
Contributor

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?

@JamesNK
Copy link
Member Author

JamesNK commented Apr 28, 2021

@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.

@antonfirsov
Copy link
Member

antonfirsov commented May 3, 2021

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 :)

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.

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?

@geoffkizer
Copy link
Contributor

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?

I don't think we should worry about this. If the user requests HTTP2 then they are willing to have us do HTTP2.

@scalablecory
Copy link
Contributor

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.

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.

@karelz
Copy link
Member

karelz commented May 4, 2021

Triage: If WinHttp supports it, use HTTP/2. If not, use the old fallback -- which is current status of the PR.

@antonfirsov
Copy link
Member

@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.

@antonfirsov
Copy link
Member

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

LGTM.

…tion.cs

Co-authored-by: Anton Firszov <antonfir@gmail.com>
@antonfirsov antonfirsov merged commit 5a0edac into dotnet:main May 18, 2021
@antonfirsov
Copy link
Member

Thanks!

@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants