Remove unused HTTP/2 streams from pool (#24767)

src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs

@@ -72,6 +72,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2
 
         internal const int InitialStreamPoolSize = 5;
         internal const int MaxStreamPoolSize = 100;
+        internal const long StreamPoolExpiryTicks = TimeSpan.TicksPerSecond * 5;
 
         public Http2Connection(HttpConnectionContext context)
         {
@@ -218,6 +219,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2
                     // Call UpdateCompletedStreams() prior to frame processing in order to remove any streams that have exceeded their drain timeouts.
                     UpdateCompletedStreams();
 
+                    if (result.IsCanceled)
+                    {
+                        // Heartbeat will cancel ReadAsync and trigger expiring unused streams from pool.
+                        StreamPool.RemoveExpired(SystemClock.UtcNowTicks);
+                    }
+
                     try
                     {
                         bool frameReceived = false;
@@ -647,6 +654,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2
 
             if (StreamPool.Count < MaxStreamPoolSize)
             {
+                // This property is used to remove unused streams from the pool
+                stream.DrainExpirationTicks = SystemClock.UtcNowTicks + StreamPoolExpiryTicks;
+
                 StreamPool.Push(stream);
             }
         }

src/Servers/Kestrel/Core/src/Internal/Http2/Http2StreamStack.cs

@@ -9,7 +9,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2
     // See https://github.com/dotnet/runtime/blob/master/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/BufferSegmentStack.cs
     internal struct Http2StreamStack
     {
-        private Http2StreamAsValueType[] _array;
+        // Internal for testing
+        internal Http2StreamAsValueType[] _array;
         private int _size;
 
         public Http2StreamStack(int size)
@@ -78,7 +79,59 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2
             _size++;
         }
 
-        private readonly struct Http2StreamAsValueType
+        public void RemoveExpired(long now)
+        {
+            int size = _size;
+            Http2StreamAsValueType[] array = _array;
+
+            var removeCount = CalculateRemoveCount(now, size, array);
+            if (removeCount == 0)
+            {
+                return;
+            }
+
+            var newSize = size - removeCount;
+
+            // Dispose removed streams
+            for (var i = 0; i < removeCount; i++)
+            {
+                Http2Stream stream = array[i];
+                stream.Dispose();
+            }
+
+            // Move remaining streams
+            for (var i = 0; i < newSize; i++)
+            {
+                array[i] = array[i + removeCount];
+            }
+
+            // Clear unused array indexes
+            for (var i = newSize; i < size; i++)
+            {
+                array[i] = default;
+            }
+
+            _size = newSize;
+        }
+
+        private static int CalculateRemoveCount(long now, int size, Http2StreamAsValueType[] array)
+        {
+            for (var i = 0; i < size; i++)
+            {
+                Http2Stream stream = array[i];
+                if (stream.DrainExpirationTicks >= now)
+                {
+                    // Stream is still valid. All streams after this will have a later expiration.
+                    // No reason to keep checking. Return count of streams to remove.
+                    return i;
+                }
+            }
+
+            // All will be removed.
+            return size;
+        }
+
+        internal readonly struct Http2StreamAsValueType
         {
             private readonly Http2Stream _value;
             private Http2StreamAsValueType(Http2Stream value) => _value = value;

src/Servers/Kestrel/Core/test/Http2StreamStackTests.cs

@@ -0,0 +1,113 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using Microsoft.AspNetCore.Http;
+using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2;
+using Microsoft.AspNetCore.Testing;
+using Xunit;
+
+namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests
+{
+    public class Http2StreamStackTests
+    {
+        [Fact]
+        public void RemoveExpired_Empty_NoOp()
+        {
+            var streams = new Http2StreamStack(10);
+
+            streams.RemoveExpired(100);
+        }
+
+        [Fact]
+        public void RemoveExpired_NoneExpired_NoOp()
+        {
+            var streams = new Http2StreamStack(10);
+            streams.Push(CreateStream(streamId: 1, expirationTicks: 200));
+
+            streams.RemoveExpired(100);
+
+            Assert.Equal(1, streams.Count);
+            Assert.Equal(1, ((Http2Stream)streams._array[0]).StreamId);
+        }
+
+        [Fact]
+        public void RemoveExpired_OneExpired_ExpiredStreamRemoved()
+        {
+            var streams = new Http2StreamStack(10);
+            streams.Push(CreateStream(streamId: 1, expirationTicks: 200));
+
+            streams.RemoveExpired(300);
+
+            Assert.Equal(0, streams.Count);
+            Assert.Equal(default, streams._array[0]);
+        }
+
+        [Fact]
+        public void RemoveExpired_MultipleExpired_ExpiredStreamsRemoved()
+        {
+            var streams = new Http2StreamStack(10);
+            streams.Push(CreateStream(streamId: 1, expirationTicks: 200));
+            streams.Push(CreateStream(streamId: 2, expirationTicks: 250));
+
+            streams.RemoveExpired(300);
+
+            Assert.Equal(0, streams.Count);
+            Assert.Equal(default, streams._array[0]);
+            Assert.Equal(default, streams._array[1]);
+        }
+
+        [Fact]
+        public void RemoveExpired_OneExpiredAndOneValid_ExpiredStreamRemoved()
+        {
+            var streams = new Http2StreamStack(10);
+            streams.Push(CreateStream(streamId: 1, expirationTicks: 200));
+            streams.Push(CreateStream(streamId: 2, expirationTicks: 400));
+
+            streams.RemoveExpired(300);
+
+            Assert.Equal(1, streams.Count);
+            Assert.Equal(2, ((Http2Stream)streams._array[0]).StreamId);
+            Assert.Equal(default, streams._array[1]);
+        }
+
+        [Fact]
+        public void RemoveExpired_AllExpired_ExpiredStreamRemoved()
+        {
+            var streams = new Http2StreamStack(5);
+            streams.Push(CreateStream(streamId: 1, expirationTicks: 200));
+            streams.Push(CreateStream(streamId: 2, expirationTicks: 200));
+            streams.Push(CreateStream(streamId: 3, expirationTicks: 200));
+            streams.Push(CreateStream(streamId: 4, expirationTicks: 200));
+            streams.Push(CreateStream(streamId: 5, expirationTicks: 200));
+
+            streams.RemoveExpired(300);
+
+            Assert.Equal(0, streams.Count);
+            Assert.Equal(5, streams._array.Length);
+            Assert.Equal(default, streams._array[0]);
+            Assert.Equal(default, streams._array[1]);
+            Assert.Equal(default, streams._array[2]);
+            Assert.Equal(default, streams._array[3]);
+            Assert.Equal(default, streams._array[4]);
+        }
+
+        private static Http2Stream<HttpContext> CreateStream(int streamId, long expirationTicks)
+        {
+            var context = new Http2StreamContext
+            {
+                StreamId = streamId,
+                ServiceContext = new Internal.ServiceContext
+                {
+                    ServerOptions = new KestrelServerOptions()
+                },
+                ServerPeerSettings = new Http2PeerSettings(),
+                ClientPeerSettings = new Http2PeerSettings()
+            };
+
+            return new Http2Stream<HttpContext>(new DummyApplication(), context)
+            {
+                DrainExpirationTicks = expirationTicks
+            };
+        }
+    }
+}

src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs

@@ -478,7 +478,58 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests
             Assert.Equal(0, _connection.StreamPool.Count);
 
             await StopConnectionAsync(expectedLastStreamId: 3, ignoreNonGoAwayFrames: false);
+        }
 
+        [Fact]
+        public async Task StreamPool_UnusedExpiredStream_RemovedFromPool()
+        {
+            DateTimeOffset now = _serviceContext.MockSystemClock.UtcNow;
+
+            // Heartbeat
+            TriggerTick(now);
+
+            var serverTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+
+            await InitializeConnectionAsync(async context =>
+            {
+                await _echoApplication(context);
+            });
+
+            Assert.Equal(0, _connection.StreamPool.Count);
+
+            await StartStreamAsync(1, _browserRequestHeaders, endStream: true);
+
+            await ExpectAsync(Http2FrameType.HEADERS,
+                withLength: 36,
+                withFlags: (byte)(Http2HeadersFrameFlags.END_HEADERS | Http2HeadersFrameFlags.END_STREAM),
+                withStreamId: 1);
+
+            // Ping will trigger the stream to be returned to the pool so we can assert it
+            await SendPingAsync(Http2PingFrameFlags.NONE);
+            await ExpectAsync(Http2FrameType.PING,
+                withLength: 8,
+                withFlags: (byte)Http2PingFrameFlags.ACK,
+                withStreamId: 0);
+
+            // Stream has been returned to the pool
+            Assert.Equal(1, _connection.StreamPool.Count);
+
+            _connection.StreamPool.TryPeek(out var pooledStream);
+
+            TriggerTick(now + TimeSpan.FromSeconds(1));
+
+            // Stream has not expired and is still in pool
+            Assert.Equal(1, _connection.StreamPool.Count);
+
+            TriggerTick(now + TimeSpan.FromSeconds(6));
+
+            // Stream has expired and has been removed from pool
+            Assert.Equal(0, _connection.StreamPool.Count);
+
+            // Removed stream should have been disposed
+            Assert.True(((Http2OutputProducer)pooledStream.Output)._disposed);
+
+            await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false);
         }
 
         [Fact]