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

make Http2_RequestFailsWithAppropriateHttpProtocolException test more deterministic #94112

Merged
merged 2 commits into from
Oct 31, 2023

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Oct 27, 2023

When I look at the normal passing run I see something like

passingTest

when it fails, the exchange looks like
failTest

So it seems like if the GOAWAY is sent before the client even start making request the test fails with Couldn't find HttpProtocolException with matching error code in exception: System.Net.Http.HttpRequestException: An HTTP/2 connection could not be established because the server did not complete the HTTP/2 handshake.

This may (or may not) be caused by the connection pool refactor. Since the requests are now detached, the connection can be closed by the 'GOAWAY` and the request does not fail with protocol error.

To minimize risk, I only changed the test to wait for beginning of the request to arrive so when HttpClient process the response it would produce deterministic result. I was running tests in loop with this change and I did not see failure. It was failing for me before frequently with less than 20-50 iterations.

fixes #92647

@wfurt wfurt added area-System.Net.Http test-enhancement Improvements of test source code labels Oct 27, 2023
@wfurt wfurt added this to the 9.0.0 milestone Oct 27, 2023
@wfurt wfurt requested a review from a team October 27, 2023 20:24
@wfurt wfurt self-assigned this Oct 27, 2023
@ghost
Copy link

ghost commented Oct 27, 2023

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

Issue Details

When I look at the normal passing run I see something like

passingTest

when it fails, the exchange looks like
failTest

So it seems like if the GOAWAY is sent before the client even start making request the test fails with Couldn't find HttpProtocolException with matching error code in exception: System.Net.Http.HttpRequestException: An HTTP/2 connection could not be established because the server did not complete the HTTP/2 handshake.

This may (or may not) be caused by the connection pool refactor. Since the requests are now detached, the connection can be closed by the 'GOAWAY` and the request does not fail with protocol error.

To minimize risk, I only changed the test to wait for beginning of the request to arrive so when HttpClient process the response it would produce deterministic result. I was running tests in loop with this change and I did not see failure. It was failing for me before frequently with less than 20-50 iterations.

fixes #92647

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Http, test-enhancement

Milestone: 9.0.0

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

LGTM

@wfurt
Copy link
Member Author

wfurt commented Oct 31, 2023

enterprised-linux failures are unrelated. looks like infrastructure problem.

@wfurt wfurt merged commit 748dd8e into dotnet:main Oct 31, 2023
107 of 109 checks passed
@wfurt wfurt deleted the h2 branch October 31, 2023 19:09
@ghost ghost locked as resolved and limited conversation to collaborators Dec 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http test-enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Test Failure] Http2_IncorrectServerPreface_RequestFailsWithAppropriateHttpProtocolException
2 participants