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 HTTP/2 trailing headers #44778

Closed
JamesNK opened this issue Nov 17, 2020 · 5 comments · Fixed by #48704
Closed

WinHttpHandler HTTP/2 trailing headers #44778

JamesNK opened this issue Nov 17, 2020 · 5 comments · Fixed by #48704
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Net.Http Cost:S Work that requires one engineer up to 1 week Priority:2 Work that is important, but not critical for the release Team:Libraries
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented Nov 17, 2020

WinHTTP has added support for HTTP/2 trailing headers in modern versions of Windows.

WinHttpHandler should set trailers into the HttpResponseMessage.TrailingHeaders collection in .NET Core 3+.

An idea that the gRPC team is thinking about is using WinHttpHandler to support Grpc.Net.Client on .NET Framework. For that to work the trailing headers would need to be available in .NET Framework, which doesn't have the HttpResponseMessage.TrailingHeaders API.

A possible solution that doesn't involve adding new API to .NET Framework could be to set the trailing headers collection as a value in HttpRequestMessage.Properties using a known key, e.g. __Http2ResponseTrailingHeaders.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Nov 17, 2020
@ghost
Copy link

ghost commented Nov 17, 2020

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

Issue Details
Description:

WinHTTP has added support for HTTP/2 trailing headers in modern versions of Windows.

WinHttpHandler should set trailers into the HttpResponseMessage.TrailingHeaders collection in .NET Core 3+.

An idea I'm thinking about with gRPC support on .NET Framework is using WinHttpHandler for HTTP/2. For that to work the trailing headers would need to be available in .NET Framework, which doesn't have the HttpResponseMessage.TrailingHeaders API.

A possible solution that doesn't involve adding new API to .NET Framework could be to set the trailing headers collection as a value in HttpRequestMessage.Properties using a known key, e.g. __Http2TrailingHeaders.

Author: JamesNK
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@JamesNK
Copy link
Member Author

JamesNK commented Nov 17, 2020

Example of what hacking in trailing headers could look like:

        public static HttpResponseHeaders GetTrailingHeaders(this HttpResponseMessage responseMessage)
        {
#if !NET472
            return responseMessage.TrailingHeaders;
#else
            if (!responseMessage.RequestMessage.Properties.TryGetValue("__Http2ResponseTrailingHeaders", out var headers))
            {
                throw new InvalidOperationException();
            }
            return (HttpResponseHeaders)headers;
#endif
        }

@karelz karelz added the api-needs-work API needs work before it is approved, it is NOT ready for implementation label Dec 2, 2020
@karelz
Copy link
Member

karelz commented Dec 2, 2020

Triage: Making it compatible with .NET Core 3.0+ makes sense.
If it is worth exposing on .NET Framework is subject to discussion (which we will have tomorrow) and should be split off into separate issue.

@karelz karelz added Priority:2 Work that is important, but not critical for the release Team:Libraries and removed untriaged New issue has not been triaged by the area owner labels Jan 6, 2021
@karelz karelz added this to the 6.0.0 milestone Jan 6, 2021
@antonfirsov
Copy link
Member

antonfirsov commented Jan 7, 2021

Following up on #46602 (comment) I also think that we should continue API discussions here, under the issue, before going further with implementation details.

At the moment I'm a bit pessimistic, since I don't see a good way to expose this without going against established BCL practices. A list of challanges I see:

  • HttpRequestMessage.Properties is obsolete since .NET 5. Although there is no obsoletion warning when building against .NET Framework, exposing a new feature trough this bag doesn't look good
  • There aren't many precedents for exchanging information with the help of arbitrary property-bag keys. The only thing I was able to find is [browser][wasm] Configuring request options in Browser WebAssembly #39182 / Configuring request options in Browser WebAssembly #34168
  • In general, extensibility is not a strong point of HttpRequestMessage, especially when targeting multiple frameworks
  • API (header container) differs depending on the target, behaviors differs depending on the OS

The best thing I can think of to overcome these challanges is to introduce an extension method within the WinHttpHandler lib, so we can at least hide some of this mess:

[Edit] Changed the proposal so the method throws WinHttpException instead of returning false.

public static class WinHttpExtensions
{
    // Returns a header collection if WINHTTP_QUERY_FLAG_TRAILERS is supported, throws WinHttpException otherwise.
    // Hides the access of the property bag as an implementation detail.
    // We may decide to also set HttpResponseMessage.TrailingHeaders for consistency, but the recommended way to access them is to use the extension method.
    [SupportedOSPlatform("windows10.0.BUILD")]
    public static HttpResponseHeaders GetWinHttpTrailingHeaders(this HttpResponseMessage responseMessage);
}

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 7, 2021
@karelz karelz added the Cost:S Work that requires one engineer up to 1 week label Jan 12, 2021
@JamesNK
Copy link
Member Author

JamesNK commented Jan 14, 2021

I believe a GetWinHttpTrailingHeaders method in WinHttpHandler package will need to return HttpHeaders (base type of HttpResponseHeaders).

The constructor for HttpResponseHeaders isn't public so creating an adhoc one in WinHttpHandler package isn't possible.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 23, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 24, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 10, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Net.Http Cost:S Work that requires one engineer up to 1 week Priority:2 Work that is important, but not critical for the release Team:Libraries
Projects
None yet
4 participants