diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs index 9a043b7036..2dded1c2d1 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs @@ -4,9 +4,11 @@ using System; using System.Buffers; using System.Collections.Concurrent; +using System.Collections.Generic; using System.Diagnostics; using System.IO; using System.IO.Pipelines; +using System.Runtime.CompilerServices; using System.Security.Authentication; using System.Text; using System.Threading; @@ -26,26 +28,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 { public class Http2Connection : IHttp2StreamLifetimeHandler, IHttpHeadersHandler, IRequestProcessor { - private enum RequestHeaderParsingState - { - Ready, - PseudoHeaderFields, - Headers, - Trailers - } - - [Flags] - private enum PseudoHeaderFields - { - None = 0x0, - Authority = 0x1, - Method = 0x2, - Path = 0x4, - Scheme = 0x8, - Status = 0x10, - Unknown = 0x40000000 - } - public static byte[] ClientPreface { get; } = Encoding.ASCII.GetBytes("PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n"); private static readonly PseudoHeaderFields _mandatoryRequestPseudoHeaderFields = @@ -78,15 +60,18 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 private Http2HeadersFrameFlags _headerFlags; private int _totalParsedHeaderSize; private bool _isMethodConnect; - private readonly object _stateLock = new object(); private int _highestOpenedStreamId; - private Http2ConnectionState _state = Http2ConnectionState.Open; - private readonly TaskCompletionSource _streamsCompleted = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + private bool _gracefulCloseStarted; - private readonly ConcurrentDictionary _streams = new ConcurrentDictionary(); - private readonly ConcurrentDictionary _drainingStreams = new ConcurrentDictionary(); + private readonly Dictionary _streams = new Dictionary(); private int _activeStreamCount = 0; + // The following are the only fields that can be modified outside of the ProcessRequestsAsync loop. + private readonly ConcurrentQueue _completedStreams = new ConcurrentQueue(); + private readonly StreamCloseAwaitable _streamCompletionAwaitable = new StreamCloseAwaitable(); + private int _gracefulCloseInitiator; + private int _isClosed; + public Http2Connection(HttpConnectionContext context) { var httpLimits = context.ServiceContext.ServerOptions.Limits; @@ -120,6 +105,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 public PipeReader Input => _context.Transport.Input; public IKestrelTrace Log => _context.ServiceContext.Log; public IFeatureCollection ConnectionFeatures => _context.ConnectionFeatures; + public ISystemClock SystemClock => _context.ServiceContext.SystemClock; public ITimeoutControl TimeoutControl => _context.TimeoutControl; public KestrelServerLimits Limits => _context.ServiceContext.ServerOptions.Limits; @@ -127,33 +113,22 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 public void OnInputOrOutputCompleted() { - lock (_stateLock) - { - if (_state != Http2ConnectionState.Closed) - { - UpdateState(Http2ConnectionState.Closed); - } - } - + TryClose(); _frameWriter.Abort(new ConnectionAbortedException(CoreStrings.ConnectionAbortedByClient)); } public void Abort(ConnectionAbortedException ex) { - lock (_stateLock) + if (TryClose()) { - if (_state != Http2ConnectionState.Closed) - { - _frameWriter.WriteGoAwayAsync(_highestOpenedStreamId, Http2ErrorCode.INTERNAL_ERROR); - UpdateState(Http2ConnectionState.Closed); - } + _frameWriter.WriteGoAwayAsync(int.MaxValue, Http2ErrorCode.INTERNAL_ERROR); } _frameWriter.Abort(ex); } public void StopProcessingNextRequest() - => StopProcessingNextRequest(true); + => StopProcessingNextRequest(serverInitiated: true); public void HandleRequestHeadersTimeout() { @@ -167,30 +142,13 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 Abort(new ConnectionAbortedException(CoreStrings.BadRequest_RequestBodyTimeout)); } - public void StopProcessingNextRequest(bool sendGracefulGoAway = false) + public void StopProcessingNextRequest(bool serverInitiated) { - lock (_stateLock) + var initiator = serverInitiated ? GracefulCloseInitiator.Server : GracefulCloseInitiator.Client; + + if (Interlocked.CompareExchange(ref _gracefulCloseInitiator, initiator, GracefulCloseInitiator.None) == GracefulCloseInitiator.None) { - if (_state == Http2ConnectionState.Open) - { - if (_activeStreamCount == 0) - { - _frameWriter.WriteGoAwayAsync(_highestOpenedStreamId, Http2ErrorCode.NO_ERROR); - UpdateState(Http2ConnectionState.Closed); - - // Wake up request processing loop so the connection can complete if there are no pending requests - Input.CancelPendingRead(); - } - else - { - if (sendGracefulGoAway) - { - _frameWriter.WriteGoAwayAsync(Int32.MaxValue, Http2ErrorCode.NO_ERROR); - } - - UpdateState(Http2ConnectionState.Closing); - } - } + Input.CancelPendingRead(); } } @@ -211,7 +169,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 return; } - if (_state != Http2ConnectionState.Closed) + if (_isClosed == 0) { await _frameWriter.WriteSettingsAsync(_serverSettings.GetNonProtocolDefaults()); // Inform the client that the connection window is larger than the default. It can't be lowered here, @@ -224,13 +182,16 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 } } - while (_state != Http2ConnectionState.Closed) + while (_isClosed == 0) { var result = await Input.ReadAsync(); var readableBuffer = result.Buffer; var consumed = readableBuffer.Start; var examined = readableBuffer.Start; + // Call UpdateCompletedStreams() prior to frame processing in order to remove any streams that have exceded their drain timeouts. + UpdateCompletedStreams(); + try { if (!readableBuffer.IsEmpty) @@ -262,6 +223,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 finally { Input.AdvanceTo(consumed, examined); + + UpdateConnectionState(); } } } @@ -305,18 +268,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 try { - lock (_stateLock) + if (TryClose()) { - if (_state != Http2ConnectionState.Closed) - { - _frameWriter.WriteGoAwayAsync(_highestOpenedStreamId, errorCode); - UpdateState(Http2ConnectionState.Closed); - } - - if (_activeStreamCount == 0) - { - _streamsCompleted.TrySetResult(null); - } + await _frameWriter.WriteGoAwayAsync(_highestOpenedStreamId, errorCode); } // Ensure aborting each stream doesn't result in unnecessary WINDOW_UPDATE frames being sent. @@ -327,8 +281,14 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 stream.Abort(new IOException(CoreStrings.Http2StreamAborted, connectionError)); } - await _streamsCompleted.Task; + while (_activeStreamCount > 0) + { + await _streamCompletionAwaitable; + UpdateCompletedStreams(); + } + // This cancels keep-alive and request header timeouts, but not the response drain timeout. + TimeoutControl.CancelTimeout(); TimeoutControl.StartDrainTimeout(Limits.MinResponseDataRate, Limits.MaxResponseBufferSize); _frameWriter.Complete(); @@ -364,7 +324,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 private async Task TryReadPrefaceAsync() { - while (_state != Http2ConnectionState.Closed) + while (_isClosed == 0) { var result = await Input.ReadAsync(); var readableBuffer = result.Buffer; @@ -389,6 +349,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 finally { Input.AdvanceTo(consumed, examined); + + UpdateConnectionState(); } } @@ -498,12 +460,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 throw new Http2ConnectionErrorException(CoreStrings.FormatHttp2ErrorStreamHalfClosedRemote(_incomingFrame.Type, stream.StreamId), Http2ErrorCode.STREAM_CLOSED); } - if (_incomingFrame.DataEndStream && stream.IsDraining) - { - // No more frames expected. - RemoveDrainingStream(_incomingFrame.StreamId); - } - return stream.OnDataAsync(_incomingFrame, payload); } @@ -589,46 +545,42 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 } else { - // Cancel keep-alive timeout and start header timeout if necessary. The keep-alive timeout can be - // started on another thread so the lock is necessary. - lock (_stateLock) + // Cancel keep-alive timeout and start header timeout if necessary. + if (TimeoutControl.TimerReason != TimeoutReason.None) { - if (TimeoutControl.TimerReason != TimeoutReason.None) - { - Debug.Assert(TimeoutControl.TimerReason == TimeoutReason.KeepAlive, "Non keep-alive timeout set at start of stream."); - TimeoutControl.CancelTimeout(); - } - - if (!_incomingFrame.HeadersEndHeaders) - { - TimeoutControl.SetTimeout(Limits.RequestHeadersTimeout.Ticks, TimeoutReason.RequestHeaders); - } - - // Start a new stream - _currentHeadersStream = new Http2Stream(new Http2StreamContext - { - ConnectionId = ConnectionId, - StreamId = _incomingFrame.StreamId, - ServiceContext = _context.ServiceContext, - ConnectionFeatures = _context.ConnectionFeatures, - MemoryPool = _context.MemoryPool, - LocalEndPoint = _context.LocalEndPoint, - RemoteEndPoint = _context.RemoteEndPoint, - StreamLifetimeHandler = this, - ClientPeerSettings = _clientSettings, - ServerPeerSettings = _serverSettings, - FrameWriter = _frameWriter, - ConnectionInputFlowControl = _inputFlowControl, - ConnectionOutputFlowControl = _outputFlowControl, - TimeoutControl = TimeoutControl, - }); - - _currentHeadersStream.Reset(); - _headerFlags = _incomingFrame.HeadersFlags; - - var headersPayload = payload.Slice(0, _incomingFrame.HeadersPayloadLength); // Minus padding - return DecodeHeadersAsync(application, _incomingFrame.HeadersEndHeaders, headersPayload); + Debug.Assert(TimeoutControl.TimerReason == TimeoutReason.KeepAlive, "Non keep-alive timeout set at start of stream."); + TimeoutControl.CancelTimeout(); } + + if (!_incomingFrame.HeadersEndHeaders) + { + TimeoutControl.SetTimeout(Limits.RequestHeadersTimeout.Ticks, TimeoutReason.RequestHeaders); + } + + // Start a new stream + _currentHeadersStream = new Http2Stream(new Http2StreamContext + { + ConnectionId = ConnectionId, + StreamId = _incomingFrame.StreamId, + ServiceContext = _context.ServiceContext, + ConnectionFeatures = _context.ConnectionFeatures, + MemoryPool = _context.MemoryPool, + LocalEndPoint = _context.LocalEndPoint, + RemoteEndPoint = _context.RemoteEndPoint, + StreamLifetimeHandler = this, + ClientPeerSettings = _clientSettings, + ServerPeerSettings = _serverSettings, + FrameWriter = _frameWriter, + ConnectionInputFlowControl = _inputFlowControl, + ConnectionOutputFlowControl = _outputFlowControl, + TimeoutControl = TimeoutControl, + }); + + _currentHeadersStream.Reset(); + _headerFlags = _incomingFrame.HeadersFlags; + + var headersPayload = payload.Slice(0, _incomingFrame.HeadersPayloadLength); // Minus padding + return DecodeHeadersAsync(application, _incomingFrame.HeadersEndHeaders, headersPayload); } } @@ -685,16 +637,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 throw new Http2ConnectionErrorException(CoreStrings.FormatHttp2ErrorStreamAborted(_incomingFrame.Type, stream.StreamId), Http2ErrorCode.STREAM_CLOSED); } - if (stream.IsDraining) - { - // This stream was aborted by the server earlier and now the client is aborting it as well. No more frames are expected. - RemoveDrainingStream(_incomingFrame.StreamId); - } - else - { - // No additional inbound header or data frames are allowed for this stream after receiving a reset. - stream.AbortRstStreamReceived(); - } + // No additional inbound header or data frames are allowed for this stream after receiving a reset. + stream.AbortRstStreamReceived(); } return Task.CompletedTask; @@ -809,7 +753,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 throw new Http2ConnectionErrorException(CoreStrings.FormatHttp2ErrorStreamIdNotZero(_incomingFrame.Type), Http2ErrorCode.PROTOCOL_ERROR); } - StopProcessingNextRequest(sendGracefulGoAway: false); + StopProcessingNextRequest(serverInitiated: false); return Task.CompletedTask; } @@ -896,17 +840,14 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 } else { - lock (_stateLock) + Debug.Assert(TimeoutControl.TimerReason == TimeoutReason.RequestHeaders, "Received continuation frame without request header timeout being set."); + + if (_incomingFrame.HeadersEndHeaders) { - Debug.Assert(TimeoutControl.TimerReason == TimeoutReason.RequestHeaders, "Received continuation frame without request header timeout being set."); - - if (_incomingFrame.HeadersEndHeaders) - { - TimeoutControl.CancelTimeout(); - } - - return DecodeHeadersAsync(application, _incomingFrame.ContinuationEndHeaders, payload); + TimeoutControl.CancelTimeout(); } + + return DecodeHeadersAsync(application, _incomingFrame.ContinuationEndHeaders, payload); } } @@ -920,7 +861,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 return Task.CompletedTask; } - // This is always called with the _stateLock acquired. private Task DecodeHeadersAsync(IHttpApplication application, bool endHeaders, ReadOnlySequence payload) { try @@ -930,11 +870,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 if (endHeaders) { - if (_state != Http2ConnectionState.Closed) - { - StartStream(application); - } - + StartStream(application); ResetRequestHeaderParsingState(); } } @@ -953,16 +889,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 if (endHeaders) { - if (_currentHeadersStream.IsDraining) - { - // This stream is aborted and abandon, no action required - RemoveDrainingStream(_currentHeadersStream.StreamId); - } - else - { - _currentHeadersStream.OnEndStreamReceived(); - } - + _currentHeadersStream.OnEndStreamReceived(); ResetRequestHeaderParsingState(); } @@ -1041,64 +968,100 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 } } - void IHttp2StreamLifetimeHandler.OnStreamCompleted(int streamId) + void IRequestProcessor.Tick(DateTimeOffset now) { - lock (_stateLock) + Input.CancelPendingRead(); + } + + void IHttp2StreamLifetimeHandler.OnStreamCompleted(Http2Stream stream) + { + _completedStreams.Enqueue(stream); + _streamCompletionAwaitable.Complete(); + } + + private void UpdateCompletedStreams() + { + Http2Stream firstRequedStream = null; + var now = SystemClock.UtcNowTicks; + + while (_completedStreams.TryDequeue(out var stream)) { - _activeStreamCount--; - - // Get, Add, Remove so the steam is always registered in at least one collection at a time. - if (_streams.TryGetValue(streamId, out var stream)) + if (stream == firstRequedStream) { - if (stream.IsDraining) - { - stream.DrainExpirationTicks = - _context.ServiceContext.SystemClock.UtcNowTicks + Constants.RequestBodyDrainTimeout.Ticks; - - _drainingStreams.TryAdd(streamId, stream); - } - else - { - _streams.TryRemove(streamId, out _); - } + // We've checked every stream that was in _completedStreams by the time + // _checkCompletedStreams was unset, so exit the loop. + _completedStreams.Enqueue(stream); + break; } - if (_activeStreamCount == 0) + if (stream.DrainExpirationTicks == default) { - if (_state == Http2ConnectionState.Closing) - { - _frameWriter.WriteGoAwayAsync(_highestOpenedStreamId, Http2ErrorCode.NO_ERROR); - UpdateState(Http2ConnectionState.Closed); + // This is our first time checking this stream. + _activeStreamCount--; + stream.DrainExpirationTicks = now + Constants.RequestBodyDrainTimeout.Ticks; + } - // Wake up request processing loop so the connection can complete if there are no pending requests - Input.CancelPendingRead(); + if (stream.EndStreamReceived || stream.RstStreamReceived || stream.DrainExpirationTicks < now) + { + if (stream == _currentHeadersStream) + { + // The drain expired out while receiving trailers. The most recent incoming frame is either a header or continuation frame for the timed out stream. + throw new Http2ConnectionErrorException(CoreStrings.FormatHttp2ErrorStreamClosed(_incomingFrame.Type, _incomingFrame.StreamId), Http2ErrorCode.STREAM_CLOSED); } - if (_state == Http2ConnectionState.Open) + _streams.Remove(stream.StreamId); + } + else + { + if (firstRequedStream == null) { - // If we're awaiting headers, either a new stream will be started, or there will be a connection - // error possibly due to a request header timeout, so no need to start a keep-alive timeout. - if (TimeoutControl.TimerReason != TimeoutReason.RequestHeaders) - { - TimeoutControl.SetTimeout(Limits.KeepAliveTimeout.Ticks, TimeoutReason.KeepAlive); - } - } - else - { - // Complete the task waiting on all streams to finish - _streamsCompleted.TrySetResult(null); + firstRequedStream = stream; } + + _completedStreams.Enqueue(stream); } } } - void IRequestProcessor.Tick(DateTimeOffset now) + private void UpdateConnectionState() { - foreach (var stream in _drainingStreams) + if (_isClosed != 0) { - if (now.Ticks > stream.Value.DrainExpirationTicks) + return; + } + + if (_gracefulCloseInitiator != GracefulCloseInitiator.None && !_gracefulCloseStarted) + { + _gracefulCloseStarted = true; + + Log.Http2ConnectionClosing(_context.ConnectionId); + + if (_gracefulCloseInitiator == GracefulCloseInitiator.Server && _activeStreamCount > 0) { - RemoveDrainingStream(stream.Key); + _frameWriter.WriteGoAwayAsync(int.MaxValue, Http2ErrorCode.NO_ERROR); + } + } + + if (_activeStreamCount == 0) + { + if (_gracefulCloseStarted) + { + if (TryClose()) + { + _frameWriter.WriteGoAwayAsync(_highestOpenedStreamId, Http2ErrorCode.NO_ERROR); + } + } + else + { + if (TimeoutControl.TimerReason == TimeoutReason.None) + { + TimeoutControl.SetTimeout(Limits.KeepAliveTimeout.Ticks, TimeoutReason.KeepAlive); + } + + // If we're awaiting headers, either a new stream will be started, or there will be a connection + // error possibly due to a request header timeout, so no need to start a keep-alive timeout. + Debug.Assert(TimeoutControl.TimerReason == TimeoutReason.RequestHeaders || + TimeoutControl.TimerReason == TimeoutReason.KeepAlive); } } } @@ -1270,29 +1233,80 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 return name.SequenceEqual(_connectionBytes) || (name.SequenceEqual(_teBytes) && !value.SequenceEqual(_trailersBytes)); } - private void UpdateState(Http2ConnectionState state) + private bool TryClose() { - _state = state; - if (state == Http2ConnectionState.Closing) + if (Interlocked.Exchange(ref _isClosed, 1) == 0) { - Log.Http2ConnectionClosing(_context.ConnectionId); - } - else if (state == Http2ConnectionState.Closed) - { - // This cancels keep-alive and request header timeouts, but not the response drain timeout. - TimeoutControl.CancelTimeout(); Log.Http2ConnectionClosed(_context.ConnectionId, _highestOpenedStreamId); + return true; + } + + return false; + } + + private class StreamCloseAwaitable : ICriticalNotifyCompletion + { + private static readonly Action _callbackCompleted = () => { }; + + // Initialize to completed so UpdateCompletedStreams runs at least once during connection teardown + // if there are still active streams. + private Action _callback = _callbackCompleted; + + public StreamCloseAwaitable GetAwaiter() => this; + public bool IsCompleted => ReferenceEquals(_callback, _callbackCompleted); + + public void GetResult() + { + Debug.Assert(ReferenceEquals(_callback, _callbackCompleted)); + + _callback = null; + } + + public void OnCompleted(Action continuation) + { + if (ReferenceEquals(_callback, _callbackCompleted) || + ReferenceEquals(Interlocked.CompareExchange(ref _callback, continuation, null), _callbackCompleted)) + { + Task.Run(continuation); + } + } + + public void UnsafeOnCompleted(Action continuation) + { + OnCompleted(continuation); + } + + public void Complete() + { + Interlocked.Exchange(ref _callback, _callbackCompleted)?.Invoke(); } } - // Note this may be called concurrently based on incoming frames and Ticks. - private void RemoveDrainingStream(int key) + private enum RequestHeaderParsingState { - _streams.TryRemove(key, out _); - // It's possible to be marked as draining and have RemoveDrainingStream called - // before being added to the draining collection. In that case the next Tick would - // remove it anyways. - _drainingStreams.TryRemove(key, out _); + Ready, + PseudoHeaderFields, + Headers, + Trailers + } + + [Flags] + private enum PseudoHeaderFields + { + None = 0x0, + Authority = 0x1, + Method = 0x2, + Path = 0x4, + Scheme = 0x8, + Status = 0x10, + Unknown = 0x40000000 + } + + private static class GracefulCloseInitiator + { + public const int None = 0; + public const int Server = 1; + public const int Client = 2; } } } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2ConnectionState.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2ConnectionState.cs deleted file mode 100644 index cb1518807d..0000000000 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2ConnectionState.cs +++ /dev/null @@ -1,12 +0,0 @@ -// 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. - -namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 -{ - public enum Http2ConnectionState - { - Open = 0, - Closing, - Closed - } -} diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs index e8e3ccaf0c..2f67445b27 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs @@ -529,6 +529,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 { lock (_writeLock) { + if (_completed) + { + return Task.CompletedTask; + } + _outgoingFrame.PrepareGoAway(lastStreamId, errorCode); WriteHeaderUnsynchronized(); diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs index da9bbc9dfe..c51dfcf84c 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs @@ -66,7 +66,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 public bool EndStreamReceived => (_completionState & StreamCompletionFlags.EndStreamReceived) == StreamCompletionFlags.EndStreamReceived; private bool IsAborted => (_completionState & StreamCompletionFlags.Aborted) == StreamCompletionFlags.Aborted; internal bool RstStreamReceived => (_completionState & StreamCompletionFlags.RstStreamReceived) == StreamCompletionFlags.RstStreamReceived; - internal bool IsDraining => (_completionState & StreamCompletionFlags.Draining) == StreamCompletionFlags.Draining; public bool ReceivedEmptyRequestBody { @@ -94,8 +93,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 { Log.RequestBodyNotEntirelyRead(ConnectionIdFeature, TraceIdentifier); - ApplyCompletionFlag(StreamCompletionFlags.Draining); - var states = ApplyCompletionFlag(StreamCompletionFlags.Aborted); if (states.OldState != states.NewState) { @@ -117,7 +114,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 } finally { - _context.StreamLifetimeHandler.OnStreamCompleted(StreamId); + _context.StreamLifetimeHandler.OnStreamCompleted(this); } } @@ -509,7 +506,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 RstStreamReceived = 1, EndStreamReceived = 2, Aborted = 4, - Draining = 8, } } } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/IHttp2StreamLifetimeHandler.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/IHttp2StreamLifetimeHandler.cs index fcb9c89637..b81b6ca5aa 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/IHttp2StreamLifetimeHandler.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/IHttp2StreamLifetimeHandler.cs @@ -5,6 +5,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 { public interface IHttp2StreamLifetimeHandler { - void OnStreamCompleted(int streamId); + void OnStreamCompleted(Http2Stream stream); } } diff --git a/src/Servers/Kestrel/Core/src/Internal/HttpConnection.cs b/src/Servers/Kestrel/Core/src/Internal/HttpConnection.cs index 1fdd228fc7..63a926585a 100644 --- a/src/Servers/Kestrel/Core/src/Internal/HttpConnection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/HttpConnection.cs @@ -1,4 +1,4 @@ -// Copyright (c) .NET Foundation. All rights reserved. +// 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 System; diff --git a/src/Servers/Kestrel/Core/src/Internal/IRequestProcessor.cs b/src/Servers/Kestrel/Core/src/Internal/IRequestProcessor.cs index 7e8e12a964..9f886de64d 100644 --- a/src/Servers/Kestrel/Core/src/Internal/IRequestProcessor.cs +++ b/src/Servers/Kestrel/Core/src/Internal/IRequestProcessor.cs @@ -1,4 +1,4 @@ -// Copyright (c) .NET Foundation. All rights reserved. +// 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 System; diff --git a/src/Servers/Kestrel/test/FunctionalTests/Http2/ShutdownTests.cs b/src/Servers/Kestrel/test/FunctionalTests/Http2/ShutdownTests.cs index cb7c5a1ee4..230931cd77 100644 --- a/src/Servers/Kestrel/test/FunctionalTests/Http2/ShutdownTests.cs +++ b/src/Servers/Kestrel/test/FunctionalTests/Http2/ShutdownTests.cs @@ -56,13 +56,17 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests.Http2 .Setup(m => m.Http2ConnectionClosing(It.IsAny())) .Callback(() => requestStopping.SetResult(null)); + var testContext = new TestServiceContext(LoggerFactory, mockKestrelTrace.Object); + + testContext.InitializeHeartbeat(); + using (var server = new TestServer(async context => { requestStarted.SetResult(null); await requestUnblocked.Task.DefaultTimeout(); await context.Response.WriteAsync("hello world " + context.Request.Protocol); }, - new TestServiceContext(LoggerFactory, mockKestrelTrace.Object), + testContext, kestrelOptions => { kestrelOptions.Listen(IPAddress.Loopback, 0, listenOptions => diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs index 08236ac973..8d3c91b8f2 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs @@ -2678,6 +2678,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests { await InitializeConnectionAsync(_echoApplication); + StartHeartbeat(); + // Start some streams await StartStreamAsync(1, _browserRequestHeaders, endStream: false); await StartStreamAsync(3, _browserRequestHeaders, endStream: false); @@ -3559,7 +3561,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests _connection.Abort(new ConnectionAbortedException()); await _closedStateReached.Task.DefaultTimeout(); - VerifyGoAway(await ReceiveFrameAsync(), 0, Http2ErrorCode.INTERNAL_ERROR); + VerifyGoAway(await ReceiveFrameAsync(), int.MaxValue, Http2ErrorCode.INTERNAL_ERROR); } [Fact] @@ -3627,6 +3629,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests { await InitializeConnectionAsync(_echoApplication); + StartHeartbeat(); + await StartStreamAsync(1, _browserRequestHeaders, endStream: false); _connection.StopProcessingNextRequest(); @@ -3657,6 +3661,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests { await InitializeConnectionAsync(_echoApplication); + StartHeartbeat(); + await StartStreamAsync(1, _browserRequestHeaders, endStream: false); _connection.StopProcessingNextRequest(); diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs index 8375561f9b..9225795d1c 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs @@ -1,4 +1,4 @@ -// Copyright (c) .NET Foundation. All rights reserved. +// 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 System; @@ -1567,7 +1567,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests withFlags: (byte)Http2DataFrameFlags.NONE, withStreamId: 0); - VerifyGoAway(goAway, 1, Http2ErrorCode.INTERNAL_ERROR); + VerifyGoAway(goAway, int.MaxValue, Http2ErrorCode.INTERNAL_ERROR); _pair.Application.Output.Complete(); await _connectionTask; @@ -1995,7 +1995,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests _pair.Application.Output.Complete(); - await WaitForConnectionErrorAsync(ignoreNonGoAwayFrames: false, expectedLastStreamId: 1, Http2ErrorCode.INTERNAL_ERROR, + await WaitForConnectionErrorAsync(ignoreNonGoAwayFrames: false, expectedLastStreamId: int.MaxValue, Http2ErrorCode.INTERNAL_ERROR, CoreStrings.HPackErrorNotEnoughBuffer); } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2TestBase.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2TestBase.cs index c6c1fe7075..41a451eb76 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2TestBase.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2TestBase.cs @@ -1,4 +1,4 @@ -// Copyright (c) .NET Foundation. All rights reserved. +// 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 System; @@ -152,6 +152,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests protected readonly RequestDelegate _appAbort; protected TestServiceContext _serviceContext; + private Timer _timer; internal DuplexPipe.DuplexPipePair _pair; protected Http2Connection _connection; @@ -385,6 +386,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests public override void Dispose() { + _timer?.Dispose(); _pair.Application?.Input.Complete(); _pair.Application?.Output.Complete(); _pair.Transport?.Input.Complete(); @@ -457,6 +459,19 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests withStreamId: 0); } + protected void StartHeartbeat() + { + if (_timer == null) + { + _timer = new Timer(OnHeartbeat, state: this, dueTime: Heartbeat.Interval, period: Heartbeat.Interval); + } + } + + private static void OnHeartbeat(object state) + { + ((IRequestProcessor)((Http2TestBase)state)._connection)?.Tick(default); + } + protected Task StartStreamAsync(int streamId, IEnumerable> headers, bool endStream) { var writableBuffer = _pair.Application.Output; diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2TimeoutTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2TimeoutTests.cs index 18a8a7b698..5168fcf6f6 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2TimeoutTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2TimeoutTests.cs @@ -1,4 +1,4 @@ -// Copyright (c) .NET Foundation. All rights reserved. +// 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 System; @@ -77,6 +77,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests await InitializeConnectionAsync(_noopApplication); + StartHeartbeat(); + AdvanceClock(limits.KeepAliveTimeout + Heartbeat.Interval); // keep-alive timeout set but not fired. @@ -148,7 +150,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests await WaitForConnectionErrorAsync( ignoreNonGoAwayFrames: false, - expectedLastStreamId: 1, + expectedLastStreamId: int.MaxValue, Http2ErrorCode.INTERNAL_ERROR, CoreStrings.BadRequest_RequestHeadersTimeout); @@ -192,7 +194,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests [Theory] [InlineData(Http2FrameType.DATA)] - [InlineData(Http2FrameType.CONTINUATION, Skip = "https://github.com/aspnet/KestrelHttpServer/issues/3077")] + [InlineData(Http2FrameType.CONTINUATION)] public async Task AbortedStream_ResetsAndDrainsRequest_RefusesFramesAfterCooldownExpires(Http2FrameType finalFrameType) { var mockSystemClock = _serviceContext.MockSystemClock; @@ -216,6 +218,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests if (finalFrameType == Http2FrameType.CONTINUATION) { await SendHeadersAsync(1, Http2HeadersFrameFlags.END_STREAM, new byte[0]); + await SendContinuationAsync(1, Http2ContinuationFrameFlags.NONE, new byte[0]); } // There's a race when the appfunc is exiting about how soon it unregisters the stream, so retry until success. @@ -223,7 +226,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests { // Just past the timeout mockSystemClock.UtcNow += Constants.RequestBodyDrainTimeout + TimeSpan.FromTicks(1); - (_connection as IRequestProcessor).Tick(mockSystemClock.UtcNow); // Send an extra frame to make it fail switch (finalFrameType) @@ -305,7 +307,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests await WaitForConnectionErrorAsync( ignoreNonGoAwayFrames: false, - expectedLastStreamId: 1, + expectedLastStreamId: int.MaxValue, Http2ErrorCode.INTERNAL_ERROR, null); @@ -363,7 +365,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests await WaitForConnectionErrorAsync( ignoreNonGoAwayFrames: false, - expectedLastStreamId: 1, + expectedLastStreamId: int.MaxValue, Http2ErrorCode.INTERNAL_ERROR, null); @@ -417,7 +419,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests await WaitForConnectionErrorAsync( ignoreNonGoAwayFrames: false, - expectedLastStreamId: 1, + expectedLastStreamId: int.MaxValue, Http2ErrorCode.INTERNAL_ERROR, null); @@ -473,7 +475,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests await WaitForConnectionErrorAsync( ignoreNonGoAwayFrames: false, - expectedLastStreamId: 1, + expectedLastStreamId: int.MaxValue, Http2ErrorCode.INTERNAL_ERROR, null); @@ -541,7 +543,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests await WaitForConnectionErrorAsync( ignoreNonGoAwayFrames: false, - expectedLastStreamId: 3, + expectedLastStreamId: int.MaxValue, Http2ErrorCode.INTERNAL_ERROR, null); @@ -590,7 +592,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests await WaitForConnectionErrorAsync( ignoreNonGoAwayFrames: false, - expectedLastStreamId: 1, + expectedLastStreamId: int.MaxValue, Http2ErrorCode.INTERNAL_ERROR, null); @@ -643,7 +645,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests await WaitForConnectionErrorAsync( ignoreNonGoAwayFrames: false, - expectedLastStreamId: 1, + expectedLastStreamId: int.MaxValue, Http2ErrorCode.INTERNAL_ERROR, null); @@ -712,7 +714,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests await WaitForConnectionErrorAsync( ignoreNonGoAwayFrames: false, - expectedLastStreamId: 3, + expectedLastStreamId: int.MaxValue, Http2ErrorCode.INTERNAL_ERROR, null); @@ -782,7 +784,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests await WaitForConnectionErrorAsync( ignoreNonGoAwayFrames: false, - expectedLastStreamId: 3, + expectedLastStreamId: int.MaxValue, Http2ErrorCode.INTERNAL_ERROR, null); @@ -877,7 +879,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests await WaitForConnectionErrorAsync( ignoreNonGoAwayFrames: false, - expectedLastStreamId: 3, + expectedLastStreamId: int.MaxValue, Http2ErrorCode.INTERNAL_ERROR, null);