WinHttpHandler: Read HTTP/2 trailing headers by JamesNK · Pull Request #46602 · dotnet/runtime

JamesNK · 2021-01-05T21:49:47Z

Fixes #44778

Read HTTP/2 trailing headers when using WinHttpHandler.

Notes:

Adds netstandard2.1 target.
Loads trailers into HttpResponseMessage.TrailingHeaders in netstandard2.1. With older targets the trailers are stashed in HttpRequestMessage.Properties.
Requires modern version of Windows. Should the OS version be checked to avoid using new option on older Windows versions?

ghost · 2021-01-05T21:49:49Z

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

Issue Details

Read HTTP/2 trailing headers when using WinHttpHandler.

Notes:

Adds netstandard2.1 target.
Loads trailers into HttpResponseMessage.TrailingHeaders in netstandard2.1. With older targets the trailers are stashed in HttpRequestMessage.Properties.
Requires modern version of Windows. Should the OS version be checked to avoid using new option on older Windows versions?

Author:	JamesNK
Assignees:	-
Labels:	`area-System.Net.Http`
Milestone:	-

JamesNK · 2021-01-05T21:51:17Z

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpResponseStream.cs

+            // Only load response trailers if:
+            // 1. HTTP/2 or later
+            // 2. Response trailers not already loaded
+            if (_readTrailingHeaders || _responseMessage.Version < WinHttpHandler.HttpVersion20)


OS version check here?

JamesNK · 2021-01-05T21:51:47Z

src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/TrailingHeadersTest.cs

+        protected static Frame MakeDataFrame(int streamId, byte[] data, bool endStream = false) =>
+            new DataFrame(data, (endStream ? FrameFlags.EndStream : FrameFlags.None), 0, streamId);
+
+        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.SupportsAlpn))]


OS version condition on these tests?

....WinHttpHandler/tests/FunctionalTests/System.Net.Http.WinHttpHandler.Functional.Tests.csproj

antonfirsov

I see that #44778 still has the api-needs-work label. Even if there are no new API additions technically, these are behavioral changes with API aspects, so we should some sort of consensus before proceeding. (Maybe this has already happened and I'm just missing something -- @karelz ?)

The proposed solution has several oddities which are very concerning IMO:

Behavior differs depending on OS version in a way that is not transparent to the users. (If my understanding is correct: we fill some collection when WINHTTP_QUERY_FLAG_TRAILERS is supported, otherwise we don't.)
Behavior differs depending on the target (what collection to fill)

These differences are far from being obvious. The question we should raise to ourselves is: how would we document / communicate such an API to the public? How would we respond to critiques claiming the way we implemented this is overcomplicated?

@geoffkizer any thoughts?

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpResponseParser.cs

antonfirsov · 2021-01-06T16:28:10Z

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpResponseParser.cs

+                else
                {
-                    throw WinHttpException.CreateExceptionUsingError(lastError, nameof(Interop.WinHttp.WinHttpQueryHeaders));
+                    if (!(lastError == Interop.WinHttp.ERROR_INSUFFICIENT_BUFFER || lastError == Interop.WinHttp.ERROR_WINHTTP_HEADER_NOT_FOUND))


Is ERROR_WINHTTP_HEADER_NOT_FOUND here to fail silently when WINHTTP_QUERY_FLAG_TRAILERS is unsupported?

No, it is to handle HTTP responses that don't have any trailers. A response will always have headers, but trailers are optional.

I think it is better to have an OS version check to see whether trailers are supported. That would determine whether the call is made to begin with.

How are Windows version checks does in this library? (or other runtime libraries)

In this library these checks are unprecedental, setting unsupported options usually leads to WinHttpException. I've seen checks on Environment.OSVersion in other BCL libs though .

What if WinHttp decides to backport the feature to earlier OS versions? I would feel more comfortable with established pattern of relying on WinHttpException. Is that possible?

With the current design, this feature is implicit (filling the collection whenever possible), so throwing is not an option.

However I think that checking the error code after the first WinHttpQueryHeaders shall be a sufficient (and actually better) solution.

One thing to consider here --- we should be very careful about having an API that, depending on OS version:

a) works as expected.
b) appears to work, but has an empty collection with zero notice to user.

This will be very fragile.

@scalablecory I tried to address this in my proposal in #44778 (comment). I suggest to discuss the API under the issue, comments and ideas are more than welcome there.

I discussed OS support detection for trailers with WinHttp team and this is their suggestion:

How about this for detection.

You try to query WINHTTP_OPTION_STREAM_ERROR_CODE from you session handle.

If it fails with ERROR_INVALID_PARAMETER, there is no support for trailers.

Otherwise, it should fail ERROR_WINHTTP_INCORRECT_HANDLE_TYPE, as the option cannot be queried from session handles.

They prefered doing that over hardcoding an OS version number because WinHttp trailer support could be backported to more Windows versions.

You try to query WINHTTP_OPTION_STREAM_ERROR_CODE from you session handle.

I guess these are distinct features which have been introduced in the same release right? What if they get misaligned because trailing headers get backported to an earlier version?

I presume so. This is the WinHttp team's suggestion. They recommended against checking OS version.

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpResponseParser.cs

karelz · 2021-01-07T13:18:39Z

Thanks @JamesNK for your PR!

Even if there are no new API additions technically, these are behavioral changes with API aspects, so we should some sort of consensus before proceeding. (Maybe this has already happened and I'm just missing something -- @karelz ?)

I agree we need to have consensus on the API shape first. Even though it is "just" a property bag, we need to make sure it is aligned with our strategy for all handlers (incl. naming convention of the properties) and IMO we should go through official API review first.
Key question IMO: Should we treat it as a temporary hack (only some builds of WinHttpHandler expose the property), or as permanent API which we should support on WinHttpHandler in all circumstances? (see #46602 (comment) above)

I think it would be best to have the API discussion on the issue #44778 rather than in the PR ... thoughts?

JamesNK · 2021-01-19T21:54:59Z

I tested this PR using the gRPC interop tests. The interop tests use WinHttpHandler + Grpc.Net.Client to send end-to-end gRPC calls.

I can confirm that all the interop tests that send unary (simple request/response) and server streaming gRPC calls succeed with these changes, which is what I expected. The tests succeed on both .NET 5 and .NET Framework.

....WinHttpHandler/tests/FunctionalTests/System.Net.Http.WinHttpHandler.Functional.Tests.csproj

antonfirsov

Not an extensive review yet, just a few remarks.

src/libraries/System.Net.Http.WinHttpHandler/ref/System.Net.Http.WinHttpHandler.cs

antonfirsov · 2021-01-27T14:33:36Z

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpResponseStream.cs


                    // Write that data out to the output stream
+#if NETSTANDARD2_1
+                    await destination.WriteAsync(buffer.AsMemory(0, bytesRead), cancellationToken).ConfigureAwait(false);


Are there any observable advantages from using this overload of Stream.WriteAsync explicitly? I would expect that the performance of (most) underlying implementations is identical. Or am I missing something?

There is an error when not calling it on netstandard2.1:

error CA1835: Change the 'WriteAsync' method call to use the 'Stream.WriteAsync(ReadOnlyMemory, CancellationToken)' overload

antonfirsov · 2021-01-27T14:43:22Z

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpResponseParser.cs

+                else
                {
-                    throw WinHttpException.CreateExceptionUsingError(lastError, nameof(Interop.WinHttp.WinHttpQueryHeaders));
+                    if (!(lastError == Interop.WinHttp.ERROR_INSUFFICIENT_BUFFER || lastError == Interop.WinHttp.ERROR_WINHTTP_HEADER_NOT_FOUND))


You try to query WINHTTP_OPTION_STREAM_ERROR_CODE from you session handle.

I guess these are distinct features which have been introduced in the same release right? What if they get misaligned because trailing headers get backported to an earlier version?

antonfirsov · 2021-02-19T17:08:29Z

/azp run runtime

azure-pipelines · 2021-02-19T17:08:54Z

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov · 2021-02-23T19:28:28Z

I'm closing this PR so it doesn't show up in stale PR reports. A replacement PR will follow in a day or two.

JamesNK added the area-System.Net.Http label Jan 5, 2021

JamesNK commented Jan 5, 2021

View reviewed changes

....WinHttpHandler/tests/FunctionalTests/System.Net.Http.WinHttpHandler.Functional.Tests.csproj Outdated Show resolved Hide resolved

antonfirsov reviewed Jan 6, 2021

View reviewed changes

antonfirsov mentioned this pull request Jan 7, 2021

WinHttpHandler HTTP/2 trailing headers #44778

Closed

danmoseley reviewed Jan 19, 2021

View reviewed changes

....WinHttpHandler/tests/FunctionalTests/System.Net.Http.WinHttpHandler.Functional.Tests.csproj Show resolved Hide resolved

antonfirsov reviewed Jan 27, 2021

View reviewed changes

WinHttpHandler: Read trailing headers

df5dcd1

JamesNK force-pushed the jamesnk/winhttp-trailingheaders branch from e56ee37 to df5dcd1 Compare January 28, 2021 07:44

runfoapp bot mentioned this pull request Jan 28, 2021

Cannot access a disposed object.\r\nObject name: 'System.Net.Sockets.Socket' from System.Net.Sockets.Tests.ReceiveFrom*.ClosedDuringOperation_Throws_ObjectDisposedExceptionOrSocketException #47561

Closed

karelz assigned antonfirsov Feb 10, 2021

JamesNK mentioned this pull request Feb 16, 2021

Target .NET Standard 2.0 grpc/grpc-dotnet#1118

Merged

antonfirsov added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 19, 2021

antonfirsov added NO-REVIEW Experimental/testing PR, do NOT review it and removed area-System.Net.Http labels Feb 19, 2021

antonfirsov closed this Feb 23, 2021

antonfirsov mentioned this pull request Feb 24, 2021

WinHttpHandler: Read HTTP/2 trailing headers (replacement PR) #48704

Merged

ghost locked as resolved and limited conversation to collaborators Mar 25, 2021

Conversation

JamesNK commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jan 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

antonfirsov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

antonfirsov Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

karelz commented Jan 7, 2021

Uh oh!

JamesNK commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

antonfirsov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov commented Feb 19, 2021

Uh oh!

azure-pipelines bot commented Feb 19, 2021

Uh oh!

antonfirsov commented Feb 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JamesNK commented Jan 5, 2021 •

edited

Loading

antonfirsov Jan 6, 2021 •

edited

Loading

JamesNK Jan 7, 2021 •

edited

Loading

JamesNK Jan 13, 2021 •

edited

Loading

JamesNK Jan 28, 2021 •

edited

Loading

JamesNK commented Jan 19, 2021 •

edited

Loading