From 54ecdaefce1b97d857f1aaa15976a18f85285741 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Sat, 21 Sep 2019 08:48:21 +1200 Subject: [PATCH] HTTP2: Send remaining data and trailers together (#13914) --- .../src/Internal/Http2/Http2FrameWriter.cs | 10 +- .../src/Internal/Http2/Http2OutputProducer.cs | 6 +- .../Http2/Http2StreamTests.cs | 121 ++++++++++++++++++ 3 files changed, 133 insertions(+), 4 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs index 7555b9f223..1177504aa6 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs @@ -239,7 +239,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 } } - public ValueTask WriteDataAsync(int streamId, StreamOutputFlowControl flowControl, in ReadOnlySequence data, bool endStream) + public ValueTask WriteDataAsync(int streamId, StreamOutputFlowControl flowControl, in ReadOnlySequence data, bool endStream, bool forceFlush) { // The Length property of a ReadOnlySequence can be expensive, so we cache the value. var dataLength = data.Length; @@ -261,7 +261,13 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 // This cast is safe since if dataLength would overflow an int, it's guaranteed to be greater than the available flow control window. flowControl.Advance((int)dataLength); WriteDataUnsynchronized(streamId, data, dataLength, endStream); - return TimeFlushUnsynchronizedAsync(); + + if (forceFlush) + { + return TimeFlushUnsynchronizedAsync(); + } + + return default; } } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs index 1e5f09732a..1c02ddfb3c 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs @@ -375,7 +375,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 // Write any remaining content then write trailers if (readResult.Buffer.Length > 0) { - flushResult = await _frameWriter.WriteDataAsync(_streamId, _flowControl, readResult.Buffer, endStream: false); + // Only flush if required (i.e. content length exceeds flow control availability) + // Writing remaining content without flushing allows content and trailers to be sent in the same packet + await _frameWriter.WriteDataAsync(_streamId, _flowControl, readResult.Buffer, endStream: false, forceFlush: false); } _stream.ResponseTrailers.SetReadOnly(); @@ -399,7 +401,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 { _stream.DecrementActiveClientStreamCount(); } - flushResult = await _frameWriter.WriteDataAsync(_streamId, _flowControl, readResult.Buffer, endStream); + flushResult = await _frameWriter.WriteDataAsync(_streamId, _flowControl, readResult.Buffer, endStream, forceFlush: true); } _pipeReader.AdvanceTo(readResult.Buffer.End); diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs index c74f321a51..caafd8ff61 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs @@ -2044,6 +2044,127 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests Assert.Contains(CoreStrings.HPackErrorNotEnoughBuffer, message.Exception.Message); } + [Fact] + public async Task ResponseTrailers_WithLargeUnflushedData_DataExceedsFlowControlAvailableAndNotSentWithTrailers() + { + const int windowSize = (int)Http2PeerSettings.DefaultMaxFrameSize; + _clientSettings.InitialWindowSize = windowSize; + + var headers = new[] + { + new KeyValuePair(HeaderNames.Method, "GET"), + new KeyValuePair(HeaderNames.Path, "/"), + new KeyValuePair(HeaderNames.Scheme, "http"), + }; + await InitializeConnectionAsync(async context => + { + await context.Response.StartAsync(); + + // Body exceeds flow control available and requires the client to allow more + // data via updating the window + context.Response.BodyWriter.GetMemory(windowSize + 1); + context.Response.BodyWriter.Advance(windowSize + 1); + + context.Response.AppendTrailer("CustomName", "Custom Value"); + }).DefaultTimeout(); + + await StartStreamAsync(1, headers, endStream: true).DefaultTimeout(); + + var headersFrame = await ExpectAsync(Http2FrameType.HEADERS, + withLength: 37, + withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS, + withStreamId: 1).DefaultTimeout(); + + await ExpectAsync(Http2FrameType.DATA, + withLength: 16384, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 1).DefaultTimeout(); + + var dataTask = ExpectAsync(Http2FrameType.DATA, + withLength: 1, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 1).DefaultTimeout(); + + // Reading final frame of data requires window update + // Verify this data task is waiting on window update + Assert.False(dataTask.IsCompletedSuccessfully); + + await SendWindowUpdateAsync(1, 1); + + await dataTask; + + var trailersFrame = await ExpectAsync(Http2FrameType.HEADERS, + withLength: 25, + withFlags: (byte)(Http2HeadersFrameFlags.END_HEADERS | Http2HeadersFrameFlags.END_STREAM), + withStreamId: 1).DefaultTimeout(); + + await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false).DefaultTimeout(); + + _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]); + + _decodedHeaders.Clear(); + _hpackDecoder.Decode(trailersFrame.PayloadSequence, endHeaders: true, handler: this); + + Assert.Single(_decodedHeaders); + Assert.Equal("Custom Value", _decodedHeaders["CustomName"]); + } + + [Fact] + public async Task ResponseTrailers_WithUnflushedData_DataSentWithTrailers() + { + var headers = new[] + { + new KeyValuePair(HeaderNames.Method, "GET"), + new KeyValuePair(HeaderNames.Path, "/"), + new KeyValuePair(HeaderNames.Scheme, "http"), + }; + await InitializeConnectionAsync(async context => + { + await context.Response.StartAsync(); + + var s = context.Response.BodyWriter.GetMemory(1); + s.Span[0] = byte.MaxValue; + context.Response.BodyWriter.Advance(1); + + context.Response.AppendTrailer("CustomName", "Custom Value"); + }); + + await StartStreamAsync(1, headers, endStream: true); + + var headersFrame = await ExpectAsync(Http2FrameType.HEADERS, + withLength: 37, + withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS, + withStreamId: 1); + + await ExpectAsync(Http2FrameType.DATA, + withLength: 1, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 1); + + var trailersFrame = await ExpectAsync(Http2FrameType.HEADERS, + withLength: 25, + 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(2, _decodedHeaders.Count); + Assert.Contains("date", _decodedHeaders.Keys, StringComparer.OrdinalIgnoreCase); + Assert.Equal("200", _decodedHeaders[HeaderNames.Status]); + + _decodedHeaders.Clear(); + _hpackDecoder.Decode(trailersFrame.PayloadSequence, endHeaders: true, handler: this); + + Assert.Single(_decodedHeaders); + Assert.Equal("Custom Value", _decodedHeaders["CustomName"]); + } + [Fact] public async Task ApplicationException_BeforeFirstWrite_Sends500() {