From 7d9f12a23afcc1bc13c34ea7dd3733559452de19 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Tue, 2 Aug 2022 06:48:40 +0800 Subject: [PATCH 1/5] HTTP/3: Support QUIC idle timeout more cleanly --- .../src/Internal/Http3/Http3ControlStream.cs | 4 +-- .../Transport.Quic/src/Internal/QuicLog.cs | 27 +++++++++++++++++-- .../src/Internal/QuicStreamContext.cs | 26 +++++++----------- 3 files changed, 37 insertions(+), 20 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs index adf2080c06b1..49650ec9061d 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs @@ -59,8 +59,8 @@ public Http3ControlStream(Http3StreamContext context, long? headerType) private void OnStreamClosed() { - Abort(new ConnectionAbortedException("HTTP_CLOSED_CRITICAL_STREAM"), Http3ErrorCode.InternalError); - _connectionClosed = true; + //Abort(new ConnectionAbortedException("HTTP_CLOSED_CRITICAL_STREAM"), Http3ErrorCode.ClosedCriticalStream); + //_connectionClosed = true; } public PipeReader Input => _context.Transport.Input; diff --git a/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicLog.cs b/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicLog.cs index 48d21d37ca88..eef6e0ee74a6 100644 --- a/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicLog.cs +++ b/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicLog.cs @@ -120,7 +120,7 @@ public static void StreamShutdownWrite(ILogger logger, QuicStreamContext streamC } } - [LoggerMessage(11, LogLevel.Debug, @"Stream id ""{ConnectionId}"" read aborted by peer with error code {ErrorCode}.", EventName = "StreamAborted", SkipEnabledCheck = true)] + [LoggerMessage(11, LogLevel.Debug, @"Stream id ""{ConnectionId}"" read aborted by peer with error code {ErrorCode}.", EventName = "StreamAbortedRead", SkipEnabledCheck = true)] private static partial void StreamAbortedReadCore(ILogger logger, string connectionId, long errorCode); public static void StreamAbortedRead(ILogger logger, QuicStreamContext streamContext, long errorCode) @@ -131,7 +131,7 @@ public static void StreamAbortedRead(ILogger logger, QuicStreamContext streamCon } } - [LoggerMessage(12, LogLevel.Debug, @"Stream id ""{ConnectionId}"" write aborted by peer with error code {ErrorCode}.", EventName = "StreamAborted", SkipEnabledCheck = true)] + [LoggerMessage(12, LogLevel.Debug, @"Stream id ""{ConnectionId}"" write aborted by peer with error code {ErrorCode}.", EventName = "StreamAbortedWrite", SkipEnabledCheck = true)] private static partial void StreamAbortedWriteCore(ILogger logger, string connectionId, long errorCode); public static void StreamAbortedWrite(ILogger logger, QuicStreamContext streamContext, long errorCode) @@ -210,6 +210,29 @@ public static void StreamReused(ILogger logger, QuicStreamContext streamContext) [LoggerMessage(21, LogLevel.Debug, "QUIC listener aborted.", EventName = "ConnectionListenerAborted")] public static partial void ConnectionListenerAborted(ILogger logger, Exception exception); + + [LoggerMessage(22, LogLevel.Debug, @"Stream id ""{ConnectionId}"" read timed out.", EventName = "StreamTimeoutRead", SkipEnabledCheck = true)] + private static partial void StreamTimeoutReadCore(ILogger logger, string connectionId); + + public static void StreamTimeoutRead(ILogger logger, QuicStreamContext streamContext) + { + if (logger.IsEnabled(LogLevel.Debug)) + { + StreamTimeoutReadCore(logger, streamContext.ConnectionId); + } + } + + [LoggerMessage(23, LogLevel.Debug, @"Stream id ""{ConnectionId}"" write timed out.", EventName = "StreamTimeoutWrite", SkipEnabledCheck = true)] + private static partial void StreamTimeoutWriteCore(ILogger logger, string connectionId); + + public static void StreamTimeoutWrite(ILogger logger, QuicStreamContext streamContext) + { + if (logger.IsEnabled(LogLevel.Debug)) + { + StreamTimeoutWriteCore(logger, streamContext.ConnectionId); + } + } + private static StreamType GetStreamType(QuicStreamContext streamContext) => streamContext.CanRead && streamContext.CanWrite ? StreamType.Bidirectional diff --git a/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs b/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs index b1776ad06aa2..9a95793a59de 100644 --- a/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs +++ b/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs @@ -270,7 +270,7 @@ private async ValueTask DoReceiveAsync() } } } - catch (QuicException ex) when (ex.QuicError == QuicError.StreamAborted) + catch (QuicException ex) when (ex.QuicError is QuicError.StreamAborted or QuicError.ConnectionAborted) { // Abort from peer. _error = ex.ApplicationErrorCode; @@ -281,18 +281,15 @@ private async ValueTask DoReceiveAsync() _clientAbort = true; } - catch (QuicException ex) when (ex.QuicError == QuicError.ConnectionAborted) + catch (QuicException ex) when (ex.QuicError is QuicError.ConnectionIdle) { - // Abort from peer. - _error = ex.ApplicationErrorCode; - QuicLog.StreamAbortedRead(_log, this, ex.ApplicationErrorCode.GetValueOrDefault()); + // Abort from timeout. + QuicLog.StreamTimeoutRead(_log, this); // This could be ignored if _shutdownReason is already set. error = new ConnectionResetException(ex.Message, ex); - - _clientAbort = true; } - catch (QuicException ex) when (ex.QuicError == QuicError.OperationAborted) + catch (QuicException ex) when (ex.QuicError is QuicError.OperationAborted) { // AbortRead has been called for the stream. error = new ConnectionAbortedException(ex.Message, ex); @@ -434,7 +431,7 @@ private async ValueTask DoSendAsync() } } } - catch (QuicException ex) when (ex.QuicError == QuicError.StreamAborted) + catch (QuicException ex) when (ex.QuicError is QuicError.StreamAborted or QuicError.ConnectionAborted) { // Abort from peer. _error = ex.ApplicationErrorCode; @@ -445,18 +442,15 @@ private async ValueTask DoSendAsync() _clientAbort = true; } - catch (QuicException ex) when (ex.QuicError == QuicError.ConnectionAborted) + catch (QuicException ex) when (ex.QuicError is QuicError.ConnectionIdle) { - // Abort from peer. - _error = ex.ApplicationErrorCode; - QuicLog.StreamAbortedWrite(_log, this, ex.ApplicationErrorCode.GetValueOrDefault()); + // Abort from timeout. + QuicLog.StreamTimeoutWrite(_log, this); // This could be ignored if _shutdownReason is already set. shutdownReason = new ConnectionResetException(ex.Message, ex); - - _clientAbort = true; } - catch (QuicException ex) when (ex.QuicError == QuicError.OperationAborted) + catch (QuicException ex) when (ex.QuicError is QuicError.OperationAborted) { // AbortWrite has been called for the stream. // Possibily might also get here from connection closing. From b82d413546b0eba0303528b925a3be2341d47e14 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Thu, 11 Aug 2022 07:01:31 +0800 Subject: [PATCH 2/5] WIP --- .../Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs | 3 +++ src/Servers/Kestrel/Transport.Quic/src/Internal/QuicLog.cs | 1 - 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs index 49650ec9061d..7c412058fbb9 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs @@ -59,6 +59,9 @@ public Http3ControlStream(Http3StreamContext context, long? headerType) private void OnStreamClosed() { + _connectionClosed = true; + _connectionClosed = false; + //Abort(new ConnectionAbortedException("HTTP_CLOSED_CRITICAL_STREAM"), Http3ErrorCode.ClosedCriticalStream); //_connectionClosed = true; } diff --git a/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicLog.cs b/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicLog.cs index eef6e0ee74a6..d58061344f24 100644 --- a/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicLog.cs +++ b/src/Servers/Kestrel/Transport.Quic/src/Internal/QuicLog.cs @@ -210,7 +210,6 @@ public static void StreamReused(ILogger logger, QuicStreamContext streamContext) [LoggerMessage(21, LogLevel.Debug, "QUIC listener aborted.", EventName = "ConnectionListenerAborted")] public static partial void ConnectionListenerAborted(ILogger logger, Exception exception); - [LoggerMessage(22, LogLevel.Debug, @"Stream id ""{ConnectionId}"" read timed out.", EventName = "StreamTimeoutRead", SkipEnabledCheck = true)] private static partial void StreamTimeoutReadCore(ILogger logger, string connectionId); From dd7f37abe44304eeea08d4d0d735a0ef0d800da5 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Thu, 11 Aug 2022 21:35:18 +0800 Subject: [PATCH 3/5] Delay aborting connection with critical stream error --- .../src/Internal/Http3/Http3Connection.cs | 42 ++++++++++++++ .../src/Internal/Http3/Http3ControlStream.cs | 55 +++++++++++++------ .../Core/src/Internal/Http3/Http3Stream.cs | 14 +---- .../Core/src/Internal/Http3/IHttp3Stream.cs | 4 ++ .../Internal/Http3/StreamCompletionFlags.cs | 14 +++++ .../Http3/Http3ConnectionTests.cs | 8 +++ 6 files changed, 107 insertions(+), 30 deletions(-) create mode 100644 src/Servers/Kestrel/Core/src/Internal/Http3/StreamCompletionFlags.cs diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs index 4993ea7f1264..fd1b499ff3e0 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs @@ -198,9 +198,51 @@ public void Tick(DateTimeOffset now) return; } + ValidateOpenControlStreams(now); UpdateStreamTimeouts(now); } + private void ValidateOpenControlStreams(DateTimeOffset now) + { + var ticks = now.Ticks; + + // This method validates that a connnection's control streams are open. + // + // They're checked on a delayed timer because when a connection is aborted or timed out, notifications are sent to open streams + // and the connection simultaneously. This is a problem because when a control stream is closed the connection should be aborted + // with the H3_CLOSED_CRITICAL_STREAM status. There is a race between the connection closing for the real reason, and control + // streams closing the connection with H3_CLOSED_CRITICAL_STREAM. + // + // Realistically, control streams are never closed except when the connection is. A small delay in aborting the connection in the + // unlikely situation where a control stream is incorrectly closed should be fine. + ValidateOpenControlStream(OutboundControlStream, this, ticks); + ValidateOpenControlStream(ControlStream, this, ticks); + ValidateOpenControlStream(EncoderStream, this, ticks); + ValidateOpenControlStream(DecoderStream, this, ticks); + + static void ValidateOpenControlStream(Http3ControlStream? stream, Http3Connection connection, long ticks) + { + if (stream != null) + { + if (stream.IsCompleted || stream.IsAborted || stream.EndStreamReceived) + { + // If a control stream is no longer active then set a timeout so that the connection is aborted next tick. + if (stream.StreamTimeoutTicks == default) + { + // On overflow, use max value. + var connectionCloseTicks = ticks + (TimeSpan.TicksPerSecond / 2); + stream.StreamTimeoutTicks = connectionCloseTicks >= 0 ? connectionCloseTicks : long.MaxValue; + } + + if (stream.StreamTimeoutTicks < ticks) + { + connection.Abort(new ConnectionAbortedException("A control stream used by the connection was closed or reset."), Http3ErrorCode.ClosedCriticalStream); + } + } + } + } + } + private void UpdateStreamTimeouts(DateTimeOffset now) { // This method checks for timeouts: diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs index 7c412058fbb9..acc5124a42d1 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs @@ -29,9 +29,14 @@ internal abstract class Http3ControlStream : IHttp3Stream, IThreadPoolWorkItem private volatile int _isClosed; private long _headerType; private int _gracefulCloseInitiator; - private bool _connectionClosed; + private readonly object _completionLock = new(); private bool _haveReceivedSettingsFrame; + private StreamCompletionFlags _completionState; + + public bool EndStreamReceived => (_completionState & StreamCompletionFlags.EndStreamReceived) == StreamCompletionFlags.EndStreamReceived; + public bool IsAborted => (_completionState & StreamCompletionFlags.Aborted) == StreamCompletionFlags.Aborted; + public bool IsCompleted => (_completionState & StreamCompletionFlags.Completed) == StreamCompletionFlags.Completed; public long StreamId => _streamIdFeature.StreamId; @@ -59,11 +64,7 @@ public Http3ControlStream(Http3StreamContext context, long? headerType) private void OnStreamClosed() { - _connectionClosed = true; - _connectionClosed = false; - - //Abort(new ConnectionAbortedException("HTTP_CLOSED_CRITICAL_STREAM"), Http3ErrorCode.ClosedCriticalStream); - //_connectionClosed = true; + ApplyCompletionFlag(StreamCompletionFlags.Completed); } public PipeReader Input => _context.Transport.Input; @@ -77,15 +78,27 @@ private void OnStreamClosed() public void Abort(ConnectionAbortedException abortReason, Http3ErrorCode errorCode) { - // TODO - Should there be a check here to track abort state to avoid - // running twice for a request? + lock (_completionLock) + { + if (IsCompleted || IsAborted) + { + return; + } - Log.Http3StreamAbort(_context.ConnectionId, errorCode, abortReason); + var (oldState, newState) = ApplyCompletionFlag(StreamCompletionFlags.Aborted); - _errorCodeFeature.Error = (long)errorCode; - _frameWriter.Abort(abortReason); + if (oldState == newState) + { + return; + } + + Log.Http3StreamAbort(_context.ConnectionId, errorCode, abortReason); - Input.Complete(abortReason); + _errorCodeFeature.Error = (long)errorCode; + _frameWriter.Abort(abortReason); + + Input.Complete(abortReason); + } } public void OnInputOrOutputCompleted() @@ -104,6 +117,17 @@ private bool TryClose() return false; } + private (StreamCompletionFlags OldState, StreamCompletionFlags NewState) ApplyCompletionFlag(StreamCompletionFlags completionState) + { + lock (_completionLock) + { + var oldCompletionState = _completionState; + _completionState |= completionState; + + return (oldCompletionState, _completionState); + } + } + internal async ValueTask SendStreamIdAsync(long id) { await _frameWriter.WriteStreamIdAsync(id); @@ -215,6 +239,7 @@ public async Task ProcessRequestAsync(IHttpApplication appli } finally { + ApplyCompletionFlag(StreamCompletionFlags.Completed); _context.StreamLifetimeHandler.OnStreamCompleted(this); } } @@ -244,12 +269,6 @@ private async Task HandleControlStream() if (result.IsCompleted) { - if (!_connectionClosed) - { - // https://quicwg.org/base-drafts/draft-ietf-quic-http.html#section-6.2.1-2 - throw new Http3ConnectionErrorException(CoreStrings.Http3ErrorControlStreamClientClosedInbound, Http3ErrorCode.ClosedCriticalStream); - } - return; } } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs index 8b6ad2dfd54d..e867076bba22 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs @@ -62,8 +62,8 @@ internal abstract partial class Http3Stream : HttpProtocol, IHttp3Stream, IHttpS protected readonly Http3RawFrame _incomingFrame = new(); public bool EndStreamReceived => (_completionState & StreamCompletionFlags.EndStreamReceived) == StreamCompletionFlags.EndStreamReceived; - private bool IsAborted => (_completionState & StreamCompletionFlags.Aborted) == StreamCompletionFlags.Aborted; - private bool IsCompleted => (_completionState & StreamCompletionFlags.Completed) == StreamCompletionFlags.Completed; + public bool IsAborted => (_completionState & StreamCompletionFlags.Aborted) == StreamCompletionFlags.Aborted; + public bool IsCompleted => (_completionState & StreamCompletionFlags.Completed) == StreamCompletionFlags.Completed; public Pipe RequestBodyPipe { get; private set; } = default!; public long? InputRemaining { get; internal set; } @@ -1232,16 +1232,6 @@ private enum PseudoHeaderFields Unknown = 0x40000000 } - [Flags] - private enum StreamCompletionFlags - { - None = 0, - EndStreamReceived = 1, - AbortedRead = 2, - Aborted = 4, - Completed = 8, - } - private static class GracefulCloseInitiator { public const int None = 0; diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/IHttp3Stream.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/IHttp3Stream.cs index 590755fda1a9..379b5e98444a 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/IHttp3Stream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/IHttp3Stream.cs @@ -36,6 +36,10 @@ internal interface IHttp3Stream bool IsRequestStream { get; } + bool EndStreamReceived { get; } + bool IsAborted { get; } + bool IsCompleted { get; } + string TraceIdentifier { get; } void Abort(ConnectionAbortedException abortReason, Http3ErrorCode errorCode); diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/StreamCompletionFlags.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/StreamCompletionFlags.cs new file mode 100644 index 000000000000..78d1edb1f6b2 --- /dev/null +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/StreamCompletionFlags.cs @@ -0,0 +1,14 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http3; + +[Flags] +internal enum StreamCompletionFlags +{ + None = 0, + EndStreamReceived = 1, + AbortedRead = 2, + Aborted = 4, + Completed = 8, +} diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs index 5373cb08b982..b65c56617685 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs @@ -287,6 +287,8 @@ await Http3Api.WaitForConnectionErrorAsync( [Fact] public async Task ControlStream_ClientToServer_ClientCloses_ConnectionError() { + var now = _serviceContext.MockSystemClock.UtcNow; + await Http3Api.InitializeConnectionAsync(_noopApplication); var controlStream = await Http3Api.CreateControlStream(id: 0); @@ -294,6 +296,12 @@ public async Task ControlStream_ClientToServer_ClientCloses_ConnectionError() await controlStream.EndStreamAsync(); + // Wait for control stream to finish processing and exit. + await controlStream.OnStreamCompletedTask.DefaultTimeout(); + + Http3Api.TriggerTick(now); + Http3Api.TriggerTick(now + TimeSpan.FromSeconds(1)); + await Http3Api.WaitForConnectionErrorAsync( ignoreNonGoAwayFrames: true, expectedLastStreamId: 0, From b88ab8578b9e4792065a10407bba3dd1a564a6c7 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Fri, 12 Aug 2022 08:42:21 +0800 Subject: [PATCH 4/5] Handle peer close, fix logging --- src/Servers/Kestrel/Core/src/CoreStrings.resx | 3 ++ .../src/Internal/Http3/Http3Connection.cs | 14 ++++---- .../src/Internal/Http3/Http3ControlStream.cs | 14 ++++---- .../shared/test/Http3/Http3InMemory.cs | 13 ++++++- .../Http3/Http3ConnectionTests.cs | 34 +++++++++++-------- .../Http3/Http3StreamTests.cs | 1 + .../Http3/Http3TimeoutTests.cs | 1 + 7 files changed, 53 insertions(+), 27 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/CoreStrings.resx b/src/Servers/Kestrel/Core/src/CoreStrings.resx index c24c54defa81..472e34a318a1 100644 --- a/src/Servers/Kestrel/Core/src/CoreStrings.resx +++ b/src/Servers/Kestrel/Core/src/CoreStrings.resx @@ -713,4 +713,7 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l Responses with status code 205 cannot have a non-zero Content-Length value. + + A control stream used by the connection was closed or reset. + \ No newline at end of file diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs index fd1b499ff3e0..ee5a11f9e31e 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs @@ -229,14 +229,12 @@ static void ValidateOpenControlStream(Http3ControlStream? stream, Http3Connectio // If a control stream is no longer active then set a timeout so that the connection is aborted next tick. if (stream.StreamTimeoutTicks == default) { - // On overflow, use max value. - var connectionCloseTicks = ticks + (TimeSpan.TicksPerSecond / 2); - stream.StreamTimeoutTicks = connectionCloseTicks >= 0 ? connectionCloseTicks : long.MaxValue; + stream.StreamTimeoutTicks = ticks; } if (stream.StreamTimeoutTicks < ticks) { - connection.Abort(new ConnectionAbortedException("A control stream used by the connection was closed or reset."), Http3ErrorCode.ClosedCriticalStream); + connection.OnStreamConnectionError(new Http3ConnectionErrorException("A control stream used by the connection was closed or reset.", Http3ErrorCode.ClosedCriticalStream)); } } } @@ -693,8 +691,7 @@ private async ValueTask ProcessOutboundControlStreamAsync(Http3ControlStream con { try { - await controlStream.SendStreamIdAsync(id: 0); - await controlStream.SendSettingsFrameAsync(); + await controlStream.ProcessOutboundSendsAsync(id: 0); } catch (Exception ex) { @@ -820,6 +817,11 @@ void IHttp3StreamLifetimeHandler.OnStreamCompleted(IHttp3Stream stream) } void IHttp3StreamLifetimeHandler.OnStreamConnectionError(Http3ConnectionErrorException ex) + { + OnStreamConnectionError(ex); + } + + private void OnStreamConnectionError(Http3ConnectionErrorException ex) { Log.Http3ConnectionError(ConnectionId, ex); Abort(new ConnectionAbortedException(ex.Message, ex), ex.ErrorCode); diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs index acc5124a42d1..b4048ffb4ee6 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs @@ -128,9 +128,16 @@ private bool TryClose() } } - internal async ValueTask SendStreamIdAsync(long id) + internal async ValueTask ProcessOutboundSendsAsync(long id) { + _streamClosedFeature.OnClosed(static state => + { + var stream = (Http3ControlStream)state!; + stream.OnStreamClosed(); + }, this); + await _frameWriter.WriteStreamIdAsync(id); + await _frameWriter.WriteSettingsAsync(_serverPeerSettings.GetNonProtocolDefaults()); } internal ValueTask SendGoAway(long id) @@ -139,11 +146,6 @@ internal ValueTask SendGoAway(long id) return _frameWriter.WriteGoAway(id); } - internal async ValueTask SendSettingsFrameAsync() - { - await _frameWriter.WriteSettingsAsync(_serverPeerSettings.GetNonProtocolDefaults()); - } - private async ValueTask TryReadStreamHeaderAsync() { // https://quicwg.org/base-drafts/draft-ietf-quic-http.html#section-6.2 diff --git a/src/Servers/Kestrel/shared/test/Http3/Http3InMemory.cs b/src/Servers/Kestrel/shared/test/Http3/Http3InMemory.cs index 443a1eb5e023..079ce4e2ec43 100644 --- a/src/Servers/Kestrel/shared/test/Http3/Http3InMemory.cs +++ b/src/Servers/Kestrel/shared/test/Http3/Http3InMemory.cs @@ -174,7 +174,7 @@ internal async Task WaitForGoAwayAsync(bool ignoreNonGoAwayFrames, long? expecte } } - internal void AssertConnectionError(Http3ErrorCode expectedErrorCode, Action matchExpectedErrorMessage = null, params string[] expectedErrorMessage) where TException : Exception + private void AssertConnectionError(Http3ErrorCode expectedErrorCode, Action matchExpectedErrorMessage = null, params string[] expectedErrorMessage) where TException : Exception { var currentError = (Http3ErrorCode)MultiplexedConnectionContext.Error; if (currentError != expectedErrorCode) @@ -1277,4 +1277,15 @@ public void OnClosed(Action callback, object state) } _onClosed.Add(new CloseAction(callback, state)); } + + public void Close() + { + if (_onClosed != null) + { + foreach (var onClose in _onClosed) + { + onClose.Callback(onClose.State); + } + } + } } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs index b65c56617685..f557c0ea34e0 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs @@ -243,6 +243,7 @@ await Http3Api.WaitForConnectionErrorAsync( ignoreNonGoAwayFrames: true, expectedLastStreamId: 0, expectedErrorCode: Http3ErrorCode.SettingsError, + matchExpectedErrorMessage: AssertExpectedErrorMessages, expectedErrorMessage: CoreStrings.FormatHttp3ErrorControlStreamReservedSetting($"0x{settingIdentifier.ToString("X", CultureInfo.InvariantCulture)}")); } @@ -261,6 +262,7 @@ await Http3Api.WaitForConnectionErrorAsync( ignoreNonGoAwayFrames: true, expectedLastStreamId: 0, expectedErrorCode: Http3ErrorCode.StreamCreationError, + matchExpectedErrorMessage: AssertExpectedErrorMessages, expectedErrorMessage: CoreStrings.FormatHttp3ControlStreamErrorMultipleInboundStreams(name)); } @@ -281,11 +283,12 @@ await Http3Api.WaitForConnectionErrorAsync( ignoreNonGoAwayFrames: true, expectedLastStreamId: 0, expectedErrorCode: Http3ErrorCode.UnexpectedFrame, + matchExpectedErrorMessage: AssertExpectedErrorMessages, expectedErrorMessage: CoreStrings.FormatHttp3ErrorUnsupportedFrameOnControlStream(Http3Formatting.ToFormattedType(f))); } [Fact] - public async Task ControlStream_ClientToServer_ClientCloses_ConnectionError() + public async Task ControlStream_ClientToServer_Completes_ConnectionError() { var now = _serviceContext.MockSystemClock.UtcNow; @@ -294,7 +297,7 @@ public async Task ControlStream_ClientToServer_ClientCloses_ConnectionError() var controlStream = await Http3Api.CreateControlStream(id: 0); await controlStream.SendSettingsAsync(new List()); - await controlStream.EndStreamAsync(); + await controlStream.EndStreamAsync().DefaultTimeout(); // Wait for control stream to finish processing and exit. await controlStream.OnStreamCompletedTask.DefaultTimeout(); @@ -306,27 +309,30 @@ await Http3Api.WaitForConnectionErrorAsync( ignoreNonGoAwayFrames: true, expectedLastStreamId: 0, expectedErrorCode: Http3ErrorCode.ClosedCriticalStream, - expectedErrorMessage: CoreStrings.Http3ErrorControlStreamClientClosedInbound); + matchExpectedErrorMessage: AssertExpectedErrorMessages, + expectedErrorMessage: CoreStrings.Http3ErrorControlStreamClosed); } [Fact] - public async Task ControlStream_ServerToClient_ErrorInitializing_ConnectionError() + public async Task ControlStream_ServerToClient_Closes_ConnectionError() { - Http3Api.OnCreateServerControlStream = testStreamContext => - { - var controlStream = new Microsoft.AspNetCore.Testing.Http3ControlStream(Http3Api, testStreamContext); + var now = _serviceContext.MockSystemClock.UtcNow; - // Make server connection error when trying to write to control stream. - controlStream.StreamContext.Transport.Output.Complete(); + await Http3Api.InitializeConnectionAsync(_noopApplication); - return controlStream; - }; + var controlStream = await Http3Api.GetInboundControlStream(); - await Http3Api.InitializeConnectionAsync(_noopApplication); + controlStream.StreamContext.Close(); - Http3Api.AssertConnectionError( + Http3Api.TriggerTick(now); + Http3Api.TriggerTick(now + TimeSpan.FromSeconds(1)); + + await Http3Api.WaitForConnectionErrorAsync( + ignoreNonGoAwayFrames: true, + expectedLastStreamId: 0, expectedErrorCode: Http3ErrorCode.ClosedCriticalStream, - expectedErrorMessage: CoreStrings.Http3ControlStreamErrorInitializingOutbound); + matchExpectedErrorMessage: AssertExpectedErrorMessages, + expectedErrorMessage: CoreStrings.Http3ErrorControlStreamClosed); } [Fact] diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs index da2092b13d6e..884d3d9a4f4a 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs @@ -2105,6 +2105,7 @@ await Http3Api.WaitForConnectionErrorAsync( ignoreNonGoAwayFrames: true, expectedLastStreamId: 4, expectedErrorCode: Http3ErrorCode.UnexpectedFrame, + matchExpectedErrorMessage: AssertExpectedErrorMessages, expectedErrorMessage: CoreStrings.FormatHttp3ErrorUnsupportedFrameOnRequestStream(Http3Formatting.ToFormattedType(f))); } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TimeoutTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TimeoutTests.cs index 88782e9e57c8..f68e8d874604 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TimeoutTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TimeoutTests.cs @@ -310,6 +310,7 @@ await Http3Api.WaitForConnectionErrorAsync( ignoreNonGoAwayFrames: false, expectedLastStreamId: 4, Http3ErrorCode.InternalError, + matchExpectedErrorMessage: AssertExpectedErrorMessages, expectedErrorMessage: CoreStrings.ConnectionTimedBecauseResponseMininumDataRateNotSatisfied); Assert.Contains(TestSink.Writes, w => w.EventId.Name == "ResponseMinimumDataRateNotSatisfied"); From dec1aaa7c150fe53fcfffb11af5046f703f9c25a Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Fri, 12 Aug 2022 11:00:07 +0800 Subject: [PATCH 5/5] Fix test --- src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs | 2 +- .../test/InMemory.FunctionalTests/Http3/Http3TimeoutTests.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs index ee5a11f9e31e..c94ae4f4dbe7 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs @@ -311,7 +311,7 @@ private void UpdateStreamTimeouts(DateTimeOffset now) { // Cancel connection to be consistent with other data rate limits. Log.ResponseMinimumDataRateNotSatisfied(_context.ConnectionId, stream.TraceIdentifier); - Abort(new ConnectionAbortedException(CoreStrings.ConnectionTimedBecauseResponseMininumDataRateNotSatisfied), Http3ErrorCode.InternalError); + OnStreamConnectionError(new Http3ConnectionErrorException(CoreStrings.ConnectionTimedBecauseResponseMininumDataRateNotSatisfied, Http3ErrorCode.InternalError)); } } } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TimeoutTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TimeoutTests.cs index f68e8d874604..835f5d65d509 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TimeoutTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TimeoutTests.cs @@ -306,7 +306,7 @@ public async Task ResponseDrain_SlowerThanMinimumDataRate_AbortsConnection() requestStream.StartStreamDisposeTcs.TrySetResult(); - await Http3Api.WaitForConnectionErrorAsync( + await Http3Api.WaitForConnectionErrorAsync( ignoreNonGoAwayFrames: false, expectedLastStreamId: 4, Http3ErrorCode.InternalError,