Add a limit on number of pending HTTP/2 PING ACKs

src/libraries/System.Net.Http/src/Resources/Strings.resx

@@ -423,6 +423,9 @@
   <data name="net_http_http2_invalidinitialstreamwindowsize" xml:space="preserve">
     <value>The initial HTTP/2 stream window size must be between {0} and {1}.</value>
   </data>
+  <data name="net_http_http2_frame_limit_exceeded" xml:space="preserve">
+    <value>The HTTP/2 connection was aborted because the size of the frame queue exceeded an internal limit.</value>
+  </data>
   <data name="net_MethodNotImplementedException" xml:space="preserve">
     <value>This method is not implemented by this class.</value>
   </data>

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

@@ -75,6 +75,10 @@ internal sealed partial class Http2Connection : HttpConnectionBase
 
         private Http2ProtocolErrorCode? _goAwayErrorCode;
 
+        // Cap number of untransmitted PING and SETTING ACKs and PING requests
+        private const int MaxQueuedFireAndForgetFrames = 1000;
+        private int _queuedFireAndForgetFrames;
+
         private const int MaxStreamId = int.MaxValue;
 
         // Temporary workaround for request burst handling on connection start.
@@ -926,7 +930,7 @@ private void ProcessSettingsFrame(FrameHeader frameHeader, bool initialFrame = f
 
                 // Send acknowledgement
                 // Don't wait for completion, which could happen asynchronously.
-                LogExceptions(SendSettingsAckAsync());
+                QueueSettingsAck();
             }
         }
 
@@ -1007,7 +1011,7 @@ private void ProcessPingFrame(FrameHeader frameHeader)
             }
             else
             {
-                LogExceptions(SendPingAsync(pingContentLong, isAck: true));
+                QueuePing(pingContentLong, isAck: true);
             }
             _incomingBuffer.Discard(frameHeader.PayloadLength);
         }
@@ -1288,20 +1292,35 @@ private async Task ProcessOutgoingFramesAsync()
             }
         }
 
-        private Task SendSettingsAckAsync() =>
-            PerformWriteAsync(FrameHeader.Size, this, static (thisRef, writeBuffer) =>
+        private void QueueSettingsAck()
+        {
+            if (!TryIncrementQueuedFireAndForgetFrames())
+            {
+                return;
+            }
+
+            LogExceptions(PerformWriteAsync(FrameHeader.Size, this, static (thisRef, writeBuffer) =>
             {
                 if (NetEventSource.Log.IsEnabled()) thisRef.Trace("Started writing.");
 
                 FrameHeader.WriteTo(writeBuffer.Span, 0, FrameType.Settings, FrameFlags.Ack, streamId: 0);
 
+                thisRef.DecrementQueuedFireAndForgetFrames();
+
                 return true;
-            });
+            }));
+        }
 
         /// <param name="pingContent">The 8-byte ping content to send, read as a big-endian integer.</param>
         /// <param name="isAck">Determine whether the frame is ping or ping ack.</param>
-        private Task SendPingAsync(long pingContent, bool isAck = false) =>
-            PerformWriteAsync(FrameHeader.Size + FrameHeader.PingLength, (thisRef: this, pingContent, isAck), static (state, writeBuffer) =>
+        private void QueuePing(long pingContent, bool isAck = false)
+        {
+            if (!TryIncrementQueuedFireAndForgetFrames())
+            {
+                return;
+            }
+
+            LogExceptions(PerformWriteAsync(FrameHeader.Size + FrameHeader.PingLength, (thisRef: this, pingContent, isAck), static (state, writeBuffer) =>
             {
                 if (NetEventSource.Log.IsEnabled()) state.thisRef.Trace($"Started writing. {nameof(pingContent)}={state.pingContent}");
 
@@ -1311,8 +1330,11 @@ private Task SendPingAsync(long pingContent, bool isAck = false) =>
                 FrameHeader.WriteTo(span, FrameHeader.PingLength, FrameType.Ping, state.isAck ? FrameFlags.Ack : FrameFlags.None, streamId: 0);
                 BinaryPrimitives.WriteInt64BigEndian(span.Slice(FrameHeader.Size), state.pingContent);
 
+                state.thisRef.DecrementQueuedFireAndForgetFrames();
+
                 return true;
-            });
+            }));
+        }
 
         private Task SendRstStreamAsync(int streamId, Http2ProtocolErrorCode errorCode) =>
             PerformWriteAsync(FrameHeader.Size + FrameHeader.RstStreamLength, (thisRef: this, streamId, errorCode), static (s, writeBuffer) =>
@@ -1815,6 +1837,28 @@ private bool ForceSendConnectionWindowUpdate()
             return true;
         }
 
+        private bool TryIncrementQueuedFireAndForgetFrames()
+        {
+            if (Interlocked.Increment(ref _queuedFireAndForgetFrames) > MaxQueuedFireAndForgetFrames)
+            {
+                if (NetEventSource.Log.IsEnabled()) this.Trace("Number of untransmitted PING and SETTING frames exceeded limit.");
+
+                // Close connection when there is too much outstanding frames
+                var ex = new HttpIOException(HttpRequestError.Unknown, SR.net_http_http2_frame_limit_exceeded);
+                Abort(ex);
+
+                return false;
+            }
+
+            return true;
+        }
+
+        private void DecrementQueuedFireAndForgetFrames()
+        {
+            int pending = Interlocked.Decrement(ref _queuedFireAndForgetFrames);
+            Debug.Assert(pending >= 0);
+        }
+
         /// <summary>Abort all streams and cause further processing to fail.</summary>
         /// <param name="abortException">Exception causing Abort to be called.</param>
         private void Abort(Exception abortException)
@@ -2173,7 +2217,7 @@ private void VerifyKeepAlive()
                         _keepAlivePingTimeoutTimestamp = now + _keepAlivePingTimeout;
 
                         long pingPayload = Interlocked.Increment(ref _keepAlivePingPayload);
-                        LogExceptions(SendPingAsync(pingPayload));
+                        QueuePing(pingPayload);
                         return;
                     }
                     break;

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

@@ -220,7 +220,7 @@ internal void OnDataOrHeadersReceived(Http2Connection connection, bool sendWindo
                     // Send a PING
                     _pingCounter--;
                     if (NetEventSource.Log.IsEnabled()) connection.Trace($"[FlowControl] Sending RTT PING with payload {_pingCounter}");
-                    connection.LogExceptions(connection.SendPingAsync(_pingCounter, isAck: false));
+                    connection.QueuePing(_pingCounter, isAck: false);
                     _pingSentTimestamp = now;
                     _state = State.PingSent;
                 }

src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs

@@ -3206,6 +3206,176 @@ await conn.WriteFrameAsync(
             await Task.WhenAll(connectionTasks).ConfigureAwait(false);
         }
 
+        [ConditionalFact(typeof(SocketsHttpHandlerTest_Http2), nameof(SupportsAlpn))]
+        public async Task Http2_SettingInFlightLimitExceeded()
+        {
+            // test that client abort connection when server send too much PING/SETTINGs while not processing ACK replies
+
+            await Http2LoopbackServer.CreateClientAndServerAsync(
+                async uri =>
+                {
+                    using HttpClient client = CreateHttpClient();
+
+                    Exception e = await Assert.ThrowsAsync<HttpRequestException>(() => client.GetAsync(uri));
+                    Assert.True(e.Message.StartsWith(SR.net_http_http2_frame_limit_exceeded), "Bad Exception Message");
+
+                    // test that we can open connection after failure
+                    await client.GetAsync(uri);
+                },
+                async server =>
+                {
+                    server.AllowMultipleConnections = true;
+
+                    await using Http2LoopbackConnection con = await server.AcceptConnectionAsync();
+                    while (true)
+                    {
+                        try
+                        {
+                            await con.WriteFrameAsync(new Frame(0, FrameType.Settings, FrameFlags.None, 0));
+                        }
+                        catch (IOException)
+                        {
+                            break;
+                        }
+                    }
+
+                    // second connection will be more lucky
+                    await server.AcceptConnectionSendResponseAndCloseAsync(HttpStatusCode.OK, "ok");
+                });
+        }
+
+        [ConditionalFact(typeof(SocketsHttpHandlerTest_Http2), nameof(SupportsAlpn))]
+        public async Task Http2_PingInFlightLimitExceeded()
+        {
+            // almost exhaust client queue and test that following asynchronous PING issued by client trips
+
+            BlockedWriteNetworkStream? clientNetStream = null;
+
+            await Http2LoopbackServer.CreateClientAndServerAsync(
+                async uri =>
+                {
+                    using HttpClientHandler handler = CreateHttpClientHandler(allowAllCertificates: true);
+                    SocketsHttpHandler socketsHandler = GetUnderlyingSocketsHttpHandler(handler);
+
+                    socketsHandler.ConnectCallback += async (ctx, ct) =>
+                    {
+                        DnsEndPoint dnsEndPoint = ctx.DnsEndPoint;
+                        IPAddress[] addresses = await Dns.GetHostAddressesAsync(dnsEndPoint.Host, dnsEndPoint.AddressFamily, ct);
+
+                        var s = new Socket(SocketType.Stream, ProtocolType.Tcp);
+                        await s.ConnectAsync(addresses, dnsEndPoint.Port, ct);
+
+                        clientNetStream = new BlockedWriteNetworkStream(s, true);
+                        return clientNetStream;
+                    };
+                    socketsHandler.KeepAlivePingDelay = TimeSpan.FromSeconds(1);
+                    socketsHandler.KeepAlivePingPolicy = HttpKeepAlivePingPolicy.Always;
+                    socketsHandler.KeepAlivePingTimeout = TimeSpan.MaxValue;
+
+                    using HttpClient client = CreateHttpClient(handler);
+                    client.DefaultRequestVersion = HttpVersion.Version20;
+                    client.DefaultVersionPolicy = HttpVersionPolicy.RequestVersionExact;
+
+                    Exception e = await Assert.ThrowsAsync<HttpRequestException>(() => client.GetAsync(uri));
+                    Assert.True(e.Message.StartsWith(SR.net_http_http2_frame_limit_exceeded), "Bad Exception Message");
+
+                    // test that we can connect new connection after fail
+                    await client.GetAsync(uri);
+                },
+                async server =>
+                {
+                    server.AllowMultipleConnections = true;
+
+                    await using Http2LoopbackConnection con = await server.AcceptConnectionAsync();
+
+                    // wait for initial frames from client
+                    await con.ReadSettingsAsync();
+                    await con.ReadRequestHeaderAsync();
+                    PingFrame firstPing = await con.ReadPingAsync();
+
+                    long writeCntBeforeSettingsAck = clientNetStream.StartBlocking();
+
+                    // ACK for following SETTINGS will attemp flushing queue on client side and block its processing
+                    await con.WriteFrameAsync(new SettingsFrame());
+
+                    // wait before client attemp to send ACK and channel get blocked.
+                    // Without this wait it may combine SETTINGs ACK and following PING ACKs into
+                    // single buffer which will cause dequeueing them from queue and we fail to
+                    // preload queue to expect 1000 entries as required by test later.
+                    while (clientNetStream.WriteCallCount == writeCntBeforeSettingsAck)
+                    {
+                        await Task.Delay(50);
+                    }
+
+                    // fill the client queue with replies to PING, 1000 exactly fit the queue
+                    for (long i = 0; i < 1000; i++)
+                    {
+                        await con.WriteFrameAsync(new PingFrame(i, FrameFlags.None, 0));
+                    }
+
+                    // now confirm ping we received soon after initiating connection to eneble new heart beat ping
+                    // this should trip and close connection
+                    await con.WriteFrameAsync(new PingFrame(firstPing.Data, FrameFlags.Ack, 0));
+
+                    // test that server/client are operational after failure
+                    await server.AcceptConnectionSendResponseAndCloseAsync();
+                }, new Http2Options()
+                {
+                    UseSsl = false, // SSL introduce buffering which supress effect of blocking
+                    EnableTransparentPingResponse = false // need to observe initial ping
+                });
+        }
+
+        private class BlockedWriteNetworkStream : NetworkStream
+        {
+            private object _lock = new object();
+            private bool _block;
+            private long _writes;
+
+            public long WriteCallCount
+            {
+                get
+                {
+                    lock (_lock)
+                    {
+                        return _writes;
+                    }
+                }
+            }
+
+            public BlockedWriteNetworkStream(Socket socket, bool ownsSocket) : base(socket, ownsSocket) { }
+
+            public long StartBlocking()
+            {
+                lock (_lock)
+                {
+                    _block = true;
+                    return _writes;
+                }
+            }
+
+            public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) =>
+                WriteAsync(buffer.AsMemory(offset, count), cancellationToken).AsTask();
+
+            public override async ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken = default)
+            {
+                bool block;
+                lock (_lock)
+                {
+                    _writes++;
+                    block = _block;
+                }
+
+                if (block)
+                {
+                    await Task.Delay(TimeSpan.FromDays(1), cancellationToken);
+                    throw new Exception("Long delay was canceled early");
+                }
+
+                await base.WriteAsync(buffer, cancellationToken);
+            }
+        }
+
         private async Task VerifySendTasks(IReadOnlyList<Task<HttpResponseMessage>> sendTasks)
         {
             await TestHelper.WhenAllCompletedOrAnyFailed(sendTasks.ToArray()).ConfigureAwait(false);