From 6d42ff7c3884f979c432480bf867e5f5a15c18f9 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Mon, 11 Feb 2019 19:01:36 +0000 Subject: [PATCH] Don't write empty data for Flush (#7415) --- .../src/Internal/Http/Http1OutputProducer.cs | 132 ++++++++++++------ .../Http/HttpProtocol.FeatureCollection.cs | 4 +- .../src/Internal/Infrastructure/Constants.cs | 2 - 3 files changed, 95 insertions(+), 43 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs index fcc733143f..0286ce0db0 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs @@ -100,7 +100,53 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http public ValueTask FlushAsync(CancellationToken cancellationToken = default) { - return WriteAsync(Constants.EmptyData, cancellationToken); + lock (_contextLock) + { + if (_pipeWriterCompleted) + { + return default; + } + + if (_autoChunk) + { + if (_advancedBytesForChunk > 0) + { + // If there is data that was chunked before flushing (ex someone did GetMemory->Advance->FlushAsync) + // make sure to write whatever was advanced first + return FlushAsyncChunked(this, cancellationToken); + } + else + { + // If there is an empty write, we still need to update the current chunk + _currentChunkMemoryUpdated = false; + } + } + + var bytesWritten = _unflushedBytes; + _unflushedBytes = 0; + + return _flusher.FlushAsync(_minResponseDataRateFeature.MinDataRate, bytesWritten, this, cancellationToken); + } + + ValueTask FlushAsyncChunked(Http1OutputProducer producer, CancellationToken token) + { + // Local function so in the common-path the stack space for BufferWriter isn't reservered and cleared when it isn't used. + + Debug.Assert(!producer._pipeWriterCompleted); + Debug.Assert(producer._autoChunk && producer._advancedBytesForChunk > 0); + + var writer = new BufferWriter(producer._pipeWriter); + producer.WriteCurrentMemoryToPipeWriter(ref writer); + writer.Commit(); + + var bytesWritten = producer._unflushedBytes + writer.BytesCommitted; + producer._unflushedBytes = 0; + + // If there is an empty write, we still need to update the current chunk + _currentChunkMemoryUpdated = false; + + return producer._flusher.FlushAsync(producer._minResponseDataRateFeature.MinDataRate, bytesWritten, producer, token); + } } public Memory GetMemory(int sizeHint = 0) @@ -187,17 +233,23 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http // Make sure any memory used with GetMemory/Advance is written before the chunk // passed in. - WriteCurrentMemoryToPipeWriter(); - - if (buffer.Length > 0) + if (_advancedBytesForChunk > 0 || buffer.Length > 0) { var writer = new BufferWriter(_pipeWriter); + if (_advancedBytesForChunk > 0) + { + WriteCurrentMemoryToPipeWriter(ref writer); + } + + if (buffer.Length > 0) + { + + writer.WriteBeginChunkBytes(buffer.Length); + writer.Write(buffer); + writer.WriteEndChunkBytes(); + } - writer.WriteBeginChunkBytes(buffer.Length); - writer.Write(buffer); - writer.WriteEndChunkBytes(); writer.Commit(); - _unflushedBytes += writer.BytesCommitted; } } @@ -297,23 +349,29 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http return default; } + var writer = new BufferWriter(_pipeWriter); if (_autoChunk) { - // If there is data that was chunked before writing (ex someone did GetMemory->Advance->WriteAsync) - // make sure to write whatever was advanced first - WriteCurrentMemoryToPipeWriter(); + if (_advancedBytesForChunk > 0) + { + // If there is data that was chunked before writing (ex someone did GetMemory->Advance->WriteAsync) + // make sure to write whatever was advanced first + WriteCurrentMemoryToPipeWriter(ref writer); + } + else + { + // If there is an empty write, we still need to update the current chunk + _currentChunkMemoryUpdated = false; + } } - var writer = new BufferWriter(_pipeWriter); if (buffer.Length > 0) { writer.Write(buffer); - - _unflushedBytes += buffer.Length; } writer.Commit(); - var bytesWritten = _unflushedBytes; + var bytesWritten = _unflushedBytes + writer.BytesCommitted; _unflushedBytes = 0; return _flusher.FlushAsync( @@ -342,7 +400,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http if (_advancedBytesForChunk >= memoryMaxLength - Math.Min(MemorySizeThreshold, sizeHint)) { // Chunk is completely written, commit it to the pipe so GetMemory will return a new chunk of memory. - WriteCurrentMemoryToPipeWriter(); + var writer = new BufferWriter(_pipeWriter); + WriteCurrentMemoryToPipeWriter(ref writer); + writer.Commit(); + + _unflushedBytes += writer.BytesCommitted; _currentChunkMemory = _pipeWriter.GetMemory(sizeHint); _currentChunkMemoryUpdated = true; } @@ -356,35 +418,27 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http return actualMemory; } - private void WriteCurrentMemoryToPipeWriter() + private void WriteCurrentMemoryToPipeWriter(ref BufferWriter writer) { - var writer = new BufferWriter(_pipeWriter); - Debug.Assert(_advancedBytesForChunk <= _currentChunkMemory.Length); + Debug.Assert(_advancedBytesForChunk > 0); - if (_advancedBytesForChunk > 0) + var bytesWritten = writer.WriteBeginChunkBytes(_advancedBytesForChunk); + + Debug.Assert(bytesWritten <= BeginChunkLengthMax); + + if (bytesWritten < BeginChunkLengthMax) { - var bytesWritten = writer.WriteBeginChunkBytes(_advancedBytesForChunk); - - Debug.Assert(bytesWritten <= BeginChunkLengthMax); - - if (bytesWritten < BeginChunkLengthMax) - { - // If the current chunk of memory isn't completely utilized, we need to copy the contents forwards. - // This occurs if someone uses less than 255 bytes of the current Memory segment. - // Therefore, we need to copy it forwards by either 1 or 2 bytes (depending on number of bytes) - _currentChunkMemory.Slice(BeginChunkLengthMax, _advancedBytesForChunk).CopyTo(_currentChunkMemory.Slice(bytesWritten)); - } - - writer.Advance(_advancedBytesForChunk); - writer.WriteEndChunkBytes(); - writer.Commit(); - _advancedBytesForChunk = 0; - _unflushedBytes += writer.BytesCommitted; + // If the current chunk of memory isn't completely utilized, we need to copy the contents forwards. + // This occurs if someone uses less than 255 bytes of the current Memory segment. + // Therefore, we need to copy it forwards by either 1 or 2 bytes (depending on number of bytes) + _currentChunkMemory.Slice(BeginChunkLengthMax, _advancedBytesForChunk).CopyTo(_currentChunkMemory.Slice(bytesWritten)); } - // If there is an empty write, we still need to update the current chunk - _currentChunkMemoryUpdated = false; + writer.Advance(_advancedBytesForChunk); + writer.WriteEndChunkBytes(); + + _advancedBytesForChunk = 0; } private Memory GetFakeMemory(int sizeHint) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.FeatureCollection.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.FeatureCollection.cs index 9ef4f4cd4a..159f61a798 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.FeatureCollection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.FeatureCollection.cs @@ -19,14 +19,14 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http { public partial class HttpProtocol : IHttpRequestFeature, IHttpResponseFeature, + IResponseBodyPipeFeature, IHttpUpgradeFeature, IHttpConnectionFeature, IHttpRequestLifetimeFeature, IHttpRequestIdentifierFeature, IHttpBodyControlFeature, IHttpMaxRequestBodySizeFeature, - IHttpResponseStartFeature, - IResponseBodyPipeFeature + IHttpResponseStartFeature { // NOTE: When feature interfaces are added to or removed from this HttpProtocol class implementation, // then the list of `implementedFeatures` in the generated code project MUST also be updated. diff --git a/src/Servers/Kestrel/Core/src/Internal/Infrastructure/Constants.cs b/src/Servers/Kestrel/Core/src/Internal/Infrastructure/Constants.cs index 8aead7103c..c485f0bfbd 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Infrastructure/Constants.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Infrastructure/Constants.cs @@ -37,7 +37,5 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure public const string ServerName = "Kestrel"; public static readonly TimeSpan RequestBodyDrainTimeout = TimeSpan.FromSeconds(5); - - public static readonly ArraySegment EmptyData = new ArraySegment(new byte[0]); } }