From 549f8e17734105505015652436ff1d6a7e53680c Mon Sep 17 00:00:00 2001 From: David Fowler Date: Mon, 28 Jan 2019 20:36:47 +0000 Subject: [PATCH] Remove over engineered callback writing and just write chunks directly. (#6901) --- .../src/Internal/Http/Http1OutputProducer.cs | 16 +++++++--- .../Core/src/Internal/Http/HttpProtocol.cs | 27 +++------------- .../src/Internal/Http/IHttpOutputProducer.cs | 3 +- .../src/Internal/Http2/Http2OutputProducer.cs | 2 +- .../Kestrel/Core/test/OutputProducerTests.cs | 32 +++++++++++-------- .../test/LibuvOutputConsumerTests.cs | 8 +---- 6 files changed, 37 insertions(+), 51 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs index c2c94a10c8..e304824db2 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs @@ -71,7 +71,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http return WriteAsync(Constants.EmptyData, cancellationToken); } - public Task WriteAsync(Func callback, T state, CancellationToken cancellationToken) + public Task WriteChunkAsync(ReadOnlySpan buffer, CancellationToken cancellationToken) { lock (_contextLock) { @@ -80,9 +80,17 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http return Task.CompletedTask; } - var buffer = _pipeWriter; - var bytesCommitted = callback(buffer, state); - _unflushedBytes += bytesCommitted; + if (buffer.Length > 0) + { + var writer = new BufferWriter(_pipeWriter); + + writer.WriteBeginChunkBytes(buffer.Length); + writer.Write(buffer); + writer.WriteEndChunkBytes(); + writer.Commit(); + + _unflushedBytes += writer.BytesCommitted; + } } return FlushAsync(cancellationToken); diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs index 9d8dad0304..1edbc54a8e 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs @@ -30,7 +30,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http private static readonly byte[] _bytesConnectionKeepAlive = Encoding.ASCII.GetBytes("\r\nConnection: keep-alive"); private static readonly byte[] _bytesTransferEncodingChunked = Encoding.ASCII.GetBytes("\r\nTransfer-Encoding: chunked"); private static readonly byte[] _bytesServer = Encoding.ASCII.GetBytes("\r\nServer: " + Constants.ServerName); - private static readonly Func, long> _writeChunk = WriteChunk; private readonly object _onStartingSync = new Object(); private readonly object _onCompletedSync = new Object(); @@ -820,7 +819,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http { return !firstWrite ? Task.CompletedTask : FlushAsync(cancellationToken); } - return WriteChunkedAsync(data, cancellationToken); + return WriteChunkedAsync(data.Span, cancellationToken); } else { @@ -851,7 +850,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http return; } - await WriteChunkedAsync(data, cancellationToken); + await WriteChunkedAsync(data.Span, cancellationToken); } else { @@ -936,27 +935,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http } } - private Task WriteChunkedAsync(ReadOnlyMemory data, CancellationToken cancellationToken) + private Task WriteChunkedAsync(ReadOnlySpan data, CancellationToken cancellationToken) { - return Output.WriteAsync(_writeChunk, data, cancellationToken); - } - - private static long WriteChunk(PipeWriter writableBuffer, ReadOnlyMemory buffer) - { - var bytesWritten = 0L; - if (buffer.Length > 0) - { - var writer = new BufferWriter(writableBuffer); - - writer.WriteBeginChunkBytes(buffer.Length); - writer.Write(buffer.Span); - writer.WriteEndChunkBytes(); - writer.Commit(); - - bytesWritten = writer.BytesCommitted; - } - - return bytesWritten; + return Output.WriteChunkAsync(data, cancellationToken); } public void ProduceContinue() diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/IHttpOutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http/IHttpOutputProducer.cs index 1259a18f5f..ecd7ad3b8f 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/IHttpOutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/IHttpOutputProducer.cs @@ -2,7 +2,6 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.IO.Pipelines; using System.Threading; using System.Threading.Tasks; @@ -10,7 +9,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http { public interface IHttpOutputProducer { - Task WriteAsync(Func callback, T state, CancellationToken cancellationToken); + Task WriteChunkAsync(ReadOnlySpan data, CancellationToken cancellationToken); Task FlushAsync(CancellationToken cancellationToken); Task Write100ContinueAsync(); void WriteResponseHeaders(int statusCode, string ReasonPhrase, HttpResponseHeaders responseHeaders); diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs index 09111b1d77..44552fee76 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs @@ -83,7 +83,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 Dispose(); } - public Task WriteAsync(Func callback, T state, CancellationToken cancellationToken) + public Task WriteChunkAsync(ReadOnlySpan span, CancellationToken cancellationToken) { throw new NotImplementedException(); } diff --git a/src/Servers/Kestrel/Core/test/OutputProducerTests.cs b/src/Servers/Kestrel/Core/test/OutputProducerTests.cs index 5c8fab6d69..537e199e74 100644 --- a/src/Servers/Kestrel/Core/test/OutputProducerTests.cs +++ b/src/Servers/Kestrel/Core/test/OutputProducerTests.cs @@ -1,4 +1,4 @@ -// Copyright (c) .NET Foundation. All rights reserved. +// Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; @@ -46,17 +46,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests // Close socketOutput.Dispose(); - var called = false; + await socketOutput.WriteDataAsync(new byte[] { 1, 2, 3, 4 }, default); - await socketOutput.WriteAsync((buffer, state) => - { - called = true; - return 0; - }, - 0, - default); - - Assert.False(called); + Assert.True(socketOutput.Pipe.Reader.TryRead(out var result)); + Assert.True(result.IsCompleted); + Assert.True(result.Buffer.IsEmpty); } } @@ -80,7 +74,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests mockConnectionContext.Verify(f => f.Abort(null), Times.Once()); } - private Http1OutputProducer CreateOutputProducer( + private TestHttpOutputProducer CreateOutputProducer( PipeOptions pipeOptions = null, ConnectionContext connectionContext = null) { @@ -89,8 +83,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests var pipe = new Pipe(pipeOptions); var serviceContext = new TestServiceContext(); - var socketOutput = new Http1OutputProducer( - pipe.Writer, + var socketOutput = new TestHttpOutputProducer( + pipe, "0", connectionContext, serviceContext.Log, @@ -99,5 +93,15 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests return socketOutput; } + + private class TestHttpOutputProducer : Http1OutputProducer + { + public TestHttpOutputProducer(Pipe pipe, string connectionId, ConnectionContext connectionContext, IKestrelTrace log, ITimeoutControl timeoutControl, IHttpMinResponseDataRateFeature minResponseDataRateFeature) : base(pipe.Writer, connectionId, connectionContext, log, timeoutControl, minResponseDataRateFeature) + { + Pipe = pipe; + } + + public Pipe Pipe { get; } + } } } diff --git a/src/Servers/Kestrel/Transport.Libuv/test/LibuvOutputConsumerTests.cs b/src/Servers/Kestrel/Transport.Libuv/test/LibuvOutputConsumerTests.cs index 523a476238..d72e6485b5 100644 --- a/src/Servers/Kestrel/Transport.Libuv/test/LibuvOutputConsumerTests.cs +++ b/src/Servers/Kestrel/Transport.Libuv/test/LibuvOutputConsumerTests.cs @@ -304,13 +304,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Tests Assert.NotEmpty(completeQueue); // Add more bytes to the write-behind buffer to prevent the next write from - _ = outputProducer.WriteAsync((writableBuffer, state) => - { - writableBuffer.Write(state); - return state.Count; - }, - halfWriteBehindBuffer, - default); + _ = outputProducer.WriteDataAsync(halfWriteBehindBuffer, default); // Act var writeTask2 = outputProducer.WriteDataAsync(halfWriteBehindBuffer);