Add TrailingHeaders support for HTTP/2 #36128
Conversation
|
/azp run corefx-outerloop-osx |
|
/azp run corefx-outerloop-linux |
|
/azp run corefx-outerloop-windows |
|
Azure Pipelines successfully started running 1 pipeline(s). |
2 similar comments
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Azure Pipelines successfully started running 1 pipeline(s). |
src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs
Outdated
Show resolved
Hide resolved
src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.TrailingHeaders.cs
Outdated
Show resolved
Hide resolved
|
Is there much work remaining on this PR? It would be great to get it in so that I can begin testing HttpClient with some realistic gRPC workloads. |
| else | ||
| { | ||
| await SendResponseHeadersAsync(streamId, false, statusCode, headers); | ||
| await SendResponseHeadersAsync(streamId, false, statusCode, false, headers); |
There was a problem hiding this comment.
Nit: for both the existing and new Boolean args, can you please use named arguments? Otherwise, it's hard to know what they mean.
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System.Diagnostics; | ||
| using System; |
There was a problem hiding this comment.
This should not be necessary. Is there a reason you needed to add it?
| public void OnResponseHeadersStart() | ||
| { | ||
| lock (SyncObject) | ||
| { |
There was a problem hiding this comment.
What other states can this be in other than ExpectingData? Can we assert that it's only in one of those states?
| { | ||
| private static byte[] DataBytes = Encoding.ASCII.GetBytes("data"); | ||
| private static IList<HttpHeaderData> TrailingHeaders = new HttpHeaderData[] { | ||
| new HttpHeaderData("MyCoolTrailerHeader", "amazingtrailer"), new HttpHeaderData("Hello", "World") }; |
There was a problem hiding this comment.
I'm amused that my terrible choice in random name/values is being propagated into new tests :)
| public abstract class HttpClientHandlerTest_TrailingHeaders_Test : HttpClientHandlerTestBase | ||
| { | ||
| private static byte[] DataBytes = Encoding.ASCII.GetBytes("data"); | ||
| private static IList<HttpHeaderData> TrailingHeaders = new HttpHeaderData[] { |
There was a problem hiding this comment.
Nit: s_dataBytes, s_trailingHeaders
| // Server doesn't send trailing header frame. | ||
| HttpResponseMessage response = await sendTask; | ||
| Assert.Equal(HttpStatusCode.OK, response.StatusCode); | ||
| Assert.NotNull(response.TrailingHeaders); |
There was a problem hiding this comment.
The title of the test says it's expecting an empty collection; this should validate that.
| Assert.Equal(HttpStatusCode.OK, response.StatusCode); | ||
|
|
||
| // Pending read on the response content. | ||
| Console.WriteLine(response.TrailingHeaders); |
There was a problem hiding this comment.
Please remove these Console.WriteLines.
| await server.SendResponseHeadersAsync(streamId, endStream : true, isTrailingHeader:true, headers: TrailingHeaders); | ||
|
|
||
| // Read data until EOF is reached | ||
| while (stream.Read(data, 0, data.Length) != 0); |
There was a problem hiding this comment.
I think it'd be good to add another Assert.Empty check into the body of this loop, since we want to validate that the collection isn't populated until we stream.Read returns 0.
|
I'd like to test with this ASAP. When will it be published in an SDK I can download from https://github.com/dotnet/core-sdk#installers-and-binaries? |
|
If the build always runs every night then it's not a problem to wait until tomorrow. I just want to double check that it won't be days until I can use this 😄 |
|
I would expect <12h, but these things never stop surprising :) |
|
The builds run about 6-8 times per day (we batch commits), so we should have an SDK published by now |
* add trailing header tests for HTTP/2 * initial implementation * more fixes and tests * style cleanup * feedback from review * feedback from review Commit migrated from dotnet/corefx@57cb805
follow-up on #35337, fixes #34912
This adds processing for HeaderFrame after DataFrame. To track if some data was received, I split ExpectingData into two states.
The Http2GetAsync_TrailerHeaders_TrailingPseudoHeadersThrow() test give me some grief.
rfc7540 8.1.2.1. suggest that pseudo-headers in trailer should be treated as invalid response.
That means that one could get stream and we could Throw later on when reading stream is finished and we process trailing headers. This was easier for HTTP/1 where invalid headers should be ignored.
BTW besides tests, I also did some testing with nginx server sending trailers over HTTP/2.