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 race condition when cancelling pending HTTP connection attempts #110744

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Dec 16, 2024

Fixes #110598

There's a race condition here where if the initiating request completes right away, before we're able to set the CTS in InjectNewHttp11ConnectionAsync, we'll never set the timeout.

public void CancelIfNecessary(HttpConnectionPool pool, bool requestCancelled)
{
int timeout = GlobalHttpSettings.SocketsHttpHandler.PendingConnectionTimeoutOnRequestCompletion;
if (ConnectionCancellationTokenSource is null ||
timeout == Timeout.Infinite ||
pool.Settings._connectTimeout != Timeout.InfiniteTimeSpan && timeout > (int)pool.Settings._connectTimeout.TotalMilliseconds) // Do not override shorter ConnectTimeout
{
return;
}
lock (this)
{
if (ConnectionCancellationTokenSource is null)
{
return;
}

await Task.CompletedTask.ConfigureAwait(ConfigureAwaitOptions.ForceYielding);
HttpConnectionWaiter<HttpConnection> waiter = queueItem.Waiter;
HttpConnection? connection = null;
Exception? connectionException = null;
CancellationTokenSource cts = GetConnectTimeoutCancellationTokenSource();
waiter.ConnectionCancellationTokenSource = cts;

It's easy to reproduce if you add a delay between the Yield and assigning the cts, while failing the request.
The test I added was 100% deterministic if I added the delay, or increased the Parallel count from 100 to 1000, but that takes too long to run for CI.

The HTTP/3 change is just adding a try/finally to signal the waiter, looks like we missed that in #101531.
Diff without whitespace changes

@MihaZupan MihaZupan added this to the 10.0.0 milestone Dec 16, 2024
@MihaZupan MihaZupan requested review from antonfirsov and a team December 16, 2024 15:41
@MihaZupan MihaZupan self-assigned this Dec 16, 2024
@MihaZupan
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MihaZupan
Copy link
Member Author

/backport to release/9.0-staging

Copy link
Contributor

Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/12364986878

response.Dispose();
BlocklistAuthority(connection.Authority);
continue;
http3ConnectionWaiter?.CancelIfNecessary(this, cancellationToken.IsCancellationRequested);
Copy link
Member Author

Choose a reason for hiding this comment

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

With HTTP 1 and 2, we do this here instead:

http11ConnectionWaiter?.CancelIfNecessary(this, cancellationToken.IsCancellationRequested);
http2ConnectionWaiter?.CancelIfNecessary(this, cancellationToken.IsCancellationRequested);

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 modulo nitpicking comments above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants