From a8073167e7adcca0d6708d4da56e43066535e3a5 Mon Sep 17 00:00:00 2001 From: "Chris Ross (ASP.NET)" Date: Tue, 23 Oct 2018 15:49:11 -0700 Subject: [PATCH] Revert "Flush response headers #3031" This reverts commit 277a5502fd44d565d8086480b829d1c9995a78ab. --- .../Internal/Http2/Http2FrameWriter.cs | 17 +++++-- .../Internal/Http2/Http2OutputProducer.cs | 18 ++++++- .../Http2/Http2StreamTests.cs | 49 ------------------- 3 files changed, 30 insertions(+), 54 deletions(-) diff --git a/src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs b/src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs index 1242c93131..abaea59245 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs @@ -104,6 +104,19 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 } } + public Task FlushAsync(IHttpOutputAborter outputAborter, CancellationToken cancellationToken) + { + lock (_writeLock) + { + if (_completed) + { + return Task.CompletedTask; + } + + return _flusher.FlushAsync(outputAborter, cancellationToken); + } + } + public Task Write100ContinueAsync(int streamId) { lock (_writeLock) @@ -158,11 +171,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 throw new InvalidOperationException(hex.Message, hex); // Report the error to the user if this was the first write. } } - - _ = _flusher.FlushAsync(); } - public Task WriteResponseTrailersAsync(int streamId, HttpResponseTrailers headers) + public Task WriteResponseTrailers(int streamId, HttpResponseTrailers headers) { lock (_writeLock) { diff --git a/src/Kestrel.Core/Internal/Http2/Http2OutputProducer.cs b/src/Kestrel.Core/Internal/Http2/Http2OutputProducer.cs index 83e517d05f..fa31210932 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2OutputProducer.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2OutputProducer.cs @@ -28,6 +28,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 private readonly object _dataWriterLock = new object(); private readonly Pipe _dataPipe; private readonly Task _dataWriteProcessingTask; + private bool _startedWritingDataFrames; private bool _completed; private bool _disposed; @@ -100,7 +101,18 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 return Task.CompletedTask; } - return _flusher.FlushAsync(this, cancellationToken); + if (_startedWritingDataFrames) + { + // If there's already been response data written to the stream, just wait for that. Any header + // should be in front of the data frames in the connection pipe. Trailers could change things. + return _flusher.FlushAsync(this, cancellationToken); + } + else + { + // Flushing the connection pipe ensures headers already in the pipe are flushed even if no data + // frames have been written. + return _frameWriter.FlushAsync(this, cancellationToken); + } } } @@ -148,6 +160,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 return Task.CompletedTask; } + _startedWritingDataFrames = true; + _dataPipe.Writer.Write(data); return _flusher.FlushAsync(this, cancellationToken); } @@ -198,7 +212,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 await _frameWriter.WriteDataAsync(_streamId, _flowControl, _stream.MinResponseDataRate, readResult.Buffer, endStream: false); } - await _frameWriter.WriteResponseTrailersAsync(_streamId, _stream.Trailers); + await _frameWriter.WriteResponseTrailers(_streamId, _stream.Trailers); } else { diff --git a/test/Kestrel.InMemory.FunctionalTests/Http2/Http2StreamTests.cs b/test/Kestrel.InMemory.FunctionalTests/Http2/Http2StreamTests.cs index 1cf437ecab..8375561f9b 100644 --- a/test/Kestrel.InMemory.FunctionalTests/Http2/Http2StreamTests.cs +++ b/test/Kestrel.InMemory.FunctionalTests/Http2/Http2StreamTests.cs @@ -2083,54 +2083,5 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests Assert.Contains("date", _decodedHeaders.Keys, StringComparer.OrdinalIgnoreCase); Assert.Equal("200", _decodedHeaders[HeaderNames.Status]); } - - [Fact] - public async Task ResponseHeaders_NotBlockedByFlowControl() - { - var headers = new[] - { - new KeyValuePair(HeaderNames.Method, "GET"), - new KeyValuePair(HeaderNames.Path, "/"), - new KeyValuePair(HeaderNames.Scheme, "http"), - }; - await InitializeConnectionAsync(context => - { - return context.Response.WriteAsync("hello world"); - }); - - _clientSettings.InitialWindowSize = 0; - await SendSettingsAsync(); - await ExpectAsync(Http2FrameType.SETTINGS, - withLength: 0, - withFlags: (byte)Http2SettingsFrameFlags.ACK, - withStreamId: 0); - - await StartStreamAsync(1, headers, endStream: true); - - var headersFrame = await ExpectAsync(Http2FrameType.HEADERS, - withLength: 37, - withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS, - withStreamId: 1); - - await SendWindowUpdateAsync(1, 11); - - await ExpectAsync(Http2FrameType.DATA, - withLength: 11, - withFlags: (byte)Http2DataFrameFlags.NONE, - withStreamId: 1); - - await ExpectAsync(Http2FrameType.DATA, - withLength: 0, - withFlags: (byte)Http2DataFrameFlags.END_STREAM, - withStreamId: 1); - - await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); - - _hpackDecoder.Decode(headersFrame.PayloadSequence, endHeaders: false, handler: this); - - Assert.Equal(2, _decodedHeaders.Count); - Assert.Contains("date", _decodedHeaders.Keys, StringComparer.OrdinalIgnoreCase); - Assert.Equal("200", _decodedHeaders[HeaderNames.Status]); - } } } \ No newline at end of file