From 7d02bdced940b098f5b671baae4d6cc1ffafcab5 Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Mon, 16 Dec 2024 16:36:30 +0100 Subject: [PATCH 1/2] Fix race condition when cancelling pending HTTP connection attempts --- .../HttpConnectionPool.Http1.cs | 3 +- .../HttpConnectionPool.Http2.cs | 3 +- .../HttpConnectionPool.Http3.cs | 75 ++++++++++--------- .../ConnectionPool/HttpConnectionPool.cs | 22 +++++- .../SocketsHttpHandlerTest.Cancellation.cs | 62 +++++++++++++++ 5 files changed, 126 insertions(+), 39 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http1.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http1.cs index d65e1b6486de21..af78d4d4cb9a9c 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http1.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http1.cs @@ -259,8 +259,7 @@ private async Task InjectNewHttp11ConnectionAsync(RequestQueue.Q HttpConnection? connection = null; Exception? connectionException = null; - CancellationTokenSource cts = GetConnectTimeoutCancellationTokenSource(); - waiter.ConnectionCancellationTokenSource = cts; + CancellationTokenSource cts = GetConnectTimeoutCancellationTokenSource(waiter); try { connection = await CreateHttp11ConnectionAsync(queueItem.Request, true, cts.Token).ConfigureAwait(false); diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http2.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http2.cs index c3999c520f0f24..33155f12d8b73b 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http2.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http2.cs @@ -181,8 +181,7 @@ private async Task InjectNewHttp2ConnectionAsync(RequestQueue. Exception? connectionException = null; HttpConnectionWaiter waiter = queueItem.Waiter; - CancellationTokenSource cts = GetConnectTimeoutCancellationTokenSource(); - waiter.ConnectionCancellationTokenSource = cts; + CancellationTokenSource cts = GetConnectTimeoutCancellationTokenSource(waiter); try { (Stream stream, TransportContext? transportContext, Activity? activity, IPEndPoint? remoteEndPoint) = await ConnectAsync(queueItem.Request, true, cts.Token).ConfigureAwait(false); diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http3.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http3.cs index bae0ef38952555..b9e2bc2a98edf1 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http3.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http3.cs @@ -75,52 +75,60 @@ internal sealed partial class HttpConnectionPool // Loop in case we get a 421 and need to send the request to a different authority. while (true) { - if (!TryGetHttp3Authority(request, out HttpAuthority? authority, out Exception? reasonException)) + HttpConnectionWaiter? http3ConnectionWaiter = null; + try { - if (reasonException is null) + if (!TryGetHttp3Authority(request, out HttpAuthority? authority, out Exception? reasonException)) { - return null; + if (reasonException is null) + { + return null; + } + ThrowGetVersionException(request, 3, reasonException); } - ThrowGetVersionException(request, 3, reasonException); - } - long queueStartingTimestamp = HttpTelemetry.Log.IsEnabled() || Settings._metrics!.RequestsQueueDuration.Enabled ? Stopwatch.GetTimestamp() : 0; - Activity? waitForConnectionActivity = ConnectionSetupDistributedTracing.StartWaitForConnectionActivity(authority); + long queueStartingTimestamp = HttpTelemetry.Log.IsEnabled() || Settings._metrics!.RequestsQueueDuration.Enabled ? Stopwatch.GetTimestamp() : 0; + Activity? waitForConnectionActivity = ConnectionSetupDistributedTracing.StartWaitForConnectionActivity(authority); - if (!TryGetPooledHttp3Connection(request, out Http3Connection? connection, out HttpConnectionWaiter? http3ConnectionWaiter)) - { - try + if (!TryGetPooledHttp3Connection(request, out Http3Connection? connection, out http3ConnectionWaiter)) { - connection = await http3ConnectionWaiter.WaitWithCancellationAsync(cancellationToken).ConfigureAwait(false); + try + { + connection = await http3ConnectionWaiter.WaitWithCancellationAsync(cancellationToken).ConfigureAwait(false); + } + catch (Exception ex) + { + ConnectionSetupDistributedTracing.ReportError(waitForConnectionActivity, ex); + waitForConnectionActivity?.Stop(); + throw; + } } - catch (Exception ex) + + // Request cannot be sent over H/3 connection, try downgrade or report failure. + // Note that if there's an H/3 suitable origin authority but is unavailable or blocked via Alt-Svc, exception is thrown instead. + if (connection is null) { - ConnectionSetupDistributedTracing.ReportError(waitForConnectionActivity, ex); - waitForConnectionActivity?.Stop(); - throw; + return null; } - } - // Request cannot be sent over H/3 connection, try downgrade or report failure. - // Note that if there's an H/3 suitable origin authority but is unavailable or blocked via Alt-Svc, exception is thrown instead. - if (connection is null) - { - return null; - } + HttpResponseMessage response = await connection.SendAsync(request, queueStartingTimestamp, waitForConnectionActivity, cancellationToken).ConfigureAwait(false); - HttpResponseMessage response = await connection.SendAsync(request, queueStartingTimestamp, waitForConnectionActivity, cancellationToken).ConfigureAwait(false); + // If an Alt-Svc authority returns 421, it means it can't actually handle the request. + // An authority is supposed to be able to handle ALL requests to the origin, so this is a server bug. + // In this case, we blocklist the authority and retry the request at the origin. + if (response.StatusCode == HttpStatusCode.MisdirectedRequest && connection.Authority != _originAuthority) + { + response.Dispose(); + BlocklistAuthority(connection.Authority); + continue; + } - // If an Alt-Svc authority returns 421, it means it can't actually handle the request. - // An authority is supposed to be able to handle ALL requests to the origin, so this is a server bug. - // In this case, we blocklist the authority and retry the request at the origin. - if (response.StatusCode == HttpStatusCode.MisdirectedRequest && connection.Authority != _originAuthority) + return response; + } + finally { - response.Dispose(); - BlocklistAuthority(connection.Authority); - continue; + http3ConnectionWaiter?.CancelIfNecessary(this, cancellationToken.IsCancellationRequested); } - - return response; } } @@ -253,8 +261,7 @@ private async Task InjectNewHttp3ConnectionAsync(RequestQueue. HttpAuthority? authority = null; HttpConnectionWaiter waiter = queueItem.Waiter; - CancellationTokenSource cts = GetConnectTimeoutCancellationTokenSource(); - waiter.ConnectionCancellationTokenSource = cts; + CancellationTokenSource cts = GetConnectTimeoutCancellationTokenSource(waiter); Activity? connectionSetupActivity = null; try { diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.cs index 3c91fc4ccde43f..e72c048b717b2f 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.cs @@ -827,7 +827,27 @@ private async ValueTask EstablishSocksTunnel(HttpRequestMessage request, return stream; } - private CancellationTokenSource GetConnectTimeoutCancellationTokenSource() => new CancellationTokenSource(Settings._connectTimeout); + private CancellationTokenSource GetConnectTimeoutCancellationTokenSource(HttpConnectionWaiter waiter) + where T : HttpConnectionBase? + { + var cts = new CancellationTokenSource(Settings._connectTimeout); + + lock (waiter) + { + waiter.ConnectionCancellationTokenSource = cts; + + // The initiating request for this connection attempt may complete concurrently at any time. + // If it completed before we've set the CTS, CancelIfNecessary would no-op. + // Check it again now that we're holding the lock and ensure we always set a timeout. + if (waiter.Task.IsCompleted) + { + waiter.CancelIfNecessary(this, requestCancelled: waiter.Task.IsCanceled); + waiter.ConnectionCancellationTokenSource = null; + } + } + + return cts; + } private static Exception CreateConnectTimeoutException(OperationCanceledException oce) { diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs index ae1669c18e68b8..9ae4d15f64cd73 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs @@ -393,6 +393,68 @@ await RemoteExecutor.Invoke(static async (versionString, timoutStr) => }, UseVersion.ToString(), timeout.ToString()).DisposeAsync(); } + [OuterLoop("We wait for PendingConnectionTimeout which defaults to 5 seconds.")] + [Fact] + public async Task PendingConnectionTimeout_SignalsAllConnectionAttempts() + { + if (UseVersion == HttpVersion.Version30) + { + // HTTP3 does not support ConnectCallback + return; + } + + int pendingConnectionAttempts = 0; + bool connectionAttemptTimedOut = false; + + using var handler = new SocketsHttpHandler + { + ConnectCallback = async (context, cancellation) => + { + Interlocked.Increment(ref pendingConnectionAttempts); + try + { + await Assert.ThrowsAsync(() => Task.Delay(-1, cancellation)).WaitAsync(TestHelper.PassingTestTimeout); + cancellation.ThrowIfCancellationRequested(); + throw new UnreachableException(); + } + catch (TimeoutException) + { + connectionAttemptTimedOut = true; + throw; + } + finally + { + Interlocked.Decrement(ref pendingConnectionAttempts); + } + } + }; + + using HttpClient client = CreateHttpClient(handler); + client.Timeout = TimeSpan.FromSeconds(2); + + // Many of these requests should trigger new connection attempts, and all of those should eventually be cleaned up. + await Parallel.ForAsync(0, 100, async (_, _) => + { + await Assert.ThrowsAnyAsync(() => client.GetAsync("https://dummy")); + }); + + Stopwatch stopwatch = Stopwatch.StartNew(); + + while (Volatile.Read(ref pendingConnectionAttempts) > 0) + { + Assert.False(connectionAttemptTimedOut); + + if (stopwatch.Elapsed > 2 * TestHelper.PassingTestTimeout) + { + Assert.Fail("Connection attempts took too long to get cleaned up"); + } + + await Task.Delay(100); + } + + Assert.False(connectionAttemptTimedOut); + } + private sealed class SetTcsContent : StreamContent { private readonly TaskCompletionSource _tcs; From 662bd545cca863a08abce1d67137bbeeeac3805a Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Wed, 18 Dec 2024 18:34:59 +0100 Subject: [PATCH 2/2] Improve comments and naming --- .../ConnectionPool/HttpConnectionPool.Http3.cs | 2 +- .../ConnectionPool/HttpConnectionPool.cs | 16 ++++++++++------ .../ConnectionPool/HttpConnectionWaiter.cs | 2 +- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http3.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http3.cs index b9e2bc2a98edf1..af6fd1e2fb3736 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http3.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http3.cs @@ -127,7 +127,7 @@ internal sealed partial class HttpConnectionPool } finally { - http3ConnectionWaiter?.CancelIfNecessary(this, cancellationToken.IsCancellationRequested); + http3ConnectionWaiter?.SetTimeoutToPendingConnectionAttempt(this, cancellationToken.IsCancellationRequested); } } } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.cs index e72c048b717b2f..fee5a120f8e2e4 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.cs @@ -559,8 +559,8 @@ public async ValueTask SendWithVersionDetectionAndRetryAsyn // We never cancel both attempts at the same time. When downgrade happens, it's possible that both waiters are non-null, // but in that case http2ConnectionWaiter.ConnectionCancellationTokenSource shall be null. Debug.Assert(http11ConnectionWaiter is null || http2ConnectionWaiter?.ConnectionCancellationTokenSource is null); - http11ConnectionWaiter?.CancelIfNecessary(this, cancellationToken.IsCancellationRequested); - http2ConnectionWaiter?.CancelIfNecessary(this, cancellationToken.IsCancellationRequested); + http11ConnectionWaiter?.SetTimeoutToPendingConnectionAttempt(this, cancellationToken.IsCancellationRequested); + http2ConnectionWaiter?.SetTimeoutToPendingConnectionAttempt(this, cancellationToken.IsCancellationRequested); } } } @@ -834,14 +834,18 @@ private CancellationTokenSource GetConnectTimeoutCancellationTokenSource(Http lock (waiter) { + // After a request completes (or is canceled), it will call into SetTimeoutToPendingConnectionAttempt, + // which will no-op if ConnectionCancellationTokenSource is not set, assuming that the connection attempt is done. + // As the initiating request for this connection attempt may complete concurrently at any time, + // there is a race condition where the first call to SetTimeoutToPendingConnectionAttempt may happen + // before we were able to set the CTS, so no timeout will be applied even though the request is already done. waiter.ConnectionCancellationTokenSource = cts; - // The initiating request for this connection attempt may complete concurrently at any time. - // If it completed before we've set the CTS, CancelIfNecessary would no-op. - // Check it again now that we're holding the lock and ensure we always set a timeout. + // To fix that, we check whether the waiter already completed now that we're holding a lock. + // If it had, call SetTimeoutToPendingConnectionAttempt again now that the CTS is set. if (waiter.Task.IsCompleted) { - waiter.CancelIfNecessary(this, requestCancelled: waiter.Task.IsCanceled); + waiter.SetTimeoutToPendingConnectionAttempt(this, requestCancelled: waiter.Task.IsCanceled); waiter.ConnectionCancellationTokenSource = null; } } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionWaiter.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionWaiter.cs index 9e1e0f7127d688..48c8d6a29a550a 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionWaiter.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionWaiter.cs @@ -75,7 +75,7 @@ public bool TrySignal(T connection) } } - public void CancelIfNecessary(HttpConnectionPool pool, bool requestCancelled) + public void SetTimeoutToPendingConnectionAttempt(HttpConnectionPool pool, bool requestCancelled) { int timeout = GlobalHttpSettings.SocketsHttpHandler.PendingConnectionTimeoutOnRequestCompletion; if (ConnectionCancellationTokenSource is null ||