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

Fix Http2 MultiConnection test race conditions #91156

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

MihaZupan
Copy link
Member

Closes #91075

These tests have logic along the lines of

var connection1 = await SetupConnectionAsync(StreamLimit);
await StartStreamLimitRequestsAsync();

var connection2 = await SetupConnectionAsync(StreamLimit);
await StartStreamLimitRequestsAsync();

When a new Http2 connection is being added to the pool, it will first check whether there are any pending requests on the queue. It will signal those requests before storing itself in the list of available connections on the pool.

When the warmup request in SetupConnectionAsync was woken up, it would be fully processed quickly. We would then continue to queue up new client requests assuming we already have a connection in the pool to handle them.
However due to the race condition, the connection hasn't added itself to the pool yet, so these queued requests end up kicking off a new connection attempt.

You can then get into a situation where the next SetupConnectionAsync will receive a request, but it won't be the same request as the warmup task. Because the warmup task never got a response, the test would time out.

This change fixes this race by making sure to accept all pending requests on the server first, before initiating a new connection. As all previous requests will be accepted already, SetupConnectionAsync will see the first request on the connection being the warmup one correctly.


The issue happening with Http2_MultipleConnectionsEnabled_IdleConnectionTimeoutExpired_ConnectionRemovedAndNewCreated is a slight derivation of the above.
We would also start an extra connection attempt, but as the test involves waiting 20 seconds for the connection to hit the pool idle timeout, that connection attempt would end up hitting PendingConnectionTimeoutOnRequestCompletion.
When we went to accept a new connection after the fact, the loopback server would accept that already-cancelled connect attempt, and fail when trying to use that connection.

This change fixes that by blocking that third connection attempt from going through ConnectCallback to the socket layer until we're ready to accept it.

@MihaZupan MihaZupan added this to the 9.0.0 milestone Aug 26, 2023
@MihaZupan MihaZupan requested a review from a team August 26, 2023 13:04
@MihaZupan MihaZupan self-assigned this Aug 26, 2023
@ghost
Copy link

ghost commented Aug 26, 2023

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

Issue Details

Closes #91075

These tests have logic along the lines of

var connection1 = await SetupConnectionAsync(StreamLimit);
await StartStreamLimitRequestsAsync();

var connection2 = await SetupConnectionAsync(StreamLimit);
await StartStreamLimitRequestsAsync();

When a new Http2 connection is being added to the pool, it will first check whether there are any pending requests on the queue. It will signal those requests before storing itself in the list of available connections on the pool.

When the warmup request in SetupConnectionAsync was woken up, it would be fully processed quickly. We would then continue to queue up new client requests assuming we already have a connection in the pool to handle them.
However due to the race condition, the connection hasn't added itself to the pool yet, so these queued requests end up kicking off a new connection attempt.

You can then get into a situation where the next SetupConnectionAsync will receive a request, but it won't be the same request as the warmup task. Because the warmup task never got a response, the test would time out.

This change fixes this race by making sure to accept all pending requests on the server first, before initiating a new connection. As all previous requests will be accepted already, SetupConnectionAsync will see the first request on the connection being the warmup one correctly.


The issue happening with Http2_MultipleConnectionsEnabled_IdleConnectionTimeoutExpired_ConnectionRemovedAndNewCreated is a slight derivation of the above.
We would also start an extra connection attempt, but as the test involves waiting 20 seconds for the connection to hit the pool idle timeout, that connection attempt would end up hitting PendingConnectionTimeoutOnRequestCompletion.
When we went to accept a new connection after the fact, the loopback server would accept that already-cancelled connect attempt, and fail when trying to use that connection.

This change fixes that by blocking that third connection attempt from going through ConnectCallback to the socket layer until we're ready to accept it.

Author: MihaZupan
Assignees: MihaZupan
Labels:

area-System.Net.Http

Milestone: 9.0.0

@MihaZupan
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM

@MihaZupan MihaZupan merged commit 36cf0e4 into dotnet:main Aug 29, 2023
107 of 113 checks passed
@MihaZupan
Copy link
Member Author

/backport to release/8.0

@github-actions
Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6027293964

@ghost ghost locked as resolved and limited conversation to collaborators Sep 29, 2023
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_MultipleConnectionsEnabled test failures
2 participants