Fix HTTP/2 Pings not being sent/enforced in multiple scenarios

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http2.cs

@@ -19,6 +19,14 @@ internal sealed partial class HttpConnectionPool
     {
         /// <summary>List of available HTTP/2 connections stored in the pool.</summary>
         private List<Http2Connection>? _availableHttp2Connections;
+        /// <summary>
+        /// HTTP/2 connections created by this pool that haven't completed their teardown yet.
+        /// Unlike <see cref="_availableHttp2Connections"/>, 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 <see cref="HeartBeat"/> working for the whole lifetime of a connection.
+        /// </summary>
+        private List<Http2Connection>? _http2ConnectionsForHeartBeat;
         /// <summary>The number of HTTP/2 connections associated with the pool, including in use, available, and pending.</summary>
         private int _associatedHttp2ConnectionCount;
         /// <summary>Indicates whether an HTTP/2 connection is in the process of being established.</summary>
@@ -576,12 +584,61 @@ public void InvalidateHttp2Connection(Http2Connection connection)
             }
         }
 
-        public void HeartBeat()
+        /// <summary>Whether HTTP/2 connections in this pool should be sending keep alive PINGs.</summary>
+        private bool Http2KeepAlivePingEnabled => Settings._keepAlivePingDelay != Timeout.InfiniteTimeSpan;
+
+        /// <summary>
+        /// Registers a newly created HTTP/2 connection with the pool so that it participates in <see cref="HeartBeat"/>.
+        /// Called from the <see cref="Http2Connection"/> constructor so that the connection is tracked even if it
+        /// tears down before it's ever handed out to a request.
+        /// </summary>
+        public void AddHttp2ConnectionForHeartBeat(Http2Connection connection)
+        {
+            Debug.Assert(!HasSyncObjLock);
+
+            if (!Http2KeepAlivePingEnabled)
+            {
+                return;
+            }
+
+            lock (SyncObj)
+            {
+                (_http2ConnectionsForHeartBeat ??= new List<Http2Connection>()).Add(connection);
+            }
+        }
+
+        /// <summary>Called when an HTTP/2 connection has completed its teardown and no longer needs heart beats.</summary>
+        public void RemoveHttp2ConnectionFromHeartBeat(Http2Connection connection)
+        {
+            Debug.Assert(!HasSyncObjLock);
+
+            if (!Http2KeepAlivePingEnabled)
+            {
+                return;
+            }
+
+            lock (SyncObj)
+            {
+                bool removed = _http2ConnectionsForHeartBeat?.Remove(connection) ?? false;
+                Debug.Assert(removed);
+            }
+        }
+
+        /// <summary>
+        /// Sends keep alive PINGs on all live HTTP/2 connections.
+        /// Returns whether the pool may still have HTTP/2 connections that need heart beats.
+        /// </summary>
+        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<Http2Connection> list, ref List<HttpConnectionBase>? toDispose, long nowTicks, TimeSpan pooledConnectionLifetime, TimeSpan pooledConnectionIdleTimeout)

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.

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;
+        /// <summary>Set once <see cref="SetupAsync"/> completes. Until then, no keep alive PINGs are sent.</summary>
+        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();
         }
 

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs

@@ -37,7 +37,7 @@ internal sealed class HttpConnectionPoolManager : IDisposable
         private readonly ConcurrentDictionary<HttpConnectionKey, HttpConnectionPool> _pools;
         /// <summary>Timer used to initiate cleaning of the pools.</summary>
         private readonly Timer? _cleaningTimer;
-        /// <summary>Heart beat timer currently used for Http2 ping only.</summary>
+        /// <summary>Heart beat timer currently used for Http2 ping only. Not stopped by <see cref="Dispose"/>; it stops itself.</summary>
         private readonly Timer? _heartBeatTimer;
 
         private readonly HttpConnectionSettings _settings;
@@ -53,6 +53,8 @@ internal sealed class HttpConnectionPoolManager : IDisposable
         /// <see cref="ConcurrentDictionary{TKey,TValue}.IsEmpty"/> call.
         /// </summary>
         private bool _timerIsRunning;
+        /// <summary>Whether <see cref="Dispose"/> has been called.</summary>
+        private bool _disposed;
         /// <summary>Object used to synchronize access to state in the pool.</summary>
         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<HttpConnectionPoolManager>)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<HttpResponseMessage> SendAsyncMultiProxy(HttpRequestMess
         /// <summary>Disposes of the pools, disposing of each individual pool.</summary>
         public void Dispose()
         {
+            _disposed = true;
             _cleaningTimer?.Dispose();
-            _heartBeatTimer?.Dispose();
             foreach (KeyValuePair<HttpConnectionKey, HttpConnectionPool> 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()
+        /// <summary>Sends keep alive PINGs on all pooled connections, and reports whether any connections remain.</summary>
+        private bool HeartBeat()
         {
+            bool anyLiveConnections = false;
+
             foreach (KeyValuePair<HttpConnectionKey, HttpConnectionPool> pool in _pools)
             {
-                pool.Value.HeartBeat();
+                anyLiveConnections |= pool.Value.HeartBeat();
             }
+
+            return anyLiveConnections;
         }
 
         private static string GetIdentityIfDefaultCredentialsUsed(bool defaultCredentialsUsed)