diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs index 848aca40c7..c3dbfb5d02 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs @@ -65,7 +65,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 private int _gracefulCloseInitiator; private int _isClosed; - private Http2StreamStack _streamPool; + // Internal for testing + internal Http2StreamStack StreamPool; internal const int InitialStreamPoolSize = 5; internal const int MaxStreamPoolSize = 40; @@ -111,7 +112,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 _serverSettings.InitialWindowSize = (uint)http2Limits.InitialStreamWindowSize; // Start pool off at a smaller size if the max number of streams is less than the InitialStreamPoolSize - _streamPool = new Http2StreamStack(Math.Min(InitialStreamPoolSize, http2Limits.MaxStreamsPerConnection)); + StreamPool = new Http2StreamStack(Math.Min(InitialStreamPoolSize, http2Limits.MaxStreamsPerConnection)); _inputTask = ReadInputAsync(); } @@ -572,7 +573,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 private Http2Stream GetStream(IHttpApplication application) { - if (_streamPool.TryPop(out var stream)) + if (StreamPool.TryPop(out var stream)) { stream.InitializeWithExistingContext(_incomingFrame.StreamId); return stream; @@ -606,9 +607,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 private void ReturnStream(Http2Stream stream) { - if (_streamPool.Count < MaxStreamPoolSize) + if (StreamPool.Count < MaxStreamPoolSize) { - _streamPool.Push(stream); + StreamPool.Push(stream); } } @@ -972,8 +973,13 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 catch (Http2StreamErrorException) { MakeSpaceInDrainQueue(); - // Tracked for draining - _completedStreams.Enqueue(_currentHeadersStream); + + // Because this stream isn't being queued, OnRequestProcessingEnded will not be + // automatically called and the stream won't be completed. + // Manually complete stream to ensure pipes are completed. + // Completing the stream will add it to the completed stream queue. + _currentHeadersStream.DecrementActiveClientStreamCount(); + _currentHeadersStream.CompleteStream(errored: true); throw; } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs index 7a6b75de8b..7ad02fb966 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs @@ -103,6 +103,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 } protected override void OnRequestProcessingEnded() + { + CompleteStream(errored: false); + } + + public void CompleteStream(bool errored) { try { @@ -110,14 +115,23 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 // If the app finished without reading the request body tell the client not to finish sending it. if (!EndStreamReceived && !RstStreamReceived) { - Log.RequestBodyNotEntirelyRead(ConnectionIdFeature, TraceIdentifier); + if (!errored) + { + Log.RequestBodyNotEntirelyRead(ConnectionIdFeature, TraceIdentifier); + } var (oldState, newState) = ApplyCompletionFlag(StreamCompletionFlags.Aborted); if (oldState != newState) { Debug.Assert(_decrementCalled); - // Don't block on IO. This never faults. - _ = _http2Output.WriteRstStreamAsync(Http2ErrorCode.NO_ERROR); + + // If there was an error starting the stream then we don't want to write RST_STREAM here. + // The connection will handle writing RST_STREAM with the correct error code. + if (!errored) + { + // Don't block on IO. This never faults. + _ = _http2Output.WriteRstStreamAsync(Http2ErrorCode.NO_ERROR); + } RequestBodyPipe.Writer.Complete(); } } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs index 9814918386..857c9a3e96 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs @@ -14,6 +14,7 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.Connections; using Microsoft.AspNetCore.Server.Kestrel.Core.Features; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2; +using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure; using Microsoft.AspNetCore.Testing; using Microsoft.Extensions.Logging; using Microsoft.Net.Http.Headers; @@ -24,6 +25,202 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests { public class Http2ConnectionTests : Http2TestBase { + [Fact] + public async Task StreamPool_SingleStream_ReturnedToPool() + { + await InitializeConnectionAsync(_echoApplication); + + Assert.Equal(0, _connection.StreamPool.Count); + + await StartStreamAsync(1, _browserRequestHeaders, endStream: true); + + await ExpectAsync(Http2FrameType.HEADERS, + withLength: 55, + 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); + + await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); + } + + [Fact] + public async Task StreamPool_MultipleStreamsConcurrent_StreamsReturnedToPool() + { + await InitializeConnectionAsync(_echoApplication); + + Assert.Equal(0, _connection.StreamPool.Count); + + await StartStreamAsync(1, _browserRequestHeaders, endStream: false); + await StartStreamAsync(3, _browserRequestHeaders, endStream: false); + + await SendDataAsync(1, _helloBytes, endStream: true); + + await ExpectAsync(Http2FrameType.HEADERS, + withLength: 37, + withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS, + withStreamId: 1); + await ExpectAsync(Http2FrameType.DATA, + withLength: 5, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 1); + await ExpectAsync(Http2FrameType.DATA, + withLength: 0, + withFlags: (byte)Http2DataFrameFlags.END_STREAM, + withStreamId: 1); + + await SendDataAsync(3, _helloBytes, endStream: true); + + await ExpectAsync(Http2FrameType.HEADERS, + withLength: 37, + withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS, + withStreamId: 3); + await ExpectAsync(Http2FrameType.DATA, + withLength: 5, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 3); + await ExpectAsync(Http2FrameType.DATA, + withLength: 0, + withFlags: (byte)Http2DataFrameFlags.END_STREAM, + withStreamId: 3); + + // 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); + + // Streams have been returned to the pool + Assert.Equal(2, _connection.StreamPool.Count); + + await StopConnectionAsync(expectedLastStreamId: 3, ignoreNonGoAwayFrames: false); + } + + [Fact] + public async Task StreamPool_MultipleStreamsInSequence_PooledStreamReused() + { + await InitializeConnectionAsync(_echoApplication); + + Assert.Equal(0, _connection.StreamPool.Count); + + await StartStreamAsync(1, _browserRequestHeaders, endStream: true); + + await ExpectAsync(Http2FrameType.HEADERS, + withLength: 55, + 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); + + await StartStreamAsync(3, _browserRequestHeaders, endStream: true); + + await ExpectAsync(Http2FrameType.HEADERS, + withLength: 55, + withFlags: (byte)(Http2HeadersFrameFlags.END_HEADERS | Http2HeadersFrameFlags.END_STREAM), + withStreamId: 3); + + // New stream has been taken from the pool + Assert.Equal(0, _connection.StreamPool.Count); + + // 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 was reused and returned to the pool + Assert.Equal(1, _connection.StreamPool.Count); + + await StopConnectionAsync(expectedLastStreamId: 3, ignoreNonGoAwayFrames: false); + } + + [Fact] + public async Task StreamPool_EndedStreamErrorsOnStart_ReturnedToPool() + { + await InitializeConnectionAsync(_echoApplication); + + _connection.ServerSettings.MaxConcurrentStreams = 1; + + await StartStreamAsync(1, _browserRequestHeaders, endStream: false); + + // This stream will error because it exceeds max concurrent streams + await StartStreamAsync(3, _browserRequestHeaders, endStream: true); + await WaitForStreamErrorAsync(3, Http2ErrorCode.REFUSED_STREAM, CoreStrings.Http2ErrorMaxStreams); + + // 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 returned to the pool + Assert.Equal(1, _connection.StreamPool.Count); + + Assert.True(_connection.StreamPool.TryPop(out var stream)); + + // Stream has been completed and reset before being returned + Assert.Empty(stream.RequestHeaders); + + await StopConnectionAsync(expectedLastStreamId: 3, ignoreNonGoAwayFrames: false); + } + + [Fact] + public async Task StreamPool_UnendedStreamErrorsOnStart_ReturnedToPool() + { + _serviceContext.ServerOptions.Limits.MinRequestBodyDataRate = null; + + await InitializeConnectionAsync(_echoApplication); + + _connection.ServerSettings.MaxConcurrentStreams = 1; + + await StartStreamAsync(1, _browserRequestHeaders, endStream: false); + + // This stream will error because it exceeds max concurrent streams + await StartStreamAsync(3, _browserRequestHeaders, endStream: false); + await WaitForStreamErrorAsync(3, Http2ErrorCode.REFUSED_STREAM, CoreStrings.Http2ErrorMaxStreams); + + // 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); + + AdvanceClock(TimeSpan.FromTicks(Constants.RequestBodyDrainTimeout.Ticks + 1)); + + // Ping will trigger the stream to attempt to be returned to the pool + await SendPingAsync(Http2PingFrameFlags.NONE); + await ExpectAsync(Http2FrameType.PING, + withLength: 8, + withFlags: (byte)Http2PingFrameFlags.ACK, + withStreamId: 0); + + // Stream was returned to the pool because of the drain timeout + Assert.Equal(1, _connection.StreamPool.Count); + + await StopConnectionAsync(expectedLastStreamId: 3, ignoreNonGoAwayFrames: false); + + } + [Fact] public async Task Frame_Received_OverMaxSize_FrameError() {