WinHttpHandler: Read HTTP/2 trailing headers (replacement PR) by antonfirsov · Pull Request #48704 · dotnet/runtime

antonfirsov · 2021-02-24T16:22:01Z

Fixes #44778, finalizing the work started in #46602.

Notes from the original PR:

Adds netstandard2.1 target.
Loads trailers into HttpResponseMessage.TrailingHeaders in netstandard2.1. With older targets the trailers are stashed in HttpRequestMessage.Properties.

Additions to the original PR:

Tests have been made conditional to OS Build >=19622
Feature detection logic added based on WinHTTP team's recommendation (check for WINHTTP_OPTION_STREAM_ERROR_CODE)
Fix WinHttpHandler.Unit.Tests build
Some refactor work to keep the code maintainable

Notes on testing

Currently, none of the CI machines pass the OS Version condition of the Trailers tests, therefore the only proof that the feature actually works is that the new tests passed locally on VM running a preview Windows build.
It's seems impossible to design automatic tests that work with .NET Framework. Framework doesn't support ALPN, rendering LoopbackServer useless. I validated the feature with a local-only test case, testing a feature with a remote endpoint running on localhost. It's not possible to host this service as an Azure App, because Azure can not bypass ISS, and Kestrel does not support Trailers with IIS. This is an unfortunate situation IMO, since we added this feature primarily for .NET Framework gRPC support, and looks like we will never be able to regression test it.

/cc @JamesNK

…' into winhttp/trailing-headers

…aders_Available

ghost · 2021-02-24T16:22:07Z

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

Issue Details

Fixes #44778, finalizing the work started in #46602.

Notes from the original PR:

Adds netstandard2.1 target.
Loads trailers into HttpResponseMessage.TrailingHeaders in netstandard2.1. With older targets the trailers are stashed in HttpRequestMessage.Properties.

My additions to the original PR:

Tests have been made conditional to OS Build >=19622
Feature detection logic added based on WinHTTP team's recommendation (check for WINHTTP_OPTION_STREAM_ERROR_CODE)
Fix WinHttpHandler.Unit.Tests build
Some refactor work to keep the code maintainable

Notes on testing

Currently, none of the CI machines pass the OS Version condition of the Trailer's tests, therefore the only proof that the feature actually works is that the new tests passed locally on VM running a preview Windows build.
It's seems impossible to design automatic tests that work with .NET Framework. Framework doesn't support ALPN, rendering LoopbackServer useless. I validated the feature with a local-only test case, testing a feature with a remote endpoint running on localhost. It's not possible to host this service as an Azure App, because it can't bypass ISS, and Kestrel does not support Trailers with IIS. This is an unfortunate situation IMO, since we added this feature primarily for .NET Framework gRPC support, and looks like we will never be able to regression test it.

/cc @JamesNK

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

antonfirsov · 2021-02-24T16:54:00Z

@JamesNK do you plan to do some sort of end-to-end validation of dotnet/core#5713 or have you already done it with your prototype?

JamesNK · 2021-02-24T19:55:03Z

I tested end-to-end with my earlier PR.

Once this PR is merged I'll reference WinHttpHandler preview package off the .NET 6 feed and re-run the test.

There is a copy of it here if you want to try it yourself. Replace the WinHttpHandler.dll file in the console's Lib directory.

JamesNK · 2021-02-24T20:07:05Z

It's seems impossible to design automatic tests that work with .NET Framework.

Is that is a common problem with all WinHttpHandler HTTP/2 tests on .NET Framework?

Kestrel does not support Trailers with IIS

It does with .NET 5 + Windows 10 Build 20241 or later.

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

stephentoub · 2021-02-24T20:10:47Z

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

+                if (!isTrailingHeaders)
+                {
+                    Debug.Assert(lastError != Interop.WinHttp.ERROR_WINHTTP_HEADER_NOT_FOUND);

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


Seems like this would be simpler as:

if (lastError != Interop.WinHttp.ERROR_INSUFFICIENT_BUFFER && (!isTrailingHeaders || lastError != Interop.WinHttp.ERROR_WINHTTP_HEADER_NOT_FOUND)) { throw WinHttpException.CreateExceptionUsingError(lastError, nameof(Interop.WinHttp.WinHttpQueryHeaders)); }

or something like that?

It also has to be preceded by:

Debug.Assert(isTrailingHeaders || lastError != Interop.WinHttp.ERROR_WINHTTP_HEADER_NOT_FOUND);

Can't decide if the code more readable in this form, pushed a change anyways.

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

Co-authored-by: James Newton-King <james@newtonking.com>

…rsov/runtime into winhttp/trailing-headers

antonfirsov · 2021-02-25T16:07:22Z

Is that is a common problem with all WinHttpHandler HTTP/2 tests on .NET Framework?

@JamesNK yes, I think it's a big gap, the following is a very common pattern to disable tests here:

runtime/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs

Lines 151 to 156 in 59417ef

    
           public async Task GetAsync_IPv6LinkLocalAddressUri_Success() 
        
           { 
        
               if (IsWinHttpHandler && UseVersion >= HttpVersion20.Value) 
        
               { 
        
                   return; 
        
               }

The only way to overcome this is to use our shared remote endpoints (stuff under https://corefx-net-http2.azurewebsites.net/) instead of LoopbackServer.

Kestrel does not support Trailers with IIS
It does with .NET 5 + Windows 10 Build 20241 or later.

Don't have much experience with Azure App Services, creating a website with a preview Windows build doesn't look like a trivial task to me. So I think we are stuck with local testing for now.

antonfirsov · 2021-02-25T16:22:46Z

There is an issue we need to look at before proceeding. Might be a local config issue or a bug in the Windows preview build I'm using (21320), but I'm concerned if it's something more serious.

On my preview test machine, running TrailingHeadersTests separately works, but running the whole tests suite together makes them fail with an exception coming from SslStream, also resulting in a bunch of other SSL failures which may be related:
https://gistpreview.github.io/?8c44cb205e96e698a0999d7cd7666a27

This is also happening with the original #46602 branch.

@JamesNK have you seen this on your preview test machine? @wfurt do those error messages ring any bells?

JamesNK · 2021-02-25T18:42:34Z

I don’t know.

It took me a while to setup the runtime solution and tests, I focused on just getting the new trailers tests working.

…iling-headers

JamesNK

Outstanding test failures look unrelated. Confirm they're unrelated or fix.

HTTP/2 trailers changes LGTM.

alnikola

LGTM

JamesNK · 2021-03-09T21:52:40Z

FYI

Grpc.Net.Client 2.36.0-pre1 is on NuGet. It adds .NET Standard 2.0.
I've written documentation: Split gRPC supported platforms doc into hosting services and calling services sections AspNetCore.Docs#21689. Once this PR is merged then I'll merge the docs and let internal teams know it is ready to try out.

antonfirsov · 2021-03-10T10:53:30Z

Outerloop test failures are unrelated: #40798 #44015 (comment) #45774 #40798 #1479 #43931

I'm merging this finally.

antonfirsov · 2021-03-10T11:10:03Z

FYI opened tracking issue to document the feature:
dotnet/dotnet-api-docs#5404

JamesNK and others added 17 commits January 28, 2021 20:43

WinHttpHandler: Read trailing headers

df5dcd1

Merge remote-tracking branch 'JamesNK/jamesnk/winhttp-trailingheaders…

d943c1e

…' into winhttp/trailing-headers

Merge branch 'master' into winhttp/trailing-headers

3e5f1cf

Merge branch 'master' into winhttp/trailing-headers

08d2f03

add "remote server" test

051d910

PlatformDetection.SupportsAlpn = false on .NET Framework

6f4449b

make tests conditional

ba83606

WinHttpTrailersHelper

4ea3699

add OS support check

46df0f1

refactor step 1

cc4f74d

refactor step 2

8285ee6

refactor step 3

c5375e7

Add comments

6594f31

simpify GetResponseHeaderCharBufferLength

1370e6d

fix build

3cceba8

Remove Http2GetAsyncResponseHeadersReadOption_RemoteServer_TrailingHe…

e41eacb

…aders_Available

fix WinHttpHandler.Unit.Tests

d48ae0a

ghost added the area-System.Net.Http label Feb 24, 2021

antonfirsov requested a review from a team February 24, 2021 16:23

JamesNK reviewed Feb 24, 2021

View reviewed changes

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpTrailersHelper.cs Show resolved Hide resolved

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpTrailersHelper.cs Outdated Show resolved Hide resolved

stephentoub reviewed Feb 24, 2021

View reviewed changes

antonfirsov and others added 5 commits February 25, 2021 14:59

fix unit tests

6635caf

add comments on RequestMessagePropertyName

575cbd8

Co-authored-by: James Newton-King <james@newtonking.com>

Merge branch 'winhttp/trailing-headers' of https://github.com/antonfi…

fdf4872

…rsov/runtime into winhttp/trailing-headers

simplify GetTrailersSupported

098356a

improve comments

01109e8

make code shorter in GetResponseHeaderCharBufferLength

cf13ba0

HttpClientHandlerTestBase.AllowAllCertificates should not be static

29969b4

antonfirsov mentioned this pull request Feb 26, 2021

WinHttpHandler: HttpClientHandlerTestBase.AllowAllCertificates should not be static #48817

Merged

revert previous attempt

0332b0e

Base automatically changed from master to main March 1, 2021 09:08

antonfirsov added 3 commits March 1, 2021 12:25

AllowAllCertificates: use parameter instead of static property

05b85ab

Merge branch 'winhttp/fix-HttpClientHandlerTestBase' into winhttp/tra…

a3e2350

…iling-headers

Merge branch 'main' into winhttp/trailing-headers

4604e47

This comment has been minimized.

Sign in to view

JamesNK approved these changes Mar 5, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

alnikola approved these changes Mar 9, 2021

View reviewed changes

JamesNK mentioned this pull request Mar 9, 2021

Split gRPC supported platforms doc into hosting services and calling services sections dotnet/AspNetCore.Docs#21689

Merged

antonfirsov mentioned this pull request Mar 10, 2021

Test failure: System.Net.Sockets.Tests.TelemetryTest.EventSource_SocketConnectsRemote_LogsConnectStartStop(connectMethod: \"Task\", useDnsEndPoint: True) #44015

Closed

antonfirsov merged commit 423f5e1 into dotnet:main Mar 10, 2021

antonfirsov added this to the 6.0.0 milestone Mar 10, 2021

antonfirsov mentioned this pull request Mar 10, 2021

Document WinHttpHandler trailing header feature dotnet/dotnet-api-docs#5404

Closed

ghost locked as resolved and limited conversation to collaborators Apr 9, 2021

Conversation

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

Uh oh!

Notes on testing

Uh oh!

ghost commented Feb 24, 2021

Notes on testing

Uh oh!

antonfirsov commented Feb 24, 2021

Uh oh!

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

Uh oh!

Uh oh!

JamesNK commented Feb 24, 2021

Uh oh!

Uh oh!

Uh oh!

stephentoub Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

antonfirsov Feb 25, 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!

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

Uh oh!

Uh oh!

antonfirsov commented Feb 25, 2021

Uh oh!

JamesNK commented Feb 25, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

JamesNK left a comment • 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!

This comment has been minimized.

This comment has been minimized.

alnikola left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

antonfirsov commented Mar 10, 2021

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

antonfirsov commented Feb 24, 2021 •

edited

Loading

JamesNK commented Feb 24, 2021 •

edited

Loading

antonfirsov Feb 25, 2021 •

edited

Loading

antonfirsov commented Feb 25, 2021 •

edited

Loading

JamesNK left a comment •

edited

Loading

JamesNK commented Mar 9, 2021 •

edited

Loading

antonfirsov commented Mar 10, 2021 •

edited

Loading