Skip to content

HTTP/2 Continuation test - #40533

Merged
ManickaP merged 6 commits into
dotnet:masterfrom
ManickaP:mapichov/860_h2_continuation
Aug 11, 2020
Merged

ManickaP merged 6 commits into
dotnet:masterfrom
ManickaP:mapichov/860_h2_continuation

Conversation

@ManickaP

@ManickaP ManickaP commented Aug 7, 2020

Copy link
Copy Markdown
Member

The original issue with headers not being flushed when send in CONTINUATION frame was already fixed in #39166

This PR:

Closes #860
Fixes #1496

@ghost

ghost commented Aug 7, 2020

Copy link
Copy Markdown

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

@ManickaP
ManickaP requested review from a team and geoffkizer August 7, 2020 15:49
protected override Version UseVersion => HttpVersion.Version20;

[Fact]
public async Task SendAsync_LargeHeaders_Continuation_CorrectlyWritten()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this test should be put in HttpClientHandlerTest.Http2.cs. That's where most of the HTTP2 specific tests are.

I also think that this test should specifically check that the HEADERS and CONTINUATION frames are sent as we expect them to be (i.e. one HEADER frame of max size, then a single CONTINUATION with the rest of the header data). The HTTP2 tests in HttpClientHandlerTest.Http2.cs generally try to validate the specific frames being sent, instead of just using the generic loopback logic. That's intentional; these tests are HTTP2 specific and we want to validate the specific HTTP2 low-level behavior.

I also think we should consider adding another test that causes a second CONTINUATION to be sent, so that it's HEADERS, CONTINUATION w/o EndHeaders, CONTINUATION with EndHeaders.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moved and extended to test for 0, 1 and 2 CONTINUATION frames in a request, server side reads the frames and checks the number of continuations.

@ManickaP
ManickaP requested review from a team and geoffkizer August 11, 2020 13:06
Comment thread src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs Outdated

@geoffkizer geoffkizer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall LGTM; a couple minor things above.

@ManickaP
ManickaP merged commit 302875d into dotnet:master Aug 11, 2020
@ManickaP
ManickaP deleted the mapichov/860_h2_continuation branch August 11, 2020 17:33
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HTTP2 code coverage: add tests that cause us to send a CONTINUATION frame Http/2 headers not getting flushed if there are Continuation frames

4 participants