From 1db76280aeeba3de5c45c07b67e3f235757b5479 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Fri, 6 Sep 2019 23:01:19 -0700 Subject: [PATCH] Don't call complete outside of main request loop (#13728) --- .../Core/src/Internal/Http/HttpProtocol.cs | 5 +- .../src/Internal/Http2/Http2OutputProducer.cs | 46 ++++++++++++------- .../Http2/Http2StreamTests.cs | 46 +++++++++++++++++++ 3 files changed, 78 insertions(+), 19 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs index 9320980de2..7f10c3af64 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs @@ -1009,6 +1009,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http return Task.CompletedTask; } + _isLeasedMemoryInvalid = true; + if (_requestRejectedException != null || _applicationException != null) { if (HasResponseStarted) @@ -1339,8 +1341,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http if (_isLeasedMemoryInvalid) { - throw new InvalidOperationException("Invalid ordering of calling StartAsync and Advance. " + - "Call StartAsync before calling GetMemory/GetSpan and Advance."); + throw new InvalidOperationException("Invalid ordering of calling StartAsync or CompleteAsync and Advance."); } if (_canWriteResponseBody) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs index 1e5f09732a..18adcc1a82 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs @@ -80,6 +80,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 Stop(); + // Make sure the writing side is completed. + _pipeWriter.Complete(); + if (_fakeMemoryOwner != null) { _fakeMemoryOwner.Dispose(); @@ -104,7 +107,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 lock (_dataWriterLock) { - ThrowIfSuffixSent(); + ThrowIfSuffixSentOrDisposed(); if (_completed) { @@ -130,7 +133,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 { lock (_dataWriterLock) { - ThrowIfSuffixSent(); + ThrowIfSuffixSentOrDisposed(); if (_completed) { @@ -185,7 +188,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 lock (_dataWriterLock) { - ThrowIfSuffixSent(); + ThrowIfSuffixSentOrDisposed(); // This length check is important because we don't want to set _startedWritingDataFrames unless a data // frame will actually be written causing the headers to be flushed. @@ -233,7 +236,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 { lock (_dataWriterLock) { - ThrowIfSuffixSent(); + ThrowIfSuffixSentOrDisposed(); if (_completed) { @@ -250,7 +253,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 { lock (_dataWriterLock) { - ThrowIfSuffixSent(); + ThrowIfSuffixSentOrDisposed(); if (_completed) { @@ -265,7 +268,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 { lock (_dataWriterLock) { - ThrowIfSuffixSent(); + ThrowIfSuffixSentOrDisposed(); if (_completed) { @@ -298,7 +301,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 lock (_dataWriterLock) { - ThrowIfSuffixSent(); + ThrowIfSuffixSentOrDisposed(); // This length check is important because we don't want to set _startedWritingDataFrames unless a data // frame will actually be written causing the headers to be flushed. @@ -345,10 +348,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 _completed = true; - // Complete with an exception to prevent an end of stream data frame from being sent without an - // explicit call to WriteStreamSuffixAsync. ConnectionAbortedExceptions are swallowed, so the - // message doesn't matter - _pipeWriter.Complete(new OperationCanceledException()); + _pipeReader.CancelPendingRead(); _frameWriter.AbortPendingStreamDataWrites(_flowControl); } @@ -369,7 +369,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 { readResult = await _pipeReader.ReadAsync(); - if (readResult.IsCompleted && _stream.ResponseTrailers?.Count > 0) + if (readResult.IsCanceled) + { + // Response body is aborted, break and complete reader. + break; + } + else if (readResult.IsCompleted && _stream.ResponseTrailers?.Count > 0) { // Output is ending and there are trailers to write // Write any remaining content then write trailers @@ -405,10 +410,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 _pipeReader.AdvanceTo(readResult.Buffer.End); } while (!readResult.IsCompleted); } - catch (OperationCanceledException) - { - // Writes should not throw for aborted streams/connections. - } catch (Exception ex) { _log.LogCritical(ex, nameof(Http2OutputProducer) + "." + nameof(ProcessDataWrites) + " observed an unexpected exception."); @@ -435,12 +436,17 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 } [StackTraceHidden] - private void ThrowIfSuffixSent() + private void ThrowIfSuffixSentOrDisposed() { if (_suffixSent) { ThrowSuffixSent(); } + + if (_disposed) + { + ThrowDisposed(); + } } [StackTraceHidden] @@ -449,6 +455,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 throw new InvalidOperationException("Writing is not allowed after writer was completed."); } + [StackTraceHidden] + private static void ThrowDisposed() + { + throw new InvalidOperationException("Cannot write to response after the request has completed."); + } + private static Pipe CreateDataPipe(MemoryPool pool) => new Pipe(new PipeOptions ( diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs index c74f321a51..4937000db9 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs @@ -3737,6 +3737,52 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests Assert.Equal("Hello World", Encoding.UTF8.GetString(bodyFrame.Payload.Span)); } + [Fact] + public async Task CompleteAsync_AdvanceAfterComplete_AdvanceThrows() + { + var tcs = new TaskCompletionSource(); + var headers = new[] + { + new KeyValuePair(HeaderNames.Method, "GET"), + new KeyValuePair(HeaderNames.Path, "/"), + new KeyValuePair(HeaderNames.Scheme, "http"), + }; + await InitializeConnectionAsync(async context => + { + var memory = context.Response.BodyWriter.GetMemory(12); + await context.Response.CompleteAsync(); + try + { + context.Response.BodyWriter.Advance(memory.Length); + } + catch (InvalidOperationException) + { + tcs.SetResult(null); + return; + } + + Assert.True(false); + }); + + await StartStreamAsync(1, headers, endStream: true); + + var headersFrame = await ExpectAsync(Http2FrameType.HEADERS, + withLength: 55, + withFlags: (byte)(Http2HeadersFrameFlags.END_HEADERS | Http2HeadersFrameFlags.END_STREAM), + withStreamId: 1); + + await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); + + _hpackDecoder.Decode(headersFrame.PayloadSequence, endHeaders: false, handler: this); + + Assert.Equal(3, _decodedHeaders.Count); + Assert.Contains("date", _decodedHeaders.Keys, StringComparer.OrdinalIgnoreCase); + Assert.Equal("200", _decodedHeaders[HeaderNames.Status]); + Assert.Equal("0", _decodedHeaders[HeaderNames.ContentLength]); + + await tcs.Task.DefaultTimeout(); + } + [Fact] public async Task CompleteAsync_AfterPipeWrite_WithTrailers_SendsBodyAndTrailersWithEndStream() {