From 759b2ab6311050b3232bb0f3a41ce4ba0bfedb9b Mon Sep 17 00:00:00 2001 From: MihaZupan Date: Thu, 30 Jul 2026 13:49:40 +0200 Subject: [PATCH] Fix HTTP/2 Pings not being sent/enforced in multiple scenarios --- .../HttpConnectionPool.Http2.cs | 63 +++++- .../ConnectionPool/HttpConnectionPool.cs | 4 +- .../SocketsHttpHandler/Http2Connection.cs | 32 ++- .../HttpConnectionPoolManager.cs | 27 ++- ...cketsHttpHandlerTest.Http2KeepAlivePing.cs | 203 +++++++++++++++++- 5 files changed, 317 insertions(+), 12 deletions(-) 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 e7649f8308c46f..c2b77774e0ae8a 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 @@ -19,6 +19,14 @@ internal sealed partial class HttpConnectionPool { /// List of available HTTP/2 connections stored in the pool. private List? _availableHttp2Connections; + /// + /// HTTP/2 connections created by this pool that haven't completed their teardown yet. + /// Unlike , this also includes connections that reached + /// their stream limit, or that are shutting down (e.g. after a GOAWAY frame) but are still + /// processing requests. Only tracked if keep alive pings are enabled, as this list exists + /// solely to keep working for the whole lifetime of a connection. + /// + private List? _http2ConnectionsForHeartBeat; /// The number of HTTP/2 connections associated with the pool, including in use, available, and pending. private int _associatedHttp2ConnectionCount; /// Indicates whether an HTTP/2 connection is in the process of being established. @@ -576,12 +584,61 @@ public void InvalidateHttp2Connection(Http2Connection connection) } } - public void HeartBeat() + /// Whether HTTP/2 connections in this pool should be sending keep alive PINGs. + private bool Http2KeepAlivePingEnabled => Settings._keepAlivePingDelay != Timeout.InfiniteTimeSpan; + + /// + /// Registers a newly created HTTP/2 connection with the pool so that it participates in . + /// Called from the constructor so that the connection is tracked even if it + /// tears down before it's ever handed out to a request. + /// + public void AddHttp2ConnectionForHeartBeat(Http2Connection connection) + { + Debug.Assert(!HasSyncObjLock); + + if (!Http2KeepAlivePingEnabled) + { + return; + } + + lock (SyncObj) + { + (_http2ConnectionsForHeartBeat ??= new List()).Add(connection); + } + } + + /// Called when an HTTP/2 connection has completed its teardown and no longer needs heart beats. + public void RemoveHttp2ConnectionFromHeartBeat(Http2Connection connection) + { + Debug.Assert(!HasSyncObjLock); + + if (!Http2KeepAlivePingEnabled) + { + return; + } + + lock (SyncObj) + { + bool removed = _http2ConnectionsForHeartBeat?.Remove(connection) ?? false; + Debug.Assert(removed); + } + } + + /// + /// Sends keep alive PINGs on all live HTTP/2 connections. + /// Returns whether the pool may still have HTTP/2 connections that need heart beats. + /// + public bool HeartBeat() { Http2Connection[]? localHttp2Connections; + bool anyConnections; lock (SyncObj) { - localHttp2Connections = _availableHttp2Connections?.ToArray(); + localHttp2Connections = _http2ConnectionsForHeartBeat?.ToArray(); + + // Also account for connections that are still being established -- they aren't in the + // list yet, but they will need heart beats as soon as they are. + anyConnections = localHttp2Connections is { Length: > 0 } || _associatedHttp2ConnectionCount > 0; } // Avoid calling HeartBeat under the lock, as it may call back into HttpConnectionPool.InvalidateHttp2Connection. @@ -592,6 +649,8 @@ public void HeartBeat() http2Connection.HeartBeat(); } } + + return anyConnections; } private static int ScavengeHttp2ConnectionList(List list, ref List? toDispose, long nowTicks, TimeSpan pooledConnectionLifetime, TimeSpan pooledConnectionIdleTimeout) 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 a6780974c67b39..c52ea594d2c05b 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 @@ -1102,7 +1102,9 @@ public bool CleanCacheAndDisposeIfUnused() // if a pool was used since the last time we cleaned up, give it another chance. New pools // start out saying they've recently been used, to give them a bit of breathing room and time // for the initial collection to be added to it. - if (!_usedSinceLastCleanup && _associatedHttp11ConnectionCount == 0 && _associatedHttp2ConnectionCount == 0) + if (!_usedSinceLastCleanup && _associatedHttp11ConnectionCount == 0 && _associatedHttp2ConnectionCount == 0 && + // An HTTP/2 connection may still be draining requests (e.g. after a GOAWAY frame) and need heart beats. + (_http2ConnectionsForHeartBeat?.Count ?? 0) == 0) { _disposed = true; return true; // Pool is disposed of. It should be removed. diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs index a519390890fd7c..77b82487063dfc 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs @@ -134,6 +134,8 @@ internal enum KeepAliveState private long _nextPingRequestTimestamp; private long _keepAlivePingTimeoutTimestamp; private volatile KeepAliveState _keepAliveState; + /// Set once completes. Until then, no keep alive PINGs are sent. + private bool _setupComplete; public Http2Connection(HttpConnectionPool pool, Stream stream, Activity? connectionSetupActivity, IPEndPoint? remoteEndPoint, long connectionId) : base(pool, connectionId, connectionSetupActivity, remoteEndPoint) @@ -176,6 +178,10 @@ public Http2Connection(HttpConnectionPool pool, Stream stream, Activity? connect if (NetEventSource.Log.IsEnabled()) TraceConnection(_stream); + // Register with the pool before doing anything that may tear the connection down, + // so that the pool can run keep alive ping logic for the whole lifetime of the connection. + pool.AddHttp2ConnectionForHeartBeat(this); + static long TimeSpanToMs(TimeSpan value) { double milliseconds = value.TotalMilliseconds; @@ -270,6 +276,9 @@ public async ValueTask SetupAsync(CancellationToken cancellationToken) { _ = ProcessOutgoingFramesAsync(); } + + // The connection is now able to write frames, so it may start sending keep alive PINGs. + _setupComplete = true; } private void Shutdown() @@ -1353,8 +1362,27 @@ internal void HeartBeat() { Debug.Assert(!_pool.HasSyncObjLock); - if (_shutdown) + if (!_setupComplete) + { + // The connection is still being established. It can't send PINGs yet, and a server that + // never completes the handshake is the connect timeout's responsibility, not ours. return; + } + + if (_shutdown) + { + // The connection is shutting down (e.g. we received a GOAWAY frame), but it may still be + // processing existing requests. Keep sending PINGs while it does, as that's the only way + // to detect that the server became unresponsive. Once the last stream completes, the + // connection is torn down and unregistered from the pool, so we'll stop being called. + lock (SyncObject) + { + if (_streamsInUse == 0) + { + return; + } + } + } try { @@ -1913,6 +1941,8 @@ private void FinalTeardown() // ProcessIncomingFramesAsync and ProcessOutgoingFramesAsync respectively, and those methods are // responsible for returning the buffers. + _pool.RemoveHttp2ConnectionFromHeartBeat(this); + MarkConnectionAsClosed(); } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs index dbae72a72718d9..842056324c9190 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs @@ -37,7 +37,7 @@ internal sealed class HttpConnectionPoolManager : IDisposable private readonly ConcurrentDictionary _pools; /// Timer used to initiate cleaning of the pools. private readonly Timer? _cleaningTimer; - /// Heart beat timer currently used for Http2 ping only. + /// Heart beat timer currently used for Http2 ping only. Not stopped by ; it stops itself. private readonly Timer? _heartBeatTimer; private readonly HttpConnectionSettings _settings; @@ -53,6 +53,8 @@ internal sealed class HttpConnectionPoolManager : IDisposable /// call. /// private bool _timerIsRunning; + /// Whether has been called. + private bool _disposed; /// Object used to synchronize access to state in the pool. private object SyncObj => _pools; @@ -128,12 +130,20 @@ public HttpConnectionPoolManager(HttpConnectionSettings settings) { long heartBeatInterval = (long)Math.Max(1000, Math.Min(_settings._keepAlivePingDelay.TotalMilliseconds, _settings._keepAlivePingTimeout.TotalMilliseconds) / 4); + // Unlike the cleaning timer, this one is deliberately not stopped by Dispose. + // Requests that were already in flight keep running after the handler is disposed, + // and they must keep sending keep alive PINGs to detect an unresponsive server. + // Instead, the timer stops itself once the manager has been disposed and has no + // connections left. If the manager becomes unreachable without ever being disposed, + // the timer becomes unreachable with it and is stopped by its own finalizer. _heartBeatTimer = new Timer(static state => { var wr = (WeakReference)state!; - if (wr.TryGetTarget(out HttpConnectionPoolManager? thisRef)) + if (wr.TryGetTarget(out HttpConnectionPoolManager? manager) && + !manager.HeartBeat() && + manager._disposed) { - thisRef.HeartBeat(); + manager._heartBeatTimer?.Dispose(); } }, thisRef, heartBeatInterval, heartBeatInterval); } @@ -486,8 +496,8 @@ private async ValueTask SendAsyncMultiProxy(HttpRequestMess /// Disposes of the pools, disposing of each individual pool. public void Dispose() { + _disposed = true; _cleaningTimer?.Dispose(); - _heartBeatTimer?.Dispose(); foreach (KeyValuePair pool in _pools) { pool.Value.Dispose(); @@ -542,12 +552,17 @@ private void RemoveStalePools() // be returned to pools they weren't associated with. } - private void HeartBeat() + /// Sends keep alive PINGs on all pooled connections, and reports whether any connections remain. + private bool HeartBeat() { + bool anyLiveConnections = false; + foreach (KeyValuePair pool in _pools) { - pool.Value.HeartBeat(); + anyLiveConnections |= pool.Value.HeartBeat(); } + + return anyLiveConnections; } private static string GetIdentityIfDefaultCredentialsUsed(bool defaultCredentialsUsed) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2KeepAlivePing.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2KeepAlivePing.cs index 341c929dcf004e..b9bc85cfb45065 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2KeepAlivePing.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2KeepAlivePing.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Collections.Generic; +using System.Diagnostics; using System.IO; using System.Linq; using System.Net.Test.Common; @@ -316,6 +317,204 @@ await Http2LoopbackServer.CreateClientAndServerAsync(async uri => }, NoAutoPingResponseHttp2Options); } + [OuterLoop("Runs long")] + [Fact] + public async Task KeepAliveConfigured_ConnectionShutDownByGoAway_KeepAlivePingsAreStillSent() + { + await Http2LoopbackServer.CreateClientAndServerAsync(async uri => + { + SocketsHttpHandler handler = CreateSocketsHttpHandler(allowAllCertificates: true); + handler.KeepAlivePingTimeout = TimeSpan.FromSeconds(10); + handler.KeepAlivePingPolicy = HttpKeepAlivePingPolicy.WithActiveRequests; + handler.KeepAlivePingDelay = TimeSpan.FromSeconds(1); + + using HttpClient client = new HttpClient(handler); + client.DefaultRequestVersion = HttpVersion.Version20; + client.Timeout = TestHelper.PassingTestTimeout; + + using HttpResponseMessage response = await client.GetAsync(uri, HttpCompletionOption.ResponseHeadersRead); + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + await response.Content.ReadAsStream().CopyToAsync(Stream.Null); + }, + async server => + { + await EstablishConnectionAsync(server); + + int streamId = await ReadRequestHeaderAsync(); + await GuardConnectionWriteAsync(() => _connection.SendResponseHeadersAsync(streamId, endStream: false)); + + // Tell the client that the connection is going away, but that this stream will still be processed. + // This removes the connection from the pool, but it must keep sending PINGs while the request is active. + await GuardConnectionWriteAsync(() => _connection.SendGoAway(streamId)); + + await WaitForKeepAlivePingsAndFinishResponseAsync(streamId); + + await TerminateLoopbackConnectionAsync(); + }, NoAutoPingResponseHttp2Options); + } + + [OuterLoop("Runs long")] + [Fact] + public async Task KeepAliveConfigured_NoPingResponseAfterGoAway_RequestShouldFail() + { + // This is the user-visible symptom of a connection no longer being pinged after a GOAWAY frame: + // a long-lived request on a connection whose transport silently died would hang forever. + _sendPingResponse = false; + + await Http2LoopbackServer.CreateClientAndServerAsync(async uri => + { + SocketsHttpHandler handler = CreateSocketsHttpHandler(allowAllCertificates: true); + handler.KeepAlivePingTimeout = TimeSpan.FromSeconds(1.5); + handler.KeepAlivePingPolicy = HttpKeepAlivePingPolicy.WithActiveRequests; + handler.KeepAlivePingDelay = TimeSpan.FromSeconds(1); + + using HttpClient client = new HttpClient(handler); + client.DefaultRequestVersion = HttpVersion.Version20; + client.Timeout = TestHelper.PassingTestTimeout; + + using HttpResponseMessage response = await client.GetAsync(uri, HttpCompletionOption.ResponseHeadersRead); + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + // The request must be torn down once the server stops responding to the keep alive PINGs. + HttpProtocolException ex = await Assert.ThrowsAsync(() => response.Content.ReadAsStream().CopyToAsync(Stream.Null)); + Assert.Equal(HttpRequestError.HttpProtocolError, ex.HttpRequestError); + Assert.Contains("KeepAlivePingDelay", ex.Message); + + await _serverFinished.Task.WaitAsync(TestTimeout); + }, + async server => + { + await EstablishConnectionAsync(server); + + int streamId = await ReadRequestHeaderAsync(); + await GuardConnectionWriteAsync(() => _connection.SendResponseHeadersAsync(streamId, endStream: false)); + + await GuardConnectionWriteAsync(() => _connection.SendGoAway(streamId)); + + // Wait for the client to disconnect due to hitting the KeepAliveTimeout. + await _incomingFramesTask; + + await TerminateLoopbackConnectionAsync(); + }, NoAutoPingResponseHttp2Options); + } + + [OuterLoop("Runs long")] + [Fact] + public async Task KeepAliveConfigured_ConnectionAtMaxConcurrentStreams_KeepAlivePingsAreStillSent() + { + await Http2LoopbackServer.CreateClientAndServerAsync(async uri => + { + SocketsHttpHandler handler = CreateSocketsHttpHandler(allowAllCertificates: true); + handler.KeepAlivePingTimeout = TimeSpan.FromSeconds(10); + handler.KeepAlivePingPolicy = HttpKeepAlivePingPolicy.WithActiveRequests; + handler.KeepAlivePingDelay = TimeSpan.FromSeconds(1); + + using HttpClient client = new HttpClient(handler); + client.DefaultRequestVersion = HttpVersion.Version20; + client.Timeout = TestHelper.PassingTestTimeout; + + // Warmup request, ensuring that the connection picked up the server's MaxConcurrentStreams setting. + using HttpResponseMessage warmupResponse = await client.GetAsync(uri); + Assert.Equal(HttpStatusCode.OK, warmupResponse.StatusCode); + + // This request occupies the connection's only stream. + using HttpResponseMessage response = await client.GetAsync(uri, HttpCompletionOption.ResponseHeadersRead); + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + // A second request can't be served until the first one completes. Attempting it removes the + // connection from the pool, but it must keep sending PINGs while the first request is active. + Task queuedRequest = client.GetAsync(uri); + + await response.Content.ReadAsStream().CopyToAsync(Stream.Null); + + using HttpResponseMessage queuedResponse = await queuedRequest; + Assert.Equal(HttpStatusCode.OK, queuedResponse.StatusCode); + }, + async server => + { + await EstablishConnectionAsync(server, new SettingsEntry { SettingId = SettingId.MaxConcurrentStreams, Value = 1 }); + + int warmupStreamId = await ReadRequestHeaderAsync(); + await GuardConnectionWriteAsync(() => _connection.SendDefaultResponseAsync(warmupStreamId)); + + int streamId = await ReadRequestHeaderAsync(); + await GuardConnectionWriteAsync(() => _connection.SendResponseHeadersAsync(streamId, endStream: false)); + + await WaitForKeepAlivePingsAndFinishResponseAsync(streamId); + + // The queued request is only sent out after the previous one completed. + int queuedStreamId = await ReadRequestHeaderAsync(); + await GuardConnectionWriteAsync(() => _connection.SendDefaultResponseAsync(queuedStreamId)); + + await TerminateLoopbackConnectionAsync(); + }, NoAutoPingResponseHttp2Options); + } + + [OuterLoop("Runs long")] + [Fact] + public async Task KeepAliveConfigured_HandlerDisposedWithActiveRequest_KeepAlivePingsAreStillSent() + { + await Http2LoopbackServer.CreateClientAndServerAsync(async uri => + { + SocketsHttpHandler handler = CreateSocketsHttpHandler(allowAllCertificates: true); + handler.KeepAlivePingTimeout = TimeSpan.FromSeconds(10); + handler.KeepAlivePingPolicy = HttpKeepAlivePingPolicy.WithActiveRequests; + handler.KeepAlivePingDelay = TimeSpan.FromSeconds(1); + + using HttpClient client = new HttpClient(handler, disposeHandler: false); + client.DefaultRequestVersion = HttpVersion.Version20; + client.Timeout = TestHelper.PassingTestTimeout; + + using HttpResponseMessage response = await client.GetAsync(uri, HttpCompletionOption.ResponseHeadersRead); + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + // Disposing the handler doesn't affect requests that are already in flight, + // so they must keep sending keep alive PINGs. + handler.Dispose(); + + await response.Content.ReadAsStream().CopyToAsync(Stream.Null); + }, + async server => + { + await EstablishConnectionAsync(server); + + int streamId = await ReadRequestHeaderAsync(); + await GuardConnectionWriteAsync(() => _connection.SendResponseHeadersAsync(streamId, endStream: false)); + + await WaitForKeepAlivePingsAndFinishResponseAsync(streamId); + + await TerminateLoopbackConnectionAsync(); + }, NoAutoPingResponseHttp2Options); + } + + /// Waits for keep alive PINGs to arrive while the request is active, then completes the response. + private async Task WaitForKeepAlivePingsAndFinishResponseAsync(int streamId) + { + Interlocked.Exchange(ref _pingCounter, 0); // reset the PING counter + + Stopwatch stopwatch = Stopwatch.StartNew(); + bool receivedPings = false; + + // Don't send anything on the stream while waiting -- every frame the client receives + // resets its ping timer, so the connection must stay quiet for the PINGs to be sent. + while (stopwatch.Elapsed < TestHelper.PassingTestTimeout) + { + if (Volatile.Read(ref _pingCounter) > 0) + { + receivedPings = true; + break; + } + + await Task.Delay(100); + } + + Assert.True(receivedPings, "Timed out waiting for keep alive PINGs."); + + // Finish the response. + await GuardConnectionWriteAsync(() => _connection.SendResponseBodyAsync(streamId, new byte[64], isFinal: true)); + } + private async Task ProcessIncomingFramesAsync(CancellationToken cancellationToken) { try @@ -370,9 +569,9 @@ private async Task ProcessIncomingFramesAsync(CancellationToken cancellationToke await _connection.DisposeAsync(); } - private async Task EstablishConnectionAsync(Http2LoopbackServer server) + private async Task EstablishConnectionAsync(Http2LoopbackServer server, params SettingsEntry[] settingsEntries) { - _connection = await server.EstablishConnectionAsync(); + _connection = await server.EstablishConnectionAsync(settingsEntries); _incomingFramesTask = ProcessIncomingFramesAsync(_incomingFramesCts.Token); }