From 6410f365b87fe88e2d7c5f1851f04608df6a70ec Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Mon, 16 Jul 2018 18:39:32 -0700 Subject: [PATCH 1/8] Start of input flow control --- src/Kestrel.Core/Internal/Http/MessageBody.cs | 10 +++ .../Internal/Http2/Http2Connection.cs | 11 +-- .../Internal/Http2/Http2FrameWriter.cs | 32 +++---- .../Internal/Http2/Http2InputFlowControl.cs | 40 +++++++++ .../Internal/Http2/Http2MessageBody.cs | 17 ++-- .../Internal/Http2/Http2Stream.cs | 84 +++++++++++++++---- .../Internal/Http2/Http2StreamContext.cs | 1 + 7 files changed, 150 insertions(+), 45 deletions(-) create mode 100644 src/Kestrel.Core/Internal/Http2/Http2InputFlowControl.cs diff --git a/src/Kestrel.Core/Internal/Http/MessageBody.cs b/src/Kestrel.Core/Internal/Http/MessageBody.cs index 3ed2b86c5..b345df8ef 100644 --- a/src/Kestrel.Core/Internal/Http/MessageBody.cs +++ b/src/Kestrel.Core/Internal/Http/MessageBody.cs @@ -55,6 +55,9 @@ protected MessageBody(HttpProtocol context) var slice = readableBuffer.Slice(0, actual); consumed = readableBuffer.GetPosition(actual); slice.CopyTo(buffer.Span); + + OnDataRead(actual); + return actual; } @@ -89,6 +92,9 @@ protected MessageBody(HttpProtocol context) // REVIEW: This *could* be slower if 2 things are true // - The WriteAsync(ReadOnlyMemory) isn't overridden on the destination // - We change the Kestrel Memory Pool to not use pinned arrays but instead use native memory + + OnDataRead(memory.Length); + #if NETCOREAPP2_1 await destination.WriteAsync(memory); #elif NETSTANDARD2_0 @@ -150,6 +156,10 @@ protected virtual void OnReadStarted() { } + protected virtual void OnDataRead(int bytesRead) + { + } + private class ForZeroContentLength : MessageBody { public ForZeroContentLength(bool keepAlive) diff --git a/src/Kestrel.Core/Internal/Http2/Http2Connection.cs b/src/Kestrel.Core/Internal/Http2/Http2Connection.cs index a2110bee0..776189f39 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2Connection.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2Connection.cs @@ -61,6 +61,7 @@ private enum PseudoHeaderFields private readonly Http2ConnectionContext _context; private readonly Http2FrameWriter _frameWriter; private readonly HPackDecoder _hpackDecoder; + private readonly Http2InputFlowControl _inputFlowControl; private readonly Http2OutputFlowControl _outputFlowControl = new Http2OutputFlowControl(Http2PeerSettings.DefaultInitialWindowSize); private readonly Http2PeerSettings _serverSettings = new Http2PeerSettings(); @@ -83,6 +84,7 @@ public Http2Connection(Http2ConnectionContext context) _context = context; _frameWriter = new Http2FrameWriter(context.Transport.Output, context.Application.Input, _outputFlowControl, this); _hpackDecoder = new HPackDecoder((int)_serverSettings.HeaderTableSize); + _inputFlowControl = new Http2InputFlowControl(_frameWriter, 0, (int)Http2PeerSettings.DefaultInitialWindowSize / 2); } public string ConnectionId => _context.ConnectionId; @@ -367,8 +369,7 @@ private Task ProcessDataFrameAsync() throw new Http2ConnectionErrorException(CoreStrings.FormatHttp2ErrorStreamHalfClosedRemote(_incomingFrame.Type, stream.StreamId), Http2ErrorCode.STREAM_CLOSED); } - return stream.OnDataAsync(_incomingFrame.DataPayload, - endStream: (_incomingFrame.DataFlags & Http2DataFrameFlags.END_STREAM) == Http2DataFrameFlags.END_STREAM); + return stream.OnDataAsync(_incomingFrame); } // If we couldn't find the stream, it was either alive previously but closed with @@ -460,13 +461,14 @@ private async Task ProcessHeadersFrameAsync(IHttpApplication StreamLifetimeHandler = this, ClientPeerSettings = _clientSettings, FrameWriter = _frameWriter, + ConnectionInputFlowControl = _inputFlowControl, ConnectionOutputFlowControl = _outputFlowControl, TimeoutControl = this, }); if ((_incomingFrame.HeadersFlags & Http2HeadersFrameFlags.END_STREAM) == Http2HeadersFrameFlags.END_STREAM) { - await _currentHeadersStream.OnDataAsync(Constants.EmptyData, endStream: true); + _currentHeadersStream.OnEndStream(); } _currentHeadersStream.Reset(); @@ -741,9 +743,8 @@ private Task DecodeTrailersAsync(bool endHeaders, Span payload) if (endHeaders) { - var endStreamTask = _currentHeadersStream.OnDataAsync(Constants.EmptyData, endStream: true); + _currentHeadersStream.OnEndStream(); ResetRequestHeaderParsingState(); - return endStreamTask; } return Task.CompletedTask; diff --git a/src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs b/src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs index f010264d7..71ef6f740 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs @@ -86,7 +86,7 @@ public Task Write100ContinueAsync(int streamId) _outgoingFrame.Length = _continueBytes.Length; _continueBytes.CopyTo(_outgoingFrame.HeadersPayload); - return WriteUnsynchronizedAsync(_outgoingFrame.Raw); + return WriteFrameUnsynchronizedAsync(); } } @@ -190,7 +190,7 @@ private Task WriteDataUnsynchronizedAsync(int streamId, ReadOnlySequence d _outgoingFrame.Length = unwrittenPayloadLength; _outputWriter.Write(_outgoingFrame.Raw); - return FlushUnsynchronizedAsync(); + return _flusher.FlushAsync(); } private async Task WriteDataAsyncAwaited(int streamId, Http2StreamOutputFlowControl flowControl, ReadOnlySequence data, long dataLength, bool endStream) @@ -239,12 +239,21 @@ private async Task WriteDataAsyncAwaited(int streamId, Http2StreamOutputFlowCont await ThreadPoolAwaitable.Instance; } + public Task WriteWindowUpdateAsync(int streamId, int sizeIncrement) + { + lock (_writeLock) + { + _outgoingFrame.PrepareWindowUpdate(streamId, sizeIncrement); + return WriteFrameUnsynchronizedAsync(); + } + } + public Task WriteRstStreamAsync(int streamId, Http2ErrorCode errorCode) { lock (_writeLock) { _outgoingFrame.PrepareRstStream(streamId, errorCode); - return WriteUnsynchronizedAsync(_outgoingFrame.Raw); + return WriteFrameUnsynchronizedAsync(); } } @@ -254,7 +263,7 @@ public Task WriteSettingsAsync(Http2PeerSettings settings) { // TODO: actually send settings _outgoingFrame.PrepareSettings(Http2SettingsFrameFlags.NONE); - return WriteUnsynchronizedAsync(_outgoingFrame.Raw); + return WriteFrameUnsynchronizedAsync(); } } @@ -263,7 +272,7 @@ public Task WriteSettingsAckAsync() lock (_writeLock) { _outgoingFrame.PrepareSettings(Http2SettingsFrameFlags.ACK); - return WriteUnsynchronizedAsync(_outgoingFrame.Raw); + return WriteFrameUnsynchronizedAsync(); } } @@ -273,7 +282,7 @@ public Task WritePingAsync(Http2PingFrameFlags flags, ReadOnlySpan payload { _outgoingFrame.PreparePing(Http2PingFrameFlags.ACK); payload.CopyTo(_outgoingFrame.Payload); - return WriteUnsynchronizedAsync(_outgoingFrame.Raw); + return WriteFrameUnsynchronizedAsync(); } } @@ -282,23 +291,18 @@ public Task WriteGoAwayAsync(int lastStreamId, Http2ErrorCode errorCode) lock (_writeLock) { _outgoingFrame.PrepareGoAway(lastStreamId, errorCode); - return WriteUnsynchronizedAsync(_outgoingFrame.Raw); + return WriteFrameUnsynchronizedAsync(); } } - private Task WriteUnsynchronizedAsync(ReadOnlySpan data) + private Task WriteFrameUnsynchronizedAsync() { if (_completed) { return Task.CompletedTask; } - _outputWriter.Write(data); - return FlushUnsynchronizedAsync(); - } - - private Task FlushUnsynchronizedAsync() - { + _outputWriter.Write(_outgoingFrame.Raw); return _flusher.FlushAsync(); } diff --git a/src/Kestrel.Core/Internal/Http2/Http2InputFlowControl.cs b/src/Kestrel.Core/Internal/Http2/Http2InputFlowControl.cs new file mode 100644 index 000000000..afb0c98fa --- /dev/null +++ b/src/Kestrel.Core/Internal/Http2/Http2InputFlowControl.cs @@ -0,0 +1,40 @@ +// 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.Diagnostics; + +namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 +{ + public class Http2InputFlowControl + { + private readonly Http2FrameWriter _frameWriter; + private readonly int _streamId; + private readonly uint _minWindowSizeIncrement; + + private int _unconfirmedBytesRead; + + public Http2InputFlowControl( + Http2FrameWriter frameWriter, + int streamId, + uint minWindowSizeIncrement) + { + Debug.Assert(minWindowSizeIncrement <= Http2PeerSettings.MaxWindowSize, $"{nameof(minWindowSizeIncrement)} too large."); + + _frameWriter = frameWriter; + _streamId = streamId; + _minWindowSizeIncrement = minWindowSizeIncrement; + } + + + public void OnDataRead(int bytes) + { + _unconfirmedBytesRead += bytes; + + if (_unconfirmedBytesRead > _minWindowSizeIncrement) + { + _frameWriter.WriteWindowUpdateAsync(_streamId, _unconfirmedBytesRead); + _unconfirmedBytesRead = 0; + } + } + } +} diff --git a/src/Kestrel.Core/Internal/Http2/Http2MessageBody.cs b/src/Kestrel.Core/Internal/Http2/Http2MessageBody.cs index b6ad3af16..29d6fe4e9 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2MessageBody.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2MessageBody.cs @@ -6,7 +6,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 { - public abstract class Http2MessageBody : MessageBody + public class Http2MessageBody : MessageBody { private readonly Http2Stream _context; @@ -25,6 +25,11 @@ protected override void OnReadStarted() } } + protected override void OnDataRead(int bytesRead) + { + _context.OnDataReadByApp(bytesRead); + } + protected override Task OnConsumeAsync() => Task.CompletedTask; public override Task StopAsync() @@ -43,15 +48,7 @@ public static MessageBody For( return ZeroContentLengthClose; } - return new ForHttp2(context); - } - - private class ForHttp2 : Http2MessageBody - { - public ForHttp2(Http2Stream context) - : base(context) - { - } + return new Http2MessageBody(context); } } } diff --git a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs index 85cb88d5d..bb6408a1a 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs @@ -23,13 +23,20 @@ public partial class Http2Stream : HttpProtocol { private readonly Http2StreamContext _context; private readonly Http2OutputProducer _http2Output; + private readonly Http2InputFlowControl _inputFlowControl; private readonly Http2StreamOutputFlowControl _outputFlowControl; + + private long _totalBytesReceived; + private long _totalBytesReadByApp; + private int _requestAborted; public Http2Stream(Http2StreamContext context) : base(context) { _context = context; + + _inputFlowControl = new Http2InputFlowControl(_context.FrameWriter, _context.StreamId, (int)Http2PeerSettings.DefaultInitialWindowSize / 2); _outputFlowControl = new Http2StreamOutputFlowControl(context.ConnectionOutputFlowControl, context.ClientPeerSettings.InitialWindowSize); _http2Output = new Http2OutputProducer(context.StreamId, context.FrameWriter, _outputFlowControl, context.TimeoutControl, context.MemoryPool); Output = _http2Output; @@ -40,8 +47,6 @@ public Http2Stream(Http2StreamContext context) public bool RequestBodyStarted { get; private set; } public bool EndStreamReceived { get; private set; } - protected IHttp2StreamLifetimeHandler StreamLifetimeHandler => _context.StreamLifetimeHandler; - public override bool IsUpgradableRequest => false; protected override void OnReset() @@ -51,7 +56,7 @@ protected override void OnReset() protected override void OnRequestProcessingEnded() { - StreamLifetimeHandler.OnStreamCompleted(StreamId); + _context.StreamLifetimeHandler.OnStreamCompleted(StreamId); } protected override string CreateRequestId() @@ -246,30 +251,71 @@ private bool TryValidatePath(ReadOnlySpan pathSegment) } } - public async Task OnDataAsync(ArraySegment data, bool endStream) + public Task OnDataAsync(Http2Frame dataFrame) { // TODO: content-length accounting - // TODO: flow-control - try + var payload = dataFrame.DataPayload; + var endStream = (dataFrame.DataFlags & Http2DataFrameFlags.END_STREAM) == Http2DataFrameFlags.END_STREAM; + var flushTask = default(ValueTask); + + // TODO: Abort connection if _totalBytesReceived - _totalBytesReadByApp > Http2PeerSettings.DefaultInitialWindowSize. + _totalBytesReceived += dataFrame.Length; + + // Since padding isn't buffered, immediately count padding bytes as read for flow control purposes. + if (dataFrame.DataHasPadding) { - if (data.Count > 0) - { - RequestBodyPipe.Writer.Write(data); + // Add 1 byte for the padding length prefix. + OnDataReadByApp(dataFrame.DataPadLength + 1); + } - RequestBodyStarted = true; - await RequestBodyPipe.Writer.FlushAsync(); - } + if (payload.Count > 0) + { + RequestBodyPipe.Writer.Write(payload); + + RequestBodyStarted = true; + flushTask = RequestBodyPipe.Writer.FlushAsync(); + } + if (flushTask.IsCompleted) + { if (endStream) { - EndStreamReceived = true; - RequestBodyPipe.Writer.Complete(); + OnEndStream(); } + + return Task.CompletedTask; } - catch (Exception ex) + + return OnDataAsyncAwaited(flushTask, endStream); + } + + private async Task OnDataAsyncAwaited(ValueTask flushTask, bool endStream) + { + await flushTask; + + if (endStream) + { + OnEndStream(); + } + } + + public void OnEndStream() + { + EndStreamReceived = true; + RequestBodyPipe.Writer.Complete(); + } + + public void OnDataReadByApp(int bytesRead) + { + _totalBytesReadByApp += bytesRead; + + _inputFlowControl.OnDataRead(bytesRead); + + // TODO: This is bad, and I feel bad. + lock (_context.ConnectionInputFlowControl) { - RequestBodyPipe.Writer.Complete(ex); + _context.ConnectionInputFlowControl.OnDataRead(bytesRead); } } @@ -315,6 +361,12 @@ private void AbortCore(ConnectionAbortedException abortReason) // Unblock the request body. RequestBodyPipe.Writer.Complete(new IOException(CoreStrings.Http2StreamAborted, abortReason)); + + // Count all data for the stream as read for the purposes of connection-level flow control. + if (_totalBytesReceived > _totalBytesReadByApp) + { + _context.ConnectionInputFlowControl.OnDataRead((int)(_totalBytesReceived - _totalBytesReadByApp)); + } } } } diff --git a/src/Kestrel.Core/Internal/Http2/Http2StreamContext.cs b/src/Kestrel.Core/Internal/Http2/Http2StreamContext.cs index d8d24144a..a91629e18 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2StreamContext.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2StreamContext.cs @@ -21,6 +21,7 @@ public class Http2StreamContext : IHttpProtocolContext public IHttp2StreamLifetimeHandler StreamLifetimeHandler { get; set; } public Http2PeerSettings ClientPeerSettings { get; set; } public Http2FrameWriter FrameWriter { get; set; } + public Http2InputFlowControl ConnectionInputFlowControl { get; set; } public Http2OutputFlowControl ConnectionOutputFlowControl { get; set; } public ITimeoutControl TimeoutControl { get; set; } } From 75c30ee67434c15e3235c10356b94841486464b7 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 17 Jul 2018 19:59:51 -0700 Subject: [PATCH 2/8] Clean up input flow control --- src/Kestrel.Core/CoreStrings.resx | 3 + .../Internal/Http/Http1Connection.cs | 13 +++ .../Internal/Http/HttpProtocol.cs | 15 +-- src/Kestrel.Core/Internal/Http/MessageBody.cs | 18 +++- .../Http2FlowControl.cs} | 32 +------ .../FlowControl/Http2InputFlowControl.cs | 93 +++++++++++++++++++ .../FlowControl/Http2OutputFlowControl.cs | 74 +++++++++++++++ .../Http2OutputFlowControlAwaitable.cs | 0 .../Http2StreamInputFlowControl.cs | 86 +++++++++++++++++ .../Http2StreamOutputFlowControl.cs | 0 .../Internal/Http2/Http2Connection.cs | 2 +- .../Internal/Http2/Http2InputFlowControl.cs | 40 -------- .../Internal/Http2/Http2MessageBody.cs | 4 +- .../Internal/Http2/Http2Stream.cs | 79 +++++++--------- .../Properties/CoreStrings.Designer.cs | 14 +++ 15 files changed, 336 insertions(+), 137 deletions(-) rename src/Kestrel.Core/Internal/Http2/{Http2OutputFlowControl.cs => FlowControl/Http2FlowControl.cs} (57%) create mode 100644 src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs create mode 100644 src/Kestrel.Core/Internal/Http2/FlowControl/Http2OutputFlowControl.cs rename src/Kestrel.Core/Internal/Http2/{ => FlowControl}/Http2OutputFlowControlAwaitable.cs (100%) create mode 100644 src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamInputFlowControl.cs rename src/Kestrel.Core/Internal/Http2/{ => FlowControl}/Http2StreamOutputFlowControl.cs (100%) delete mode 100644 src/Kestrel.Core/Internal/Http2/Http2InputFlowControl.cs diff --git a/src/Kestrel.Core/CoreStrings.resx b/src/Kestrel.Core/CoreStrings.resx index 6ef16c78f..fd9344216 100644 --- a/src/Kestrel.Core/CoreStrings.resx +++ b/src/Kestrel.Core/CoreStrings.resx @@ -545,6 +545,9 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l The request stream was aborted. + + The client sent more data than what was available in the flow-control window. + CONNECT requests must not send :scheme or :path headers. diff --git a/src/Kestrel.Core/Internal/Http/Http1Connection.cs b/src/Kestrel.Core/Internal/Http/Http1Connection.cs index af32fdf01..68c6187c4 100644 --- a/src/Kestrel.Core/Internal/Http/Http1Connection.cs +++ b/src/Kestrel.Core/Internal/Http/Http1Connection.cs @@ -44,6 +44,7 @@ public Http1Connection(Http1ConnectionContext context) _keepAliveTicks = ServerOptions.Limits.KeepAliveTimeout.Ticks; _requestHeadersTimeoutTicks = ServerOptions.Limits.RequestHeadersTimeout.Ticks; + RequestBodyPipe = CreateRequestBodyPipe(); Output = new Http1OutputProducer( _context.Transport.Output, _context.ConnectionId, @@ -470,5 +471,17 @@ protected override bool TryParseRequest(ReadResult result, out bool endConnectio return false; } } + + private Pipe CreateRequestBodyPipe() + => new Pipe(new PipeOptions + ( + pool: _context.MemoryPool, + readerScheduler: ServiceContext.Scheduler, + writerScheduler: PipeScheduler.Inline, + pauseWriterThreshold: 1, + resumeWriterThreshold: 1, + useSynchronizationContext: false, + minimumSegmentSize: KestrelMemoryPool.MinimumSegmentSize + )); } } diff --git a/src/Kestrel.Core/Internal/Http/HttpProtocol.cs b/src/Kestrel.Core/Internal/Http/HttpProtocol.cs index ccd409ce7..62aa16ef6 100644 --- a/src/Kestrel.Core/Internal/Http/HttpProtocol.cs +++ b/src/Kestrel.Core/Internal/Http/HttpProtocol.cs @@ -73,12 +73,11 @@ public HttpProtocol(IHttpProtocolContext context) ServerOptions = ServiceContext.ServerOptions; HttpResponseControl = this; - RequestBodyPipe = CreateRequestBodyPipe(); } public IHttpResponseControl HttpResponseControl { get; set; } - public Pipe RequestBodyPipe { get; } + public Pipe RequestBodyPipe { get; protected set; } public ServiceContext ServiceContext => _context.ServiceContext; private IPEndPoint LocalEndPoint => _context.LocalEndPoint; @@ -1332,17 +1331,5 @@ protected void ReportApplicationError(Exception ex) Log.ApplicationError(ConnectionId, TraceIdentifier, ex); } - - private Pipe CreateRequestBodyPipe() - => new Pipe(new PipeOptions - ( - pool: _context.MemoryPool, - readerScheduler: ServiceContext.Scheduler, - writerScheduler: PipeScheduler.Inline, - pauseWriterThreshold: 1, - resumeWriterThreshold: 1, - useSynchronizationContext: false, - minimumSegmentSize: KestrelMemoryPool.MinimumSegmentSize - )); } } diff --git a/src/Kestrel.Core/Internal/Http/MessageBody.cs b/src/Kestrel.Core/Internal/Http/MessageBody.cs index b345df8ef..0cbf0e0ea 100644 --- a/src/Kestrel.Core/Internal/Http/MessageBody.cs +++ b/src/Kestrel.Core/Internal/Http/MessageBody.cs @@ -45,19 +45,18 @@ protected MessageBody(HttpProtocol context) var result = await _context.RequestBodyPipe.Reader.ReadAsync(); var readableBuffer = result.Buffer; var consumed = readableBuffer.End; + var actual = 0; try { if (!readableBuffer.IsEmpty) { - // buffer.Count is int - var actual = (int)Math.Min(readableBuffer.Length, buffer.Length); + // buffer.Count is int + actual = (int)Math.Min(readableBuffer.Length, buffer.Length); var slice = readableBuffer.Slice(0, actual); consumed = readableBuffer.GetPosition(actual); slice.CopyTo(buffer.Span); - OnDataRead(actual); - return actual; } @@ -69,6 +68,10 @@ protected MessageBody(HttpProtocol context) finally { _context.RequestBodyPipe.Reader.AdvanceTo(consumed); + + // Update the flow-control window after advancing the pipe reader, so we don't risk overfilling + // the pipe despite the client being well-behaved. + OnDataRead(actual); } } } @@ -82,6 +85,7 @@ protected MessageBody(HttpProtocol context) var result = await _context.RequestBodyPipe.Reader.ReadAsync(); var readableBuffer = result.Buffer; var consumed = readableBuffer.End; + var bytesRead = 0; try { @@ -93,7 +97,7 @@ protected MessageBody(HttpProtocol context) // - The WriteAsync(ReadOnlyMemory) isn't overridden on the destination // - We change the Kestrel Memory Pool to not use pinned arrays but instead use native memory - OnDataRead(memory.Length); + bytesRead += memory.Length; #if NETCOREAPP2_1 await destination.WriteAsync(memory); @@ -114,6 +118,10 @@ protected MessageBody(HttpProtocol context) finally { _context.RequestBodyPipe.Reader.AdvanceTo(consumed); + + // Update the flow-control window after advancing the pipe reader, so we don't risk overfilling + // the pipe despite the client being well-behaved. + OnDataRead(bytesRead); } } } diff --git a/src/Kestrel.Core/Internal/Http2/Http2OutputFlowControl.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2FlowControl.cs similarity index 57% rename from src/Kestrel.Core/Internal/Http2/Http2OutputFlowControl.cs rename to src/Kestrel.Core/Internal/Http2/FlowControl/Http2FlowControl.cs index 4e697cb10..eee363c32 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2OutputFlowControl.cs +++ b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2FlowControl.cs @@ -1,38 +1,23 @@ // 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.Collections.Generic; using System.Diagnostics; namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 { - public class Http2OutputFlowControl + public struct Http2FlowControl { - private readonly Queue _awaitableQueue = new Queue(); - - public Http2OutputFlowControl(uint initialWindowSize) + public Http2FlowControl(uint initialWindowSize) { Debug.Assert(initialWindowSize <= Http2PeerSettings.MaxWindowSize, $"{nameof(initialWindowSize)} too large."); Available = (int)initialWindowSize; + IsAborted = false; } public int Available { get; private set; } public bool IsAborted { get; private set; } - public Http2OutputFlowControlAwaitable AvailabilityAwaitable - { - get - { - Debug.Assert(!IsAborted, $"({nameof(AvailabilityAwaitable)} accessed after abort."); - Debug.Assert(Available <= 0, $"({nameof(AvailabilityAwaitable)} accessed with {Available} bytes available."); - - var awaitable = new Http2OutputFlowControlAwaitable(); - _awaitableQueue.Enqueue(awaitable); - return awaitable; - } - } - public void Advance(int bytes) { Debug.Assert(!IsAborted, $"({nameof(Advance)} called after abort."); @@ -55,23 +40,12 @@ public bool TryUpdateWindow(int bytes) Available += bytes; - while (Available > 0 && _awaitableQueue.Count > 0) - { - var awaitable = _awaitableQueue.Dequeue(); - awaitable.Complete(); - } - return true; } public void Abort() { IsAborted = true; - - while (_awaitableQueue.Count > 0) - { - _awaitableQueue.Dequeue().Complete(); - } } } } diff --git a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs new file mode 100644 index 000000000..7dc1ddd45 --- /dev/null +++ b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs @@ -0,0 +1,93 @@ +// 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.Diagnostics; + +namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 +{ + public class Http2InputFlowControl + { + private readonly Http2FlowControl _flow; + private readonly int _initialWindowSize; + private readonly int _minWindowSizeIncrement; + + private int _unconfirmedBytes; + + private readonly object _flowLock = new object(); + + public Http2InputFlowControl(uint initialWindowSize) + { + _flow = new Http2FlowControl(initialWindowSize); + _initialWindowSize = (int)initialWindowSize; + _minWindowSizeIncrement = _initialWindowSize / 2; + } + + public bool TryAdvance(int bytes) + { + lock (_flowLock) + { + // Even if the stream is aborted, the client should never send more data than was available in the + // flow-control window at the time of the abort. + if (bytes > _flow.Available) + { + throw new Http2ConnectionErrorException(CoreStrings.Http2ErrorFlowControlWindowExceeded, Http2ErrorCode.FLOW_CONTROL_ERROR); + } + + // This data won't be read by the app, so tell the caller to count the data as already consumed. + if (_flow.IsAborted) + { + return false; + } + + _flow.Advance(bytes); + return true; + } + } + + public bool TryUpdateWindow(int bytes, out int updateSize) + { + updateSize = 0; + + lock (_flowLock) + { + if (_flow.IsAborted) + { + // All data received by stream has already been returned to the connection window. + return false; + } + + if (!_flow.TryUpdateWindow(bytes)) + { + // We only try to update the window back to its initial size after the app consumes data. + // It shouldn't be possible for the window size to ever exceed Http2PeerSettings.MaxWindowSize. + Debug.Assert(false, $"{nameof(TryUpdateWindow)} attempted to grow window past max size."); + } + + var potentialUpdateSize = _unconfirmedBytes + bytes; + + if (potentialUpdateSize > _minWindowSizeIncrement) + { + _unconfirmedBytes = 0; + updateSize = potentialUpdateSize; + } + else + { + _unconfirmedBytes = potentialUpdateSize; + } + + return true; + } + } + + public int Abort() + { + lock (_flowLock) + { + _flow.Abort(); + + // Tell caller to return connection window space consumed by this stream. + return _initialWindowSize - _flow.Available; + } + } + } +} diff --git a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2OutputFlowControl.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2OutputFlowControl.cs new file mode 100644 index 000000000..93f5cb213 --- /dev/null +++ b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2OutputFlowControl.cs @@ -0,0 +1,74 @@ +// 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.Collections.Generic; +using System.Diagnostics; + +namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 +{ + public class Http2OutputFlowControl + { + private Http2FlowControl _flow; + private Queue _awaitableQueue; + + public Http2OutputFlowControl(uint initialWindowSize) + { + _flow = new Http2FlowControl(initialWindowSize); + } + + public int Available => _flow.Available; + public bool IsAborted => _flow.IsAborted; + + public Http2OutputFlowControlAwaitable AvailabilityAwaitable + { + get + { + Debug.Assert(!_flow.IsAborted, $"({nameof(AvailabilityAwaitable)} accessed after abort."); + Debug.Assert(_flow.Available <= 0, $"({nameof(AvailabilityAwaitable)} accessed with {Available} bytes available."); + + if (_awaitableQueue == null) + { + _awaitableQueue = new Queue(); + } + + var awaitable = new Http2OutputFlowControlAwaitable(); + _awaitableQueue.Enqueue(awaitable); + return awaitable; + } + } + + public void Advance(int bytes) + { + _flow.Advance(bytes); + } + + // bytes can be negative when SETTINGS_INITIAL_WINDOW_SIZE decreases mid-connection. + // This can also cause Available to become negative which MUST be allowed. + // https://httpwg.org/specs/rfc7540.html#rfc.section.6.9.2 + public bool TryUpdateWindow(int bytes) + { + if (_flow.TryUpdateWindow(bytes)) + { + while (_flow.Available > 0 && _awaitableQueue?.Count > 0) + { + _awaitableQueue.Dequeue().Complete(); + } + + return true; + } + + return false; + } + + public void Abort() + { + // Make sure to set the aborted flag before running any continuations. + _flow.Abort(); + + while (_awaitableQueue?.Count > 0) + { + _awaitableQueue.Dequeue().Complete(); + } + } + } +} diff --git a/src/Kestrel.Core/Internal/Http2/Http2OutputFlowControlAwaitable.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2OutputFlowControlAwaitable.cs similarity index 100% rename from src/Kestrel.Core/Internal/Http2/Http2OutputFlowControlAwaitable.cs rename to src/Kestrel.Core/Internal/Http2/FlowControl/Http2OutputFlowControlAwaitable.cs diff --git a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamInputFlowControl.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamInputFlowControl.cs new file mode 100644 index 000000000..813eb28ce --- /dev/null +++ b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamInputFlowControl.cs @@ -0,0 +1,86 @@ +// 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.Diagnostics; + +namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 +{ + public class Http2StreamInputFlowControl + { + private readonly Http2InputFlowControl _connectionLevelFlowControl; + private readonly Http2InputFlowControl _streamLevelFlowControl; + + private readonly int _streamId; + private readonly Http2FrameWriter _frameWriter; + + public Http2StreamInputFlowControl( + int streamId, + Http2FrameWriter frameWriter, + Http2InputFlowControl connectionLevelFlowControl, + uint initialWindowSize) + { + _connectionLevelFlowControl = connectionLevelFlowControl; + _streamLevelFlowControl = new Http2InputFlowControl(initialWindowSize); + + _streamId = streamId; + _frameWriter = frameWriter; + } + + public void Advance(int bytes) + { + var connectionSucess = _connectionLevelFlowControl.TryAdvance(bytes); + + Debug.Assert(connectionSucess, "Connection-level input flow control should never be aborted."); + + if (!_streamLevelFlowControl.TryAdvance(bytes)) + { + // The stream has already been aborted, so immediately count the bytes as read at the connection level. + UpdateConnectionWindow(bytes); + } + } + + public void UpdateWindows(int bytes) + { + if (!_streamLevelFlowControl.TryUpdateWindow(bytes, out var streamWindowUpdateSize)) + { + // Stream-level flow control was aborted. Any unread bytes have already been returned to the connection + // flow-control window by Abort(). + return; + } + + if (streamWindowUpdateSize > 0) + { + // Writing with the FrameWriter should only fail if given a canceled token, so just fire and forget. + _ = _frameWriter.WriteWindowUpdateAsync(_streamId, streamWindowUpdateSize); + } + + UpdateConnectionWindow(bytes); + } + + public void Abort() + { + var unreadBytes = _streamLevelFlowControl.Abort(); + + if (unreadBytes > 0) + { + // We assume that the app won't read the remaining data from the request body pipe. + // Even if the app does continue reading, _streamLevelFlowControl.TryUpdateWindow() will return false + // from now on which prevents double counting. + UpdateConnectionWindow(unreadBytes); + } + } + + private void UpdateConnectionWindow(int bytes) + { + var connectionSucess = _connectionLevelFlowControl.TryUpdateWindow(bytes, out var connectionWindowUpdateSize); + + Debug.Assert(connectionSucess, "Connection-level input flow control should never be aborted."); + + if (connectionWindowUpdateSize > 0) + { + // Writing with the FrameWriter should only fail if given a canceled token, so just fire and forget. + _ = _frameWriter.WriteWindowUpdateAsync(0, connectionWindowUpdateSize); + } + } + } +} diff --git a/src/Kestrel.Core/Internal/Http2/Http2StreamOutputFlowControl.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamOutputFlowControl.cs similarity index 100% rename from src/Kestrel.Core/Internal/Http2/Http2StreamOutputFlowControl.cs rename to src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamOutputFlowControl.cs diff --git a/src/Kestrel.Core/Internal/Http2/Http2Connection.cs b/src/Kestrel.Core/Internal/Http2/Http2Connection.cs index 776189f39..c6993ef96 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2Connection.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2Connection.cs @@ -84,7 +84,7 @@ public Http2Connection(Http2ConnectionContext context) _context = context; _frameWriter = new Http2FrameWriter(context.Transport.Output, context.Application.Input, _outputFlowControl, this); _hpackDecoder = new HPackDecoder((int)_serverSettings.HeaderTableSize); - _inputFlowControl = new Http2InputFlowControl(_frameWriter, 0, (int)Http2PeerSettings.DefaultInitialWindowSize / 2); + _inputFlowControl = new Http2InputFlowControl(Http2PeerSettings.DefaultInitialWindowSize / 2); } public string ConnectionId => _context.ConnectionId; diff --git a/src/Kestrel.Core/Internal/Http2/Http2InputFlowControl.cs b/src/Kestrel.Core/Internal/Http2/Http2InputFlowControl.cs deleted file mode 100644 index afb0c98fa..000000000 --- a/src/Kestrel.Core/Internal/Http2/Http2InputFlowControl.cs +++ /dev/null @@ -1,40 +0,0 @@ -// 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.Diagnostics; - -namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 -{ - public class Http2InputFlowControl - { - private readonly Http2FrameWriter _frameWriter; - private readonly int _streamId; - private readonly uint _minWindowSizeIncrement; - - private int _unconfirmedBytesRead; - - public Http2InputFlowControl( - Http2FrameWriter frameWriter, - int streamId, - uint minWindowSizeIncrement) - { - Debug.Assert(minWindowSizeIncrement <= Http2PeerSettings.MaxWindowSize, $"{nameof(minWindowSizeIncrement)} too large."); - - _frameWriter = frameWriter; - _streamId = streamId; - _minWindowSizeIncrement = minWindowSizeIncrement; - } - - - public void OnDataRead(int bytes) - { - _unconfirmedBytesRead += bytes; - - if (_unconfirmedBytesRead > _minWindowSizeIncrement) - { - _frameWriter.WriteWindowUpdateAsync(_streamId, _unconfirmedBytesRead); - _unconfirmedBytesRead = 0; - } - } - } -} diff --git a/src/Kestrel.Core/Internal/Http2/Http2MessageBody.cs b/src/Kestrel.Core/Internal/Http2/Http2MessageBody.cs index 29d6fe4e9..5f2907c21 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2MessageBody.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2MessageBody.cs @@ -27,7 +27,7 @@ protected override void OnReadStarted() protected override void OnDataRead(int bytesRead) { - _context.OnDataReadByApp(bytesRead); + _context.OnDataRead(bytesRead); } protected override Task OnConsumeAsync() => Task.CompletedTask; @@ -43,7 +43,7 @@ public static MessageBody For( HttpRequestHeaders headers, Http2Stream context) { - if (context.EndStreamReceived) + if (context.EndStreamReceived && !context.RequestBodyStarted) { return ZeroContentLengthClose; } diff --git a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs index bb6408a1a..2cb5da902 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs @@ -14,6 +14,7 @@ using Microsoft.AspNetCore.Connections.Abstractions; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure; +using Microsoft.AspNetCore.Server.Kestrel.Transport.Abstractions.Internal; using Microsoft.Extensions.Primitives; using Microsoft.Net.Http.Headers; @@ -23,12 +24,9 @@ public partial class Http2Stream : HttpProtocol { private readonly Http2StreamContext _context; private readonly Http2OutputProducer _http2Output; - private readonly Http2InputFlowControl _inputFlowControl; + private readonly Http2StreamInputFlowControl _inputFlowControl; private readonly Http2StreamOutputFlowControl _outputFlowControl; - private long _totalBytesReceived; - private long _totalBytesReadByApp; - private int _requestAborted; public Http2Stream(Http2StreamContext context) @@ -36,9 +34,11 @@ public Http2Stream(Http2StreamContext context) { _context = context; - _inputFlowControl = new Http2InputFlowControl(_context.FrameWriter, _context.StreamId, (int)Http2PeerSettings.DefaultInitialWindowSize / 2); + _inputFlowControl = new Http2StreamInputFlowControl(_context.StreamId, _context.FrameWriter, context.ConnectionInputFlowControl, Http2PeerSettings.DefaultInitialWindowSize); _outputFlowControl = new Http2StreamOutputFlowControl(context.ConnectionOutputFlowControl, context.ClientPeerSettings.InitialWindowSize); _http2Output = new Http2OutputProducer(context.StreamId, context.FrameWriter, _outputFlowControl, context.TimeoutControl, context.MemoryPool); + + RequestBodyPipe = CreateRequestBodyPipe(); Output = _http2Output; } @@ -255,49 +255,35 @@ public Task OnDataAsync(Http2Frame dataFrame) { // TODO: content-length accounting - var payload = dataFrame.DataPayload; - var endStream = (dataFrame.DataFlags & Http2DataFrameFlags.END_STREAM) == Http2DataFrameFlags.END_STREAM; - var flushTask = default(ValueTask); - - // TODO: Abort connection if _totalBytesReceived - _totalBytesReadByApp > Http2PeerSettings.DefaultInitialWindowSize. - _totalBytesReceived += dataFrame.Length; - // Since padding isn't buffered, immediately count padding bytes as read for flow control purposes. if (dataFrame.DataHasPadding) { // Add 1 byte for the padding length prefix. - OnDataReadByApp(dataFrame.DataPadLength + 1); + OnDataRead(dataFrame.DataPadLength + 1); } + var payload = dataFrame.DataPayload; + if (payload.Count > 0) { + _inputFlowControl.Advance(payload.Count); + RequestBodyPipe.Writer.Write(payload); RequestBodyStarted = true; - flushTask = RequestBodyPipe.Writer.FlushAsync(); - } - - if (flushTask.IsCompleted) - { - if (endStream) - { - OnEndStream(); - } + var flushTask = RequestBodyPipe.Writer.FlushAsync(); - return Task.CompletedTask; + // It shouldn't be possible for the RequestBodyPipe to fill up an return an incomplete task if + // _inputFlowControl.Advance() didn't throw. + Debug.Assert(flushTask.IsCompleted); } - return OnDataAsyncAwaited(flushTask, endStream); - } - - private async Task OnDataAsyncAwaited(ValueTask flushTask, bool endStream) - { - await flushTask; - - if (endStream) + if ((dataFrame.DataFlags & Http2DataFrameFlags.END_STREAM) == Http2DataFrameFlags.END_STREAM) { OnEndStream(); } + + return Task.CompletedTask; } public void OnEndStream() @@ -306,17 +292,9 @@ public void OnEndStream() RequestBodyPipe.Writer.Complete(); } - public void OnDataReadByApp(int bytesRead) + public void OnDataRead(int bytesRead) { - _totalBytesReadByApp += bytesRead; - - _inputFlowControl.OnDataRead(bytesRead); - - // TODO: This is bad, and I feel bad. - lock (_context.ConnectionInputFlowControl) - { - _context.ConnectionInputFlowControl.OnDataRead(bytesRead); - } + _inputFlowControl.UpdateWindows(bytesRead); } public bool TryUpdateOutputWindow(int bytes) @@ -362,11 +340,20 @@ private void AbortCore(ConnectionAbortedException abortReason) // Unblock the request body. RequestBodyPipe.Writer.Complete(new IOException(CoreStrings.Http2StreamAborted, abortReason)); - // Count all data for the stream as read for the purposes of connection-level flow control. - if (_totalBytesReceived > _totalBytesReadByApp) - { - _context.ConnectionInputFlowControl.OnDataRead((int)(_totalBytesReceived - _totalBytesReadByApp)); - } + // Stop counting any buffered bytes for this stream towards the connection input flow-control window. + _inputFlowControl.Abort(); } + + private Pipe CreateRequestBodyPipe() + => new Pipe(new PipeOptions + ( + pool: _context.MemoryPool, + readerScheduler: ServiceContext.Scheduler, + writerScheduler: PipeScheduler.Inline, + pauseWriterThreshold: Http2PeerSettings.DefaultInitialWindowSize, + resumeWriterThreshold: Http2PeerSettings.DefaultInitialWindowSize, + useSynchronizationContext: false, + minimumSegmentSize: KestrelMemoryPool.MinimumSegmentSize + )); } } diff --git a/src/Kestrel.Core/Properties/CoreStrings.Designer.cs b/src/Kestrel.Core/Properties/CoreStrings.Designer.cs index 60354b61e..857db81bf 100644 --- a/src/Kestrel.Core/Properties/CoreStrings.Designer.cs +++ b/src/Kestrel.Core/Properties/CoreStrings.Designer.cs @@ -2002,6 +2002,20 @@ internal static string Http2StreamAborted internal static string FormatHttp2StreamAborted() => GetString("Http2StreamAborted"); + /// + /// The client sent more data than what was available in the flow-control window. + /// + internal static string Http2ErrorFlowControlWindowExceeded + { + get => GetString("Http2ErrorFlowControlWindowExceeded"); + } + + /// + /// The client sent more data than what was available in the flow-control window. + /// + internal static string FormatHttp2ErrorFlowControlWindowExceeded() + => GetString("Http2ErrorFlowControlWindowExceeded"); + /// /// CONNECT requests must not send :scheme or :path headers. /// From b02c70b8a4125e6d72fffbab208077fd96f21821 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 18 Jul 2018 12:25:24 -0700 Subject: [PATCH 3/8] WIP --- src/Kestrel.Core/Internal/Http2/Http2MessageBody.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Kestrel.Core/Internal/Http2/Http2MessageBody.cs b/src/Kestrel.Core/Internal/Http2/Http2MessageBody.cs index 5f2907c21..9c423da75 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2MessageBody.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2MessageBody.cs @@ -30,6 +30,7 @@ protected override void OnDataRead(int bytesRead) _context.OnDataRead(bytesRead); } + // REVIEW: Can an app partially consume the request body in a non-aborted stream? Write a test. protected override Task OnConsumeAsync() => Task.CompletedTask; public override Task StopAsync() From 1750296543d90443b9caeed59c379a18698a6ab0 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 19 Jul 2018 19:35:52 -0700 Subject: [PATCH 4/8] Add tests and make minor fixes + improvements --- .../FlowControl/Http2InputFlowControl.cs | 5 +- .../Internal/Http2/Http2Connection.cs | 9 +- .../Internal/Http2/Http2FrameWriter.cs | 16 + .../Internal/Http2/Http2MessageBody.cs | 9 +- .../Internal/Http2/Http2Stream.cs | 6 +- .../Http2ConnectionTests.cs | 294 +++++++++++++++++- 6 files changed, 313 insertions(+), 26 deletions(-) diff --git a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs index 7dc1ddd45..333839526 100644 --- a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs +++ b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs @@ -7,14 +7,13 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 { public class Http2InputFlowControl { - private readonly Http2FlowControl _flow; private readonly int _initialWindowSize; private readonly int _minWindowSizeIncrement; + private readonly object _flowLock = new object(); + private Http2FlowControl _flow; private int _unconfirmedBytes; - private readonly object _flowLock = new object(); - public Http2InputFlowControl(uint initialWindowSize) { _flow = new Http2FlowControl(initialWindowSize); diff --git a/src/Kestrel.Core/Internal/Http2/Http2Connection.cs b/src/Kestrel.Core/Internal/Http2/Http2Connection.cs index c6993ef96..409ee1328 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2Connection.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2Connection.cs @@ -84,7 +84,7 @@ public Http2Connection(Http2ConnectionContext context) _context = context; _frameWriter = new Http2FrameWriter(context.Transport.Output, context.Application.Input, _outputFlowControl, this); _hpackDecoder = new HPackDecoder((int)_serverSettings.HeaderTableSize); - _inputFlowControl = new Http2InputFlowControl(Http2PeerSettings.DefaultInitialWindowSize / 2); + _inputFlowControl = new Http2InputFlowControl(Http2PeerSettings.DefaultInitialWindowSize); } public string ConnectionId => _context.ConnectionId; @@ -202,6 +202,9 @@ public async Task ProcessRequestsAsync(IHttpApplication appl try { + // Ensure aborting each stream doesn't result in unnecessary WINDOW_UPDATE frames being sent. + _frameWriter.DisableWindowUpdates(); + foreach (var stream in _streams.Values) { stream.Abort(connectionError); @@ -468,7 +471,7 @@ private async Task ProcessHeadersFrameAsync(IHttpApplication if ((_incomingFrame.HeadersFlags & Http2HeadersFrameFlags.END_STREAM) == Http2HeadersFrameFlags.END_STREAM) { - _currentHeadersStream.OnEndStream(); + _currentHeadersStream.OnEndStreamReceived(); } _currentHeadersStream.Reset(); @@ -743,7 +746,7 @@ private Task DecodeTrailersAsync(bool endHeaders, Span payload) if (endHeaders) { - _currentHeadersStream.OnEndStream(); + _currentHeadersStream.OnEndStreamReceived(); ResetRequestHeaderParsingState(); } diff --git a/src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs b/src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs index 71ef6f740..e46f80424 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs @@ -29,6 +29,7 @@ public class Http2FrameWriter private readonly StreamSafePipeFlusher _flusher; private bool _completed; + private bool _windowUpdatesDisabled; public Http2FrameWriter( PipeWriter outputPipeWriter, @@ -65,6 +66,16 @@ public void Abort(ConnectionAbortedException ex) Complete(); } + public void DisableWindowUpdates() + { + lock (_writeLock) + { + // This is called before aborting each stream during connection teardown in order to avoid + // sending unnecessary window updates right before closing the connection. + _windowUpdatesDisabled = true; + } + } + public Task FlushAsync(IHttpOutputProducer outputProducer, CancellationToken cancellationToken) { lock (_writeLock) @@ -243,6 +254,11 @@ public Task WriteWindowUpdateAsync(int streamId, int sizeIncrement) { lock (_writeLock) { + if (_windowUpdatesDisabled) + { + return Task.CompletedTask; + } + _outgoingFrame.PrepareWindowUpdate(streamId, sizeIncrement); return WriteFrameUnsynchronizedAsync(); } diff --git a/src/Kestrel.Core/Internal/Http2/Http2MessageBody.cs b/src/Kestrel.Core/Internal/Http2/Http2MessageBody.cs index 9c423da75..69048047e 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2MessageBody.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2MessageBody.cs @@ -10,7 +10,7 @@ public class Http2MessageBody : MessageBody { private readonly Http2Stream _context; - protected Http2MessageBody(Http2Stream context) + private Http2MessageBody(Http2Stream context) : base(context) { _context = context; @@ -33,12 +33,7 @@ protected override void OnDataRead(int bytesRead) // REVIEW: Can an app partially consume the request body in a non-aborted stream? Write a test. protected override Task OnConsumeAsync() => Task.CompletedTask; - public override Task StopAsync() - { - _context.RequestBodyPipe.Reader.Complete(); - _context.RequestBodyPipe.Writer.Complete(); - return Task.CompletedTask; - } + public override Task StopAsync() => Task.CompletedTask; public static MessageBody For( HttpRequestHeaders headers, diff --git a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs index 2cb5da902..cbdb55a9a 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs @@ -56,6 +56,8 @@ protected override void OnReset() protected override void OnRequestProcessingEnded() { + RequestBodyPipe.Reader.Complete(); + RequestBodyPipe.Writer.Complete(); _context.StreamLifetimeHandler.OnStreamCompleted(StreamId); } @@ -280,13 +282,13 @@ public Task OnDataAsync(Http2Frame dataFrame) if ((dataFrame.DataFlags & Http2DataFrameFlags.END_STREAM) == Http2DataFrameFlags.END_STREAM) { - OnEndStream(); + OnEndStreamReceived(); } return Task.CompletedTask; } - public void OnEndStream() + public void OnEndStreamReceived() { EndStreamReceived = true; RequestBodyPipe.Writer.Complete(); diff --git a/test/Kestrel.Core.Tests/Http2ConnectionTests.cs b/test/Kestrel.Core.Tests/Http2ConnectionTests.cs index d584b8e5f..ec6bc1564 100644 --- a/test/Kestrel.Core.Tests/Http2ConnectionTests.cs +++ b/test/Kestrel.Core.Tests/Http2ConnectionTests.cs @@ -370,7 +370,7 @@ await ExpectAsync(Http2FrameType.DATA, await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); - Assert.Equal(dataFrame.DataPayload, _helloWorldBytes); + Assert.Equal(_helloWorldBytes, dataFrame.DataPayload); } [Fact] @@ -385,6 +385,7 @@ await ExpectAsync(Http2FrameType.HEADERS, withLength: 37, withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS, withStreamId: 1); + var dataFrame = await ExpectAsync(Http2FrameType.DATA, withLength: _maxData.Length, withFlags: (byte)Http2DataFrameFlags.NONE, @@ -396,7 +397,90 @@ await ExpectAsync(Http2FrameType.DATA, await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); - Assert.Equal(dataFrame.DataPayload, _maxData); + Assert.Equal(_maxData, dataFrame.DataPayload); + } + + [Fact] + public async Task DATA_Received_GreaterThanDefaultInitialWindowSize_ReadByStream() + { + // _maxData should be 1/4th of the default initial window size + 1. + Assert.Equal(Http2PeerSettings.DefaultInitialWindowSize + 1, (uint)_maxData.Length * 4); + + // Double the client stream windows to 128KiB so no stream WINDOW_UPDATEs need to be sent. + _clientSettings.InitialWindowSize = Http2PeerSettings.DefaultInitialWindowSize * 2; + + await InitializeConnectionAsync(_echoApplication); + + // Double the client connection window to 128KiB. + await SendWindowUpdateAsync(0, (int)Http2PeerSettings.DefaultInitialWindowSize); + + await StartStreamAsync(1, _browserRequestHeaders, endStream: false); + await SendDataAsync(1, _maxData, endStream: false); + + await ExpectAsync(Http2FrameType.HEADERS, + withLength: 37, + withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS, + withStreamId: 1); + + var dataFrame1 = await ExpectAsync(Http2FrameType.DATA, + withLength: _maxData.Length, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 1); + + // Writing over half the initial window size induces both a connection-level and stream-level window update. + await SendDataAsync(1, _maxData, endStream: false); + + var streamWindowUpdateFrame1 = await ExpectAsync(Http2FrameType.WINDOW_UPDATE, + withLength: 4, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 1); + var connectionWindowUpdateFrame1 = await ExpectAsync(Http2FrameType.WINDOW_UPDATE, + withLength: 4, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 0); + + var dataFrame2 = await ExpectAsync(Http2FrameType.DATA, + withLength: _maxData.Length, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 1); + + await SendDataAsync(1, _maxData, endStream: false); + + var dataFrame3 = await ExpectAsync(Http2FrameType.DATA, + withLength: _maxData.Length, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 1); + + await SendDataAsync(1, _maxData, endStream: true); + + var streamWindowUpdateFrame2 = await ExpectAsync(Http2FrameType.WINDOW_UPDATE, + withLength: 4, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 1); + var connectionWindowUpdateFrame2 = await ExpectAsync(Http2FrameType.WINDOW_UPDATE, + withLength: 4, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 0); + + var dataFrame4 = await ExpectAsync(Http2FrameType.DATA, + withLength: _maxData.Length, + 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); + + Assert.Equal(_maxData, dataFrame1.DataPayload); + Assert.Equal(_maxData, dataFrame2.DataPayload); + Assert.Equal(_maxData, dataFrame3.DataPayload); + Assert.Equal(_maxData, dataFrame4.DataPayload); + Assert.Equal(_maxData.Length * 2, streamWindowUpdateFrame1.WindowUpdateSizeIncrement); + Assert.Equal(_maxData.Length * 2, connectionWindowUpdateFrame1.WindowUpdateSizeIncrement); + Assert.Equal(_maxData.Length * 2, streamWindowUpdateFrame2.WindowUpdateSizeIncrement); + Assert.Equal(_maxData.Length * 2, connectionWindowUpdateFrame2.WindowUpdateSizeIncrement); } [Fact] @@ -428,7 +512,7 @@ await ExpectAsync(Http2FrameType.DATA, await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); - Assert.Equal(dataFrame.DataPayload, _helloWorldBytes); + Assert.Equal(_helloWorldBytes, dataFrame.DataPayload); } [Fact] @@ -497,6 +581,111 @@ await ExpectAsync(Http2FrameType.DATA, Assert.Equal(stream3DataFrame2.DataPayload, _worldBytes); } + [Fact] + public async Task DATA_Received_Multiplexed_GreaterThanDefaultInitialWindowSize_ReadByStream() + { + // _maxData should be 1/4th of the default initial window size + 1. + Assert.Equal(Http2PeerSettings.DefaultInitialWindowSize + 1, (uint)_maxData.Length * 4); + + // Double the client stream windows to 128KiB so no stream WINDOW_UPDATEs need to be sent. + _clientSettings.InitialWindowSize = Http2PeerSettings.DefaultInitialWindowSize * 2; + + await InitializeConnectionAsync(_echoApplication); + + // Double the client connection window to 128KiB. + await SendWindowUpdateAsync(0, (int)Http2PeerSettings.DefaultInitialWindowSize); + + await StartStreamAsync(1, _browserRequestHeaders, endStream: false); + await SendDataAsync(1, _maxData, endStream: false); + + await ExpectAsync(Http2FrameType.HEADERS, + withLength: 37, + withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS, + withStreamId: 1); + + var dataFrame1 = await ExpectAsync(Http2FrameType.DATA, + withLength: _maxData.Length, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 1); + + // Writing over half the initial window size induces both a connection-level and stream-level window update. + await SendDataAsync(1, _maxData, endStream: false); + + var streamWindowUpdateFrame1 = await ExpectAsync(Http2FrameType.WINDOW_UPDATE, + withLength: 4, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 1); + var connectionWindowUpdateFrame1 = await ExpectAsync(Http2FrameType.WINDOW_UPDATE, + withLength: 4, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 0); + + var dataFrame2 = await ExpectAsync(Http2FrameType.DATA, + withLength: _maxData.Length, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 1); + + await SendDataAsync(1, _maxData, endStream: false); + + var dataFrame3 = await ExpectAsync(Http2FrameType.DATA, + withLength: _maxData.Length, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 1); + + // Uploading data to a new stream induces a second connection-level but not stream-level window update. + await StartStreamAsync(3, _browserRequestHeaders, endStream: false); + await SendDataAsync(3, _maxData, endStream: true); + + var connectionWindowUpdateFrame2 = await ExpectAsync(Http2FrameType.WINDOW_UPDATE, + withLength: 4, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 0); + + await ExpectAsync(Http2FrameType.HEADERS, + withLength: 37, + withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS, + withStreamId: 3); + + var dataFrame4 = await ExpectAsync(Http2FrameType.DATA, + withLength: _maxData.Length, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 3); + await ExpectAsync(Http2FrameType.DATA, + withLength: 0, + withFlags: (byte)Http2DataFrameFlags.END_STREAM, + withStreamId: 3); + + await SendDataAsync(1, _maxData, endStream: true); + + var streamWindowUpdateFrame2 = await ExpectAsync(Http2FrameType.WINDOW_UPDATE, + withLength: 4, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 1); + + var dataFrame5 = await ExpectAsync(Http2FrameType.DATA, + withLength: _maxData.Length, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 1); + await ExpectAsync(Http2FrameType.DATA, + withLength: 0, + withFlags: (byte)Http2DataFrameFlags.END_STREAM, + withStreamId: 1); + + await StopConnectionAsync(expectedLastStreamId: 3, ignoreNonGoAwayFrames: false); + + Assert.Equal(_maxData, dataFrame1.DataPayload); + Assert.Equal(_maxData, dataFrame2.DataPayload); + Assert.Equal(_maxData, dataFrame3.DataPayload); + Assert.Equal(_maxData.Length * 2, streamWindowUpdateFrame1.WindowUpdateSizeIncrement); + Assert.Equal(_maxData.Length * 2, connectionWindowUpdateFrame1.WindowUpdateSizeIncrement); + + Assert.Equal(_maxData, dataFrame4.DataPayload); + Assert.Equal(_maxData.Length * 2, connectionWindowUpdateFrame2.WindowUpdateSizeIncrement); + + Assert.Equal(_maxData, dataFrame5.DataPayload); + Assert.Equal(_maxData.Length * 2, streamWindowUpdateFrame2.WindowUpdateSizeIncrement); + } + [Fact] public async Task DATA_Received_Multiplexed_AppMustNotBlockOtherFrames() { @@ -589,7 +778,63 @@ await ExpectAsync(Http2FrameType.DATA, await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); - Assert.Equal(dataFrame.DataPayload, _helloWorldBytes); + Assert.Equal(_helloWorldBytes, dataFrame.DataPayload); + } + + [Theory] + [InlineData(0)] + [InlineData(1)] + [InlineData(255)] + public async Task DATA_Received_WithPadding_CountedTowardsFlowControl(byte padLength) + { + // _maxData should be 1/4th of the default initial window size + 1. + Assert.Equal(Http2PeerSettings.DefaultInitialWindowSize + 1, (uint)_maxData.Length * 4); + + var maxDataMinusPadding = new ArraySegment(_maxData, 0, _maxData.Length - padLength - 1); + + await InitializeConnectionAsync(_echoApplication); + + await StartStreamAsync(1, _browserRequestHeaders, endStream: false); + await SendDataWithPaddingAsync(1, maxDataMinusPadding, padLength, endStream: false); + + await ExpectAsync(Http2FrameType.HEADERS, + withLength: 37, + withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS, + withStreamId: 1); + + var dataFrame1 = await ExpectAsync(Http2FrameType.DATA, + withLength: maxDataMinusPadding.Count, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 1); + + // Writing over half the initial window size induces both a connection-level and stream-level window update. + await SendDataAsync(1, _maxData, endStream: true); + + var streamWindowUpdateFrame1 = await ExpectAsync(Http2FrameType.WINDOW_UPDATE, + withLength: 4, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 1); + var connectionWindowUpdateFrame1 = await ExpectAsync(Http2FrameType.WINDOW_UPDATE, + withLength: 4, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 0); + + var dataFrame2 = await ExpectAsync(Http2FrameType.DATA, + withLength: _maxData.Length, + 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); + + Assert.Equal(dataFrame1.DataPayload, maxDataMinusPadding); + Assert.Equal(_maxData, dataFrame2.DataPayload); + + Assert.Equal(_maxData.Length * 2, streamWindowUpdateFrame1.WindowUpdateSizeIncrement); + Assert.Equal(_maxData.Length * 2, connectionWindowUpdateFrame1.WindowUpdateSizeIncrement); } [Fact] @@ -776,6 +1021,33 @@ await WaitForConnectionErrorAsync( expectedErrorMessage: CoreStrings.FormatHttp2ErrorStreamClosed(Http2FrameType.DATA, streamId: 1)); } + [Fact] + public async Task DATA_Received_NoStreamWindowSpace_ConnectionError() + { + // _maxData should be 1/4th of the default initial window size + 1. + Assert.Equal(Http2PeerSettings.DefaultInitialWindowSize + 1, (uint)_maxData.Length * 4); + + await InitializeConnectionAsync(_waitForAbortApplication); + + await StartStreamAsync(1, _browserRequestHeaders, endStream: false); + + await SendDataAsync(1, _maxData, endStream: false); + await SendDataAsync(1, _maxData, endStream: false); + await SendDataAsync(1, _maxData, endStream: false); + await SendDataAsync(1, _maxData, endStream: false); + + await WaitForConnectionErrorAsync( + ignoreNonGoAwayFrames: false, + expectedLastStreamId: 1, + expectedErrorCode: Http2ErrorCode.FLOW_CONTROL_ERROR, + expectedErrorMessage: CoreStrings.Http2ErrorFlowControlWindowExceeded); + + // I hate doing this, but it avoids exceptions from MemoryPool.Dipose() in debug mode. + // The problem is since the stream's ProcessRequestsAsync loop is never awaited by the connection, it's not + // really possible to observe when all the blocks are returned to the pool in this test. + await Task.Delay(100); + } + [Fact] public async Task DATA_Sent_DespiteConnectionBackpressure_IfEmptyAndEndsStream() { @@ -1736,7 +2008,7 @@ await ExpectAsync(Http2FrameType.HEADERS, withFlags: (byte)Http2DataFrameFlags.NONE, withStreamId: streamId); - Assert.Equal(dataFrame.DataPayload, new ArraySegment(_helloWorldBytes, 0, initialWindowSize)); + Assert.Equal(new ArraySegment(_helloWorldBytes, 0, initialWindowSize), dataFrame.DataPayload); Assert.False(writeTasks[streamId].IsCompleted); } @@ -2225,7 +2497,7 @@ await ExpectAsync(Http2FrameType.HEADERS, withFlags: (byte)Http2DataFrameFlags.NONE, withStreamId: streamId); - Assert.Equal(dataFrame.DataPayload, new ArraySegment(_helloWorldBytes, 0, initialWindowSize)); + Assert.Equal(new ArraySegment(_helloWorldBytes, 0, initialWindowSize), dataFrame.DataPayload); Assert.False(writeTasks[streamId].IsCompleted); } @@ -2526,8 +2798,8 @@ await ExpectAsync(Http2FrameType.DATA, await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); - Assert.Equal(dataFrame1.DataPayload, new ArraySegment(_helloWorldBytes, 0, initialWindowSize)); - Assert.Equal(dataFrame2.DataPayload, new ArraySegment(_helloWorldBytes, initialWindowSize, initialWindowSize)); + Assert.Equal(new ArraySegment(_helloWorldBytes, 0, initialWindowSize), dataFrame1.DataPayload); + Assert.Equal(new ArraySegment(_helloWorldBytes, initialWindowSize, initialWindowSize), dataFrame2.DataPayload); } [Fact] @@ -2581,9 +2853,9 @@ await ExpectAsync(Http2FrameType.DATA, await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); - Assert.Equal(dataFrame1.DataPayload, new ArraySegment(_helloWorldBytes, 0, 6)); - Assert.Equal(dataFrame2.DataPayload, new ArraySegment(_helloWorldBytes, 6, 3)); - Assert.Equal(dataFrame3.DataPayload, new ArraySegment(_helloWorldBytes, 9, 3)); + Assert.Equal(new ArraySegment(_helloWorldBytes, 0, 6), dataFrame1.DataPayload); + Assert.Equal(new ArraySegment(_helloWorldBytes, 6, 3), dataFrame2.DataPayload); + Assert.Equal(new ArraySegment(_helloWorldBytes, 9, 3), dataFrame3.DataPayload); } [Fact] From c30a5e8ba3dceb378138ee4ca46ff70d038d13a0 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 25 Jul 2018 18:36:00 -0700 Subject: [PATCH 5/8] Address PR feedback --- .../Http2/FlowControl/Http2FlowControl.cs | 2 +- .../FlowControl/Http2InputFlowControl.cs | 26 ++++-- .../FlowControl/Http2OutputFlowControl.cs | 2 +- .../Http2OutputFlowControlAwaitable.cs | 2 +- .../Http2StreamInputFlowControl.cs | 7 +- .../Http2StreamOutputFlowControl.cs | 2 +- .../Internal/Http2/Http2Connection.cs | 3 +- .../Internal/Http2/Http2FrameWriter.cs | 17 +--- .../Internal/Http2/Http2MessageBody.cs | 1 - .../Internal/Http2/Http2OutputProducer.cs | 1 + .../Internal/Http2/Http2Stream.cs | 82 +++++++++++++++---- .../Internal/Http2/Http2StreamContext.cs | 1 + .../Http2ConnectionTests.cs | 68 +++++++++------ 13 files changed, 146 insertions(+), 68 deletions(-) diff --git a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2FlowControl.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2FlowControl.cs index eee363c32..d857dca0d 100644 --- a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2FlowControl.cs +++ b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2FlowControl.cs @@ -3,7 +3,7 @@ using System.Diagnostics; -namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 +namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.FlowControl { public struct Http2FlowControl { diff --git a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs index 333839526..534d8ef98 100644 --- a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs +++ b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs @@ -3,16 +3,17 @@ using System.Diagnostics; -namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 +namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.FlowControl { public class Http2InputFlowControl { private readonly int _initialWindowSize; private readonly int _minWindowSizeIncrement; - private readonly object _flowLock = new object(); private Http2FlowControl _flow; private int _unconfirmedBytes; + private bool _windowUpdatesDisabled; + private readonly object _flowLock = new object(); public Http2InputFlowControl(uint initialWindowSize) { @@ -32,9 +33,9 @@ public bool TryAdvance(int bytes) throw new Http2ConnectionErrorException(CoreStrings.Http2ErrorFlowControlWindowExceeded, Http2ErrorCode.FLOW_CONTROL_ERROR); } - // This data won't be read by the app, so tell the caller to count the data as already consumed. if (_flow.IsAborted) { + // This data won't be read by the app, so tell the caller to count the data as already consumed. return false; } @@ -45,10 +46,10 @@ public bool TryAdvance(int bytes) public bool TryUpdateWindow(int bytes, out int updateSize) { - updateSize = 0; - lock (_flowLock) { + updateSize = 0; + if (_flow.IsAborted) { // All data received by stream has already been returned to the connection window. @@ -62,6 +63,13 @@ public bool TryUpdateWindow(int bytes, out int updateSize) Debug.Assert(false, $"{nameof(TryUpdateWindow)} attempted to grow window past max size."); } + if (_windowUpdatesDisabled) + { + // Continue returning space to the connection window. The end of the stream has already + // been received, so don't send window updates for the stream window. + return true; + } + var potentialUpdateSize = _unconfirmedBytes + bytes; if (potentialUpdateSize > _minWindowSizeIncrement) @@ -78,6 +86,14 @@ public bool TryUpdateWindow(int bytes, out int updateSize) } } + public void StopWindowUpdates() + { + lock (_flowLock) + { + _windowUpdatesDisabled = true; + } + } + public int Abort() { lock (_flowLock) diff --git a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2OutputFlowControl.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2OutputFlowControl.cs index 93f5cb213..75c7ed93c 100644 --- a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2OutputFlowControl.cs +++ b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2OutputFlowControl.cs @@ -4,7 +4,7 @@ using System.Collections.Generic; using System.Diagnostics; -namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 +namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.FlowControl { public class Http2OutputFlowControl { diff --git a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2OutputFlowControlAwaitable.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2OutputFlowControlAwaitable.cs index a83a0f9b0..5da9115e6 100644 --- a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2OutputFlowControlAwaitable.cs +++ b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2OutputFlowControlAwaitable.cs @@ -6,7 +6,7 @@ using System.Runtime.CompilerServices; using System.Threading; -namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 +namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.FlowControl { public class Http2OutputFlowControlAwaitable : ICriticalNotifyCompletion { diff --git a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamInputFlowControl.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamInputFlowControl.cs index 813eb28ce..e89c667f8 100644 --- a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamInputFlowControl.cs +++ b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamInputFlowControl.cs @@ -3,7 +3,7 @@ using System.Diagnostics; -namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 +namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.FlowControl { public class Http2StreamInputFlowControl { @@ -57,6 +57,11 @@ public void UpdateWindows(int bytes) UpdateConnectionWindow(bytes); } + public void StopWindowUpdates() + { + _streamLevelFlowControl.StopWindowUpdates(); + } + public void Abort() { var unreadBytes = _streamLevelFlowControl.Abort(); diff --git a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamOutputFlowControl.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamOutputFlowControl.cs index 677d82137..ebcd2f3e2 100644 --- a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamOutputFlowControl.cs +++ b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamOutputFlowControl.cs @@ -5,7 +5,7 @@ using System.Diagnostics; using System.Runtime.CompilerServices; -namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 +namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.FlowControl { public class Http2StreamOutputFlowControl { diff --git a/src/Kestrel.Core/Internal/Http2/Http2Connection.cs b/src/Kestrel.Core/Internal/Http2/Http2Connection.cs index 409ee1328..6cbc86fde 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2Connection.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2Connection.cs @@ -15,6 +15,7 @@ using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.HPack; +using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.FlowControl; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure; using Microsoft.Extensions.Logging; using Microsoft.Net.Http.Headers; @@ -203,7 +204,7 @@ public async Task ProcessRequestsAsync(IHttpApplication appl try { // Ensure aborting each stream doesn't result in unnecessary WINDOW_UPDATE frames being sent. - _frameWriter.DisableWindowUpdates(); + _inputFlowControl.StopWindowUpdates(); foreach (var stream in _streams.Values) { diff --git a/src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs b/src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs index e46f80424..5c5703308 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs @@ -10,6 +10,7 @@ using Microsoft.AspNetCore.Connections; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http; +using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.FlowControl; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.HPack; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure; @@ -29,7 +30,6 @@ public class Http2FrameWriter private readonly StreamSafePipeFlusher _flusher; private bool _completed; - private bool _windowUpdatesDisabled; public Http2FrameWriter( PipeWriter outputPipeWriter, @@ -66,16 +66,6 @@ public void Abort(ConnectionAbortedException ex) Complete(); } - public void DisableWindowUpdates() - { - lock (_writeLock) - { - // This is called before aborting each stream during connection teardown in order to avoid - // sending unnecessary window updates right before closing the connection. - _windowUpdatesDisabled = true; - } - } - public Task FlushAsync(IHttpOutputProducer outputProducer, CancellationToken cancellationToken) { lock (_writeLock) @@ -254,11 +244,6 @@ public Task WriteWindowUpdateAsync(int streamId, int sizeIncrement) { lock (_writeLock) { - if (_windowUpdatesDisabled) - { - return Task.CompletedTask; - } - _outgoingFrame.PrepareWindowUpdate(streamId, sizeIncrement); return WriteFrameUnsynchronizedAsync(); } diff --git a/src/Kestrel.Core/Internal/Http2/Http2MessageBody.cs b/src/Kestrel.Core/Internal/Http2/Http2MessageBody.cs index 69048047e..2ac43a297 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2MessageBody.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2MessageBody.cs @@ -30,7 +30,6 @@ protected override void OnDataRead(int bytesRead) _context.OnDataRead(bytesRead); } - // REVIEW: Can an app partially consume the request body in a non-aborted stream? Write a test. protected override Task OnConsumeAsync() => Task.CompletedTask; public override Task StopAsync() => Task.CompletedTask; diff --git a/src/Kestrel.Core/Internal/Http2/Http2OutputProducer.cs b/src/Kestrel.Core/Internal/Http2/Http2OutputProducer.cs index d7f1c886d..8c1e49f95 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2OutputProducer.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2OutputProducer.cs @@ -9,6 +9,7 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.Connections; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http; +using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.FlowControl; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure; using Microsoft.AspNetCore.Server.Kestrel.Transport.Abstractions.Internal; diff --git a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs index cbdb55a9a..5394a3027 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs @@ -6,13 +6,10 @@ using System.Diagnostics; using System.IO; using System.IO.Pipelines; -using System.Runtime.InteropServices; -using System.Text; -using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Connections; -using Microsoft.AspNetCore.Connections.Abstractions; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http; +using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.FlowControl; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure; using Microsoft.AspNetCore.Server.Kestrel.Transport.Abstractions.Internal; using Microsoft.Extensions.Primitives; @@ -27,7 +24,8 @@ public partial class Http2Stream : HttpProtocol private readonly Http2StreamInputFlowControl _inputFlowControl; private readonly Http2StreamOutputFlowControl _outputFlowControl; - private int _requestAborted; + private StreamCompletionFlags _completionState; + private readonly object _completionLock = new object(); public Http2Stream(Http2StreamContext context) : base(context) @@ -45,7 +43,7 @@ public Http2Stream(Http2StreamContext context) public int StreamId => _context.StreamId; public bool RequestBodyStarted { get; private set; } - public bool EndStreamReceived { get; private set; } + public bool EndStreamReceived => (_completionState & StreamCompletionFlags.EndStreamReceived) == StreamCompletionFlags.EndStreamReceived; public override bool IsUpgradableRequest => false; @@ -56,9 +54,13 @@ protected override void OnReset() protected override void OnRequestProcessingEnded() { + TryApplyCompletionFlag(StreamCompletionFlags.RequestProcessingEnded); + RequestBodyPipe.Reader.Complete(); - RequestBodyPipe.Writer.Complete(); - _context.StreamLifetimeHandler.OnStreamCompleted(StreamId); + + // The app can no longer read any more of the request body, so return any bytes that weren't read to the + // connection's flow-control window. + _inputFlowControl.Abort(); } protected override string CreateRequestId() @@ -265,14 +267,22 @@ public Task OnDataAsync(Http2Frame dataFrame) } var payload = dataFrame.DataPayload; + var endStream = (dataFrame.DataFlags & Http2DataFrameFlags.END_STREAM) == Http2DataFrameFlags.END_STREAM; if (payload.Count > 0) { + RequestBodyStarted = true; + + if (endStream) + { + // No need to send any more window updates for this stream now that we've received all the data. + // Call before flushing the request body pipe, because that might induce a window update. + _inputFlowControl.StopWindowUpdates(); + } + _inputFlowControl.Advance(payload.Count); RequestBodyPipe.Writer.Write(payload); - - RequestBodyStarted = true; var flushTask = RequestBodyPipe.Writer.FlushAsync(); // It shouldn't be possible for the RequestBodyPipe to fill up an return an incomplete task if @@ -280,7 +290,7 @@ public Task OnDataAsync(Http2Frame dataFrame) Debug.Assert(flushTask.IsCompleted); } - if ((dataFrame.DataFlags & Http2DataFrameFlags.END_STREAM) == Http2DataFrameFlags.END_STREAM) + if (endStream) { OnEndStreamReceived(); } @@ -290,8 +300,11 @@ public Task OnDataAsync(Http2Frame dataFrame) public void OnEndStreamReceived() { - EndStreamReceived = true; + TryApplyCompletionFlag(StreamCompletionFlags.EndStreamReceived); + RequestBodyPipe.Writer.Complete(); + + _inputFlowControl.StopWindowUpdates(); } public void OnDataRead(int bytesRead) @@ -306,7 +319,7 @@ public bool TryUpdateOutputWindow(int bytes) public override void Abort(ConnectionAbortedException abortReason) { - if (Interlocked.Exchange(ref _requestAborted, 1) != 0) + if (!TryApplyCompletionFlag(StreamCompletionFlags.Aborted)) { return; } @@ -322,7 +335,7 @@ protected override void ApplicationAbort() private void ResetAndAbort(ConnectionAbortedException abortReason, Http2ErrorCode error) { - if (Interlocked.Exchange(ref _requestAborted, 1) != 0) + if (!TryApplyCompletionFlag(StreamCompletionFlags.Aborted)) { return; } @@ -342,7 +355,6 @@ private void AbortCore(ConnectionAbortedException abortReason) // Unblock the request body. RequestBodyPipe.Writer.Complete(new IOException(CoreStrings.Http2StreamAborted, abortReason)); - // Stop counting any buffered bytes for this stream towards the connection input flow-control window. _inputFlowControl.Abort(); } @@ -357,5 +369,45 @@ private Pipe CreateRequestBodyPipe() useSynchronizationContext: false, minimumSegmentSize: KestrelMemoryPool.MinimumSegmentSize )); + + private bool TryApplyCompletionFlag(StreamCompletionFlags completionState) + { + lock (_completionLock) + { + var lastCompletionState = _completionState; + _completionState |= completionState; + + if (ShoulStopTrackingStream(_completionState) && !ShoulStopTrackingStream(lastCompletionState)) + { + _context.StreamLifetimeHandler.OnStreamCompleted(StreamId); + } + + return _completionState != lastCompletionState; + } + } + + private static bool ShoulStopTrackingStream(StreamCompletionFlags completionState) + { + // This could be a single condition, but I think it reads better as two if's. + if ((completionState & StreamCompletionFlags.RequestProcessingEnded) == StreamCompletionFlags.RequestProcessingEnded) + { + if ((completionState & StreamCompletionFlags.Aborted) == StreamCompletionFlags.Aborted || + (completionState & StreamCompletionFlags.EndStreamReceived) == StreamCompletionFlags.EndStreamReceived) + { + return true; + } + } + + return false; + } + + [Flags] + private enum StreamCompletionFlags + { + None = 0, + RequestProcessingEnded = 1, + EndStreamReceived = 2, + Aborted = 4, + } } } diff --git a/src/Kestrel.Core/Internal/Http2/Http2StreamContext.cs b/src/Kestrel.Core/Internal/Http2/Http2StreamContext.cs index a91629e18..7f47bb9c7 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2StreamContext.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2StreamContext.cs @@ -5,6 +5,7 @@ using System.Net; using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http; +using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.FlowControl; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure; namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 diff --git a/test/Kestrel.Core.Tests/Http2ConnectionTests.cs b/test/Kestrel.Core.Tests/Http2ConnectionTests.cs index ec6bc1564..371f111af 100644 --- a/test/Kestrel.Core.Tests/Http2ConnectionTests.cs +++ b/test/Kestrel.Core.Tests/Http2ConnectionTests.cs @@ -453,10 +453,6 @@ await ExpectAsync(Http2FrameType.HEADERS, await SendDataAsync(1, _maxData, endStream: true); - var streamWindowUpdateFrame2 = await ExpectAsync(Http2FrameType.WINDOW_UPDATE, - withLength: 4, - withFlags: (byte)Http2DataFrameFlags.NONE, - withStreamId: 1); var connectionWindowUpdateFrame2 = await ExpectAsync(Http2FrameType.WINDOW_UPDATE, withLength: 4, withFlags: (byte)Http2DataFrameFlags.NONE, @@ -479,7 +475,6 @@ await ExpectAsync(Http2FrameType.DATA, Assert.Equal(_maxData, dataFrame4.DataPayload); Assert.Equal(_maxData.Length * 2, streamWindowUpdateFrame1.WindowUpdateSizeIncrement); Assert.Equal(_maxData.Length * 2, connectionWindowUpdateFrame1.WindowUpdateSizeIncrement); - Assert.Equal(_maxData.Length * 2, streamWindowUpdateFrame2.WindowUpdateSizeIncrement); Assert.Equal(_maxData.Length * 2, connectionWindowUpdateFrame2.WindowUpdateSizeIncrement); } @@ -611,7 +606,7 @@ await ExpectAsync(Http2FrameType.HEADERS, // Writing over half the initial window size induces both a connection-level and stream-level window update. await SendDataAsync(1, _maxData, endStream: false); - var streamWindowUpdateFrame1 = await ExpectAsync(Http2FrameType.WINDOW_UPDATE, + var streamWindowUpdateFrame = await ExpectAsync(Http2FrameType.WINDOW_UPDATE, withLength: 4, withFlags: (byte)Http2DataFrameFlags.NONE, withStreamId: 1); @@ -657,11 +652,6 @@ await ExpectAsync(Http2FrameType.DATA, await SendDataAsync(1, _maxData, endStream: true); - var streamWindowUpdateFrame2 = await ExpectAsync(Http2FrameType.WINDOW_UPDATE, - withLength: 4, - withFlags: (byte)Http2DataFrameFlags.NONE, - withStreamId: 1); - var dataFrame5 = await ExpectAsync(Http2FrameType.DATA, withLength: _maxData.Length, withFlags: (byte)Http2DataFrameFlags.NONE, @@ -676,14 +666,13 @@ await ExpectAsync(Http2FrameType.DATA, Assert.Equal(_maxData, dataFrame1.DataPayload); Assert.Equal(_maxData, dataFrame2.DataPayload); Assert.Equal(_maxData, dataFrame3.DataPayload); - Assert.Equal(_maxData.Length * 2, streamWindowUpdateFrame1.WindowUpdateSizeIncrement); + Assert.Equal(_maxData.Length * 2, streamWindowUpdateFrame.WindowUpdateSizeIncrement); Assert.Equal(_maxData.Length * 2, connectionWindowUpdateFrame1.WindowUpdateSizeIncrement); Assert.Equal(_maxData, dataFrame4.DataPayload); Assert.Equal(_maxData.Length * 2, connectionWindowUpdateFrame2.WindowUpdateSizeIncrement); Assert.Equal(_maxData, dataFrame5.DataPayload); - Assert.Equal(_maxData.Length * 2, streamWindowUpdateFrame2.WindowUpdateSizeIncrement); } [Fact] @@ -785,7 +774,7 @@ await ExpectAsync(Http2FrameType.DATA, [InlineData(0)] [InlineData(1)] [InlineData(255)] - public async Task DATA_Received_WithPadding_CountedTowardsFlowControl(byte padLength) + public async Task DATA_Received_WithPadding_CountsTowardsFlowControl(byte padLength) { // _maxData should be 1/4th of the default initial window size + 1. Assert.Equal(Http2PeerSettings.DefaultInitialWindowSize + 1, (uint)_maxData.Length * 4); @@ -810,11 +799,7 @@ await ExpectAsync(Http2FrameType.HEADERS, // Writing over half the initial window size induces both a connection-level and stream-level window update. await SendDataAsync(1, _maxData, endStream: true); - var streamWindowUpdateFrame1 = await ExpectAsync(Http2FrameType.WINDOW_UPDATE, - withLength: 4, - withFlags: (byte)Http2DataFrameFlags.NONE, - withStreamId: 1); - var connectionWindowUpdateFrame1 = await ExpectAsync(Http2FrameType.WINDOW_UPDATE, + var connectionWindowUpdateFrame = await ExpectAsync(Http2FrameType.WINDOW_UPDATE, withLength: 4, withFlags: (byte)Http2DataFrameFlags.NONE, withStreamId: 0); @@ -830,11 +815,43 @@ await ExpectAsync(Http2FrameType.DATA, await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); - Assert.Equal(dataFrame1.DataPayload, maxDataMinusPadding); + Assert.Equal(maxDataMinusPadding, dataFrame1.DataPayload); Assert.Equal(_maxData, dataFrame2.DataPayload); - Assert.Equal(_maxData.Length * 2, streamWindowUpdateFrame1.WindowUpdateSizeIncrement); - Assert.Equal(_maxData.Length * 2, connectionWindowUpdateFrame1.WindowUpdateSizeIncrement); + Assert.Equal(_maxData.Length * 2, connectionWindowUpdateFrame.WindowUpdateSizeIncrement); + } + + [Fact] + public async Task DATA_Received_ButNotConsumedByApp_CountsTowardsFlowControl() + { + // _maxData should be 1/4th of the default initial window size + 1. + Assert.Equal(Http2PeerSettings.DefaultInitialWindowSize + 1, (uint)_maxData.Length * 4); + + await InitializeConnectionAsync(_noopApplication); + + await StartStreamAsync(1, _browserRequestHeaders, endStream: false); + await SendDataAsync(1, _maxData, endStream: false); + + await ExpectAsync(Http2FrameType.HEADERS, + withLength: 55, + withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS, + withStreamId: 1); + await ExpectAsync(Http2FrameType.DATA, + withLength: 0, + withFlags: (byte)Http2DataFrameFlags.END_STREAM, + withStreamId: 1); + + // Writing over half the initial window size induces both a connection-level window update. + await SendDataAsync(1, _maxData, endStream: true); + + var connectionWindowUpdateFrame = await ExpectAsync(Http2FrameType.WINDOW_UPDATE, + withLength: 4, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 0); + + await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); + + Assert.Equal(_maxData.Length * 2, connectionWindowUpdateFrame.WindowUpdateSizeIncrement); } [Fact] @@ -1042,9 +1059,10 @@ await WaitForConnectionErrorAsync( expectedErrorCode: Http2ErrorCode.FLOW_CONTROL_ERROR, expectedErrorMessage: CoreStrings.Http2ErrorFlowControlWindowExceeded); - // I hate doing this, but it avoids exceptions from MemoryPool.Dipose() in debug mode. - // The problem is since the stream's ProcessRequestsAsync loop is never awaited by the connection, it's not - // really possible to observe when all the blocks are returned to the pool in this test. + // I hate doing this, but it avoids exceptions from MemoryPool.Dipose() in debug mode. The problem is since + // the stream's ProcessRequestsAsync loop is never awaited by the connection, it's not really possible to + // observe when all the blocks are returned to the pool in this test. This can be removed if we set up the + // DiagnosticMemoryPool to allow late returns or after we implement graceful shutdown. await Task.Delay(100); } From a328e1caa51fac62d994a0d18c20b54b5bf74d70 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 26 Jul 2018 12:19:42 -0700 Subject: [PATCH 6/8] Renames + RST_STREAM_Received_ReturnsSpaceToConnectionInputFlowControlWindow --- .../FlowControl/Http2InputFlowControl.cs | 10 +-- .../Internal/Http2/Http2Stream.cs | 4 +- .../Http2ConnectionTests.cs | 72 ++++++++++++++++--- 3 files changed, 72 insertions(+), 14 deletions(-) diff --git a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs index 534d8ef98..2dc7eb660 100644 --- a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs +++ b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs @@ -11,7 +11,7 @@ public class Http2InputFlowControl private readonly int _minWindowSizeIncrement; private Http2FlowControl _flow; - private int _unconfirmedBytes; + private int _unackedBytes; private bool _windowUpdatesDisabled; private readonly object _flowLock = new object(); @@ -70,16 +70,16 @@ public bool TryUpdateWindow(int bytes, out int updateSize) return true; } - var potentialUpdateSize = _unconfirmedBytes + bytes; + var potentialUpdateSize = _unackedBytes + bytes; if (potentialUpdateSize > _minWindowSizeIncrement) { - _unconfirmedBytes = 0; + _unackedBytes = 0; updateSize = potentialUpdateSize; } else { - _unconfirmedBytes = potentialUpdateSize; + _unackedBytes = potentialUpdateSize; } return true; @@ -101,6 +101,8 @@ public int Abort() _flow.Abort(); // Tell caller to return connection window space consumed by this stream. + // Even if window updates have been disable at the stream level, connection-level window updates may + // still be necessary. return _initialWindowSize - _flow.Available; } } diff --git a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs index 5394a3027..81cd91e13 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs @@ -391,8 +391,8 @@ private static bool ShoulStopTrackingStream(StreamCompletionFlags completionStat // This could be a single condition, but I think it reads better as two if's. if ((completionState & StreamCompletionFlags.RequestProcessingEnded) == StreamCompletionFlags.RequestProcessingEnded) { - if ((completionState & StreamCompletionFlags.Aborted) == StreamCompletionFlags.Aborted || - (completionState & StreamCompletionFlags.EndStreamReceived) == StreamCompletionFlags.EndStreamReceived) + if ((completionState & StreamCompletionFlags.EndStreamReceived) == StreamCompletionFlags.EndStreamReceived || + (completionState & StreamCompletionFlags.Aborted) == StreamCompletionFlags.Aborted) { return true; } diff --git a/test/Kestrel.Core.Tests/Http2ConnectionTests.cs b/test/Kestrel.Core.Tests/Http2ConnectionTests.cs index 371f111af..6c9337214 100644 --- a/test/Kestrel.Core.Tests/Http2ConnectionTests.cs +++ b/test/Kestrel.Core.Tests/Http2ConnectionTests.cs @@ -774,7 +774,7 @@ await ExpectAsync(Http2FrameType.DATA, [InlineData(0)] [InlineData(1)] [InlineData(255)] - public async Task DATA_Received_WithPadding_CountsTowardsFlowControl(byte padLength) + public async Task DATA_Received_WithPadding_CountsTowardsInputFlowControl(byte padLength) { // _maxData should be 1/4th of the default initial window size + 1. Assert.Equal(Http2PeerSettings.DefaultInitialWindowSize + 1, (uint)_maxData.Length * 4); @@ -822,7 +822,7 @@ await ExpectAsync(Http2FrameType.DATA, } [Fact] - public async Task DATA_Received_ButNotConsumedByApp_CountsTowardsFlowControl() + public async Task DATA_Received_ButNotConsumedByApp_CountsTowardsInputFlowControl() { // _maxData should be 1/4th of the default initial window size + 1. Assert.Equal(Http2PeerSettings.DefaultInitialWindowSize + 1, (uint)_maxData.Length * 4); @@ -1067,7 +1067,36 @@ await WaitForConnectionErrorAsync( } [Fact] - public async Task DATA_Sent_DespiteConnectionBackpressure_IfEmptyAndEndsStream() + public async Task DATA_Received_NoConnectionWindowSpace_ConnectionError() + { + // _maxData should be 1/4th of the default initial window size + 1. + Assert.Equal(Http2PeerSettings.DefaultInitialWindowSize + 1, (uint)_maxData.Length * 4); + + await InitializeConnectionAsync(_waitForAbortApplication); + + await StartStreamAsync(1, _browserRequestHeaders, endStream: false); + await SendDataAsync(1, _maxData, endStream: false); + await SendDataAsync(1, _maxData, endStream: false); + + await StartStreamAsync(3, _browserRequestHeaders, endStream: false); + await SendDataAsync(3, _maxData, endStream: false); + await SendDataAsync(3, _maxData, endStream: false); + + await WaitForConnectionErrorAsync( + ignoreNonGoAwayFrames: false, + expectedLastStreamId: 3, + expectedErrorCode: Http2ErrorCode.FLOW_CONTROL_ERROR, + expectedErrorMessage: CoreStrings.Http2ErrorFlowControlWindowExceeded); + + // I hate doing this, but it avoids exceptions from MemoryPool.Dipose() in debug mode. The problem is since + // the stream's ProcessRequestsAsync loop is never awaited by the connection, it's not really possible to + // observe when all the blocks are returned to the pool in this test. This can be removed if we set up the + // DiagnosticMemoryPool to allow late returns or after we implement graceful shutdown. + await Task.Delay(100); + } + + [Fact] + public async Task DATA_Sent_DespiteConnectionOutputFlowControl_IfEmptyAndEndsStream() { // Zero-length data frames are allowed to be sent even if there is no space available in the flow control window. // https://httpwg.org/specs/rfc7540.html#rfc.section.6.9.1 @@ -1152,7 +1181,7 @@ await ExpectAsync(Http2FrameType.DATA, } [Fact] - public async Task DATA_Sent_DespiteStreamBackpressure_IfEmptyAndEndsStream() + public async Task DATA_Sent_DespiteStreamOutputFlowControl_IfEmptyAndEndsStream() { // Zero-length data frames are allowed to be sent even if there is no space available in the flow control window. // https://httpwg.org/specs/rfc7540.html#rfc.section.6.9.1 @@ -1851,7 +1880,7 @@ await WaitForConnectionErrorAsync( } [Fact] - public async Task RST_STREAM_Received_RelievesConnectionBackpressure() + public async Task RST_STREAM_Received_ContinuesAppsAwaitingConnectionOutputFlowControl() { var writeTasks = new Task[4]; @@ -1971,7 +2000,7 @@ await ExpectAsync(Http2FrameType.DATA, } [Fact] - public async Task RST_STREAM_Received_RelievesStreamBackpressure() + public async Task RST_STREAM_Received_ContinuesAppsAwaitingStreamOutputFlowControl() { var writeTasks = new Task[6]; var initialWindowSize = _helloWorldBytes.Length / 2; @@ -2054,6 +2083,33 @@ await ExpectAsync(Http2FrameType.HEADERS, Assert.Contains(5, _abortedStreamIds); } + [Fact] + public async Task RST_STREAM_Received_ReturnsSpaceToConnectionInputFlowControlWindow() + { + // _maxData should be 1/4th of the default initial window size + 1. + Assert.Equal(Http2PeerSettings.DefaultInitialWindowSize + 1, (uint)_maxData.Length * 4); + + await InitializeConnectionAsync(_waitForAbortApplication); + + await StartStreamAsync(1, _browserRequestHeaders, endStream: false); + await SendDataAsync(1, _maxData, endStream: false); + await SendDataAsync(1, _maxData, endStream: false); + await SendDataAsync(1, _maxData, endStream: false); + + await SendRstStreamAsync(1); + await WaitForAllStreamsAsync(); + + var connectionWindowUpdateFrame = await ExpectAsync(Http2FrameType.WINDOW_UPDATE, + withLength: 4, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 0); + + await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); + + Assert.Contains(1, _abortedStreamIds); + Assert.Equal(_maxData.Length * 3, connectionWindowUpdateFrame.WindowUpdateSizeIncrement); + } + [Fact] public async Task RST_STREAM_Received_StreamIdZero_ConnectionError() { @@ -2360,7 +2416,7 @@ public async Task GOAWAY_Received_AbortsAllStreams() } [Fact] - public async Task GOAWAY_Received_RelievesConnectionBackpressure() + public async Task GOAWAY_Received_ContinuesAppsAwaitingConnectionOutputFlowControl() { var writeTasks = new Task[6]; var expectedFullFrameCountBeforeBackpressure = Http2PeerSettings.DefaultInitialWindowSize / _maxData.Length; @@ -2460,7 +2516,7 @@ await ExpectAsync(Http2FrameType.HEADERS, } [Fact] - public async Task GOAWAY_Received_RelievesStreamBackpressure() + public async Task GOAWAY_Received_ContinuesAppsAwaitingStreamOutputFlowControle() { var writeTasks = new Task[6]; var initialWindowSize = _helloWorldBytes.Length / 2; From 69cddcab581a0504a7fffab5bd83a53f9af9ece5 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 26 Jul 2018 15:46:21 -0700 Subject: [PATCH 7/8] Fix double abort call - Disable late memory pool block tracking in some tests - add minWindowSizeIncrement param --- .../FlowControl/Http2InputFlowControl.cs | 24 ++++-- .../Http2StreamInputFlowControl.cs | 5 +- .../Internal/Http2/Http2Connection.cs | 3 +- .../Internal/Http2/Http2Stream.cs | 8 +- .../Http2ConnectionTests.cs | 85 ++++++++++--------- .../DiagnosticMemoryPoolFactory.cs | 2 +- 6 files changed, 74 insertions(+), 53 deletions(-) diff --git a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs index 2dc7eb660..dedee593d 100644 --- a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs +++ b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs @@ -11,15 +11,17 @@ public class Http2InputFlowControl private readonly int _minWindowSizeIncrement; private Http2FlowControl _flow; - private int _unackedBytes; + private int _pendingUpdateSize; private bool _windowUpdatesDisabled; private readonly object _flowLock = new object(); - public Http2InputFlowControl(uint initialWindowSize) + public Http2InputFlowControl(uint initialWindowSize, uint minWindowSizeIncrement) { + Debug.Assert(initialWindowSize >= minWindowSizeIncrement, "minWindowSizeIncrement is greater than the window size."); + _flow = new Http2FlowControl(initialWindowSize); _initialWindowSize = (int)initialWindowSize; - _minWindowSizeIncrement = _initialWindowSize / 2; + _minWindowSizeIncrement = (int)minWindowSizeIncrement; } public bool TryAdvance(int bytes) @@ -70,16 +72,16 @@ public bool TryUpdateWindow(int bytes, out int updateSize) return true; } - var potentialUpdateSize = _unackedBytes + bytes; + var potentialUpdateSize = _pendingUpdateSize + bytes; if (potentialUpdateSize > _minWindowSizeIncrement) { - _unackedBytes = 0; + _pendingUpdateSize = 0; updateSize = potentialUpdateSize; } else { - _unackedBytes = potentialUpdateSize; + _pendingUpdateSize = potentialUpdateSize; } return true; @@ -98,11 +100,15 @@ public int Abort() { lock (_flowLock) { + if (_flow.IsAborted) + { + return 0; + } + _flow.Abort(); - // Tell caller to return connection window space consumed by this stream. - // Even if window updates have been disable at the stream level, connection-level window updates may - // still be necessary. + // Tell caller to return connection window space consumed by this stream. Even if window updates have + // been disabled at the stream level, connection-level window updates may still be necessary. return _initialWindowSize - _flow.Available; } } diff --git a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamInputFlowControl.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamInputFlowControl.cs index e89c667f8..ef2de73d4 100644 --- a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamInputFlowControl.cs +++ b/src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamInputFlowControl.cs @@ -17,10 +17,11 @@ public Http2StreamInputFlowControl( int streamId, Http2FrameWriter frameWriter, Http2InputFlowControl connectionLevelFlowControl, - uint initialWindowSize) + uint initialWindowSize, + uint minWindowSizeIncrement) { _connectionLevelFlowControl = connectionLevelFlowControl; - _streamLevelFlowControl = new Http2InputFlowControl(initialWindowSize); + _streamLevelFlowControl = new Http2InputFlowControl(initialWindowSize, minWindowSizeIncrement); _streamId = streamId; _frameWriter = frameWriter; diff --git a/src/Kestrel.Core/Internal/Http2/Http2Connection.cs b/src/Kestrel.Core/Internal/Http2/Http2Connection.cs index 6cbc86fde..756207ea1 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2Connection.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2Connection.cs @@ -62,7 +62,7 @@ private enum PseudoHeaderFields private readonly Http2ConnectionContext _context; private readonly Http2FrameWriter _frameWriter; private readonly HPackDecoder _hpackDecoder; - private readonly Http2InputFlowControl _inputFlowControl; + private readonly Http2InputFlowControl _inputFlowControl = new Http2InputFlowControl(Http2PeerSettings.DefaultInitialWindowSize, Http2PeerSettings.DefaultInitialWindowSize / 2); private readonly Http2OutputFlowControl _outputFlowControl = new Http2OutputFlowControl(Http2PeerSettings.DefaultInitialWindowSize); private readonly Http2PeerSettings _serverSettings = new Http2PeerSettings(); @@ -85,7 +85,6 @@ public Http2Connection(Http2ConnectionContext context) _context = context; _frameWriter = new Http2FrameWriter(context.Transport.Output, context.Application.Input, _outputFlowControl, this); _hpackDecoder = new HPackDecoder((int)_serverSettings.HeaderTableSize); - _inputFlowControl = new Http2InputFlowControl(Http2PeerSettings.DefaultInitialWindowSize); } public string ConnectionId => _context.ConnectionId; diff --git a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs index 81cd91e13..9ef161f0e 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs @@ -32,7 +32,13 @@ public Http2Stream(Http2StreamContext context) { _context = context; - _inputFlowControl = new Http2StreamInputFlowControl(_context.StreamId, _context.FrameWriter, context.ConnectionInputFlowControl, Http2PeerSettings.DefaultInitialWindowSize); + _inputFlowControl = new Http2StreamInputFlowControl( + _context.StreamId, + _context.FrameWriter, + context.ConnectionInputFlowControl, + Http2PeerSettings.DefaultInitialWindowSize, + Http2PeerSettings.DefaultInitialWindowSize / 2); + _outputFlowControl = new Http2StreamOutputFlowControl(context.ConnectionOutputFlowControl, context.ClientPeerSettings.InitialWindowSize); _http2Output = new Http2OutputProducer(context.StreamId, context.FrameWriter, _outputFlowControl, context.TimeoutControl, context.MemoryPool); diff --git a/test/Kestrel.Core.Tests/Http2ConnectionTests.cs b/test/Kestrel.Core.Tests/Http2ConnectionTests.cs index 6c9337214..1a6c8b16d 100644 --- a/test/Kestrel.Core.Tests/Http2ConnectionTests.cs +++ b/test/Kestrel.Core.Tests/Http2ConnectionTests.cs @@ -100,11 +100,7 @@ public class Http2ConnectionTests : IDisposable, IHttpHeadersHandler private static readonly byte[] _noData = new byte[0]; private static readonly byte[] _maxData = Encoding.ASCII.GetBytes(new string('a', Http2Frame.MinAllowedMaxFrameSize)); - private readonly MemoryPool _memoryPool = KestrelMemoryPool.Create(); - private readonly DuplexPipe.DuplexPipePair _pair; private readonly TestApplicationErrorLogger _logger; - private readonly Http2ConnectionContext _connectionContext; - private readonly Http2Connection _connection; private readonly Http2PeerSettings _clientSettings = new Http2PeerSettings(); private readonly HPackEncoder _hpackEncoder = new HPackEncoder(); private readonly HPackDecoder _hpackDecoder; @@ -126,28 +122,15 @@ public class Http2ConnectionTests : IDisposable, IHttpHeadersHandler private readonly RequestDelegate _waitForAbortFlushingApplication; private readonly RequestDelegate _waitForAbortWithDataApplication; + private MemoryPool _memoryPool; + private DuplexPipe.DuplexPipePair _pair; + private Http2ConnectionContext _connectionContext; + private Http2Connection _connection; + private Task _connectionTask; public Http2ConnectionTests() { - // Always dispatch test code back to the ThreadPool. This prevents deadlocks caused by continuing - // Http2Connection.ProcessRequestsAsync() loop with writer locks acquired. Run product code inline to make - // it easier to verify request frames are processed correctly immediately after sending the them. - var inputPipeOptions = new PipeOptions( - pool: _memoryPool, - readerScheduler: PipeScheduler.Inline, - writerScheduler: PipeScheduler.ThreadPool, - useSynchronizationContext: false - ); - var outputPipeOptions = new PipeOptions( - pool: _memoryPool, - readerScheduler: PipeScheduler.ThreadPool, - writerScheduler: PipeScheduler.Inline, - useSynchronizationContext: false - ); - - _pair = DuplexPipe.CreateConnectionPair(inputPipeOptions, outputPipeOptions); - _noopApplication = context => Task.CompletedTask; _readHeadersApplication = context => @@ -297,6 +280,31 @@ public Http2ConnectionTests() _logger = new TestApplicationErrorLogger(); + InitializeConnectionFields(KestrelMemoryPool.Create()); + } + + private void InitializeConnectionFields(MemoryPool memoryPool) + { + _memoryPool = memoryPool; + + // Always dispatch test code back to the ThreadPool. This prevents deadlocks caused by continuing + // Http2Connection.ProcessRequestsAsync() loop with writer locks acquired. Run product code inline to make + // it easier to verify request frames are processed correctly immediately after sending the them. + var inputPipeOptions = new PipeOptions( + pool: _memoryPool, + readerScheduler: PipeScheduler.Inline, + writerScheduler: PipeScheduler.ThreadPool, + useSynchronizationContext: false + ); + var outputPipeOptions = new PipeOptions( + pool: _memoryPool, + readerScheduler: PipeScheduler.ThreadPool, + writerScheduler: PipeScheduler.Inline, + useSynchronizationContext: false + ); + + _pair = DuplexPipe.CreateConnectionPair(inputPipeOptions, outputPipeOptions); + _connectionContext = new Http2ConnectionContext { ConnectionFeatures = new FeatureCollection(), @@ -308,6 +316,7 @@ public Http2ConnectionTests() Application = _pair.Application, Transport = _pair.Transport }; + _connection = new Http2Connection(_connectionContext); } @@ -714,7 +723,7 @@ await ExpectAsync(Http2FrameType.HEADERS, withLength: 37, withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS, withStreamId: 3); - await ExpectAsync(Http2FrameType.DATA, + await ExpectAsync(Http2FrameType.DATA, withLength: 5, withFlags: (byte)Http2DataFrameFlags.NONE, withStreamId: 3); @@ -840,7 +849,7 @@ await ExpectAsync(Http2FrameType.DATA, withLength: 0, withFlags: (byte)Http2DataFrameFlags.END_STREAM, withStreamId: 1); - + // Writing over half the initial window size induces both a connection-level window update. await SendDataAsync(1, _maxData, endStream: true); @@ -1041,6 +1050,12 @@ await WaitForConnectionErrorAsync( [Fact] public async Task DATA_Received_NoStreamWindowSpace_ConnectionError() { + // I hate doing this, but it avoids exceptions from MemoryPool.Dipose() in debug mode. The problem is since + // the stream's ProcessRequestsAsync loop is never awaited by the connection, it's not really possible to + // observe when all the blocks are returned. This can be removed after we implement graceful shutdown. + Dispose(); + InitializeConnectionFields(new DiagnosticMemoryPool(KestrelMemoryPool.CreateSlabMemoryPool(), allowLateReturn: true)); + // _maxData should be 1/4th of the default initial window size + 1. Assert.Equal(Http2PeerSettings.DefaultInitialWindowSize + 1, (uint)_maxData.Length * 4); @@ -1058,17 +1073,17 @@ await WaitForConnectionErrorAsync( expectedLastStreamId: 1, expectedErrorCode: Http2ErrorCode.FLOW_CONTROL_ERROR, expectedErrorMessage: CoreStrings.Http2ErrorFlowControlWindowExceeded); - - // I hate doing this, but it avoids exceptions from MemoryPool.Dipose() in debug mode. The problem is since - // the stream's ProcessRequestsAsync loop is never awaited by the connection, it's not really possible to - // observe when all the blocks are returned to the pool in this test. This can be removed if we set up the - // DiagnosticMemoryPool to allow late returns or after we implement graceful shutdown. - await Task.Delay(100); } [Fact] public async Task DATA_Received_NoConnectionWindowSpace_ConnectionError() { + // I hate doing this, but it avoids exceptions from MemoryPool.Dipose() in debug mode. The problem is since + // the stream's ProcessRequestsAsync loop is never awaited by the connection, it's not really possible to + // observe when all the blocks are returned. This can be removed after we implement graceful shutdown. + Dispose(); + InitializeConnectionFields(new DiagnosticMemoryPool(KestrelMemoryPool.CreateSlabMemoryPool(), allowLateReturn: true)); + // _maxData should be 1/4th of the default initial window size + 1. Assert.Equal(Http2PeerSettings.DefaultInitialWindowSize + 1, (uint)_maxData.Length * 4); @@ -1087,12 +1102,6 @@ await WaitForConnectionErrorAsync( expectedLastStreamId: 3, expectedErrorCode: Http2ErrorCode.FLOW_CONTROL_ERROR, expectedErrorMessage: CoreStrings.Http2ErrorFlowControlWindowExceeded); - - // I hate doing this, but it avoids exceptions from MemoryPool.Dipose() in debug mode. The problem is since - // the stream's ProcessRequestsAsync loop is never awaited by the connection, it's not really possible to - // observe when all the blocks are returned to the pool in this test. This can be removed if we set up the - // DiagnosticMemoryPool to allow late returns or after we implement graceful shutdown. - await Task.Delay(100); } [Fact] @@ -2098,12 +2107,12 @@ public async Task RST_STREAM_Received_ReturnsSpaceToConnectionInputFlowControlWi await SendRstStreamAsync(1); await WaitForAllStreamsAsync(); - + var connectionWindowUpdateFrame = await ExpectAsync(Http2FrameType.WINDOW_UPDATE, withLength: 4, withFlags: (byte)Http2DataFrameFlags.NONE, withStreamId: 0); - + await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); Assert.Contains(1, _abortedStreamIds); diff --git a/test/Kestrel.FunctionalTests/DiagnosticMemoryPoolFactory.cs b/test/Kestrel.FunctionalTests/DiagnosticMemoryPoolFactory.cs index e50e1ed30..d3a33bf2c 100644 --- a/test/Kestrel.FunctionalTests/DiagnosticMemoryPoolFactory.cs +++ b/test/Kestrel.FunctionalTests/DiagnosticMemoryPoolFactory.cs @@ -29,7 +29,7 @@ public MemoryPool Create() { lock (_pools) { - var pool = new DiagnosticMemoryPool(KestrelMemoryPool.CreateSlabMemoryPool(),_allowLateReturn, _rentTracking); + var pool = new DiagnosticMemoryPool(KestrelMemoryPool.CreateSlabMemoryPool(), _allowLateReturn, _rentTracking); _pools.Add(pool); return pool; } From bd20cf2bb7a25d13c8a52b6b175016514ce3f745 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 26 Jul 2018 15:52:36 -0700 Subject: [PATCH 8/8] Remove Http2 prefix from flow control types --- .../{Http2FlowControl.cs => FlowControl.cs} | 4 ++-- ...p2InputFlowControl.cs => InputFlowControl.cs} | 8 ++++---- ...OutputFlowControl.cs => OutputFlowControl.cs} | 16 ++++++++-------- ...waitable.cs => OutputFlowControlAwaitable.cs} | 4 ++-- ...tFlowControl.cs => StreamInputFlowControl.cs} | 12 ++++++------ ...FlowControl.cs => StreamOutputFlowControl.cs} | 14 +++++++------- .../Internal/Http2/Http2Connection.cs | 4 ++-- .../Internal/Http2/Http2FrameWriter.cs | 14 +++++++------- .../Internal/Http2/Http2OutputProducer.cs | 4 ++-- src/Kestrel.Core/Internal/Http2/Http2Stream.cs | 8 ++++---- .../Internal/Http2/Http2StreamContext.cs | 4 ++-- 11 files changed, 46 insertions(+), 46 deletions(-) rename src/Kestrel.Core/Internal/Http2/FlowControl/{Http2FlowControl.cs => FlowControl.cs} (94%) rename src/Kestrel.Core/Internal/Http2/FlowControl/{Http2InputFlowControl.cs => InputFlowControl.cs} (94%) rename src/Kestrel.Core/Internal/Http2/FlowControl/{Http2OutputFlowControl.cs => OutputFlowControl.cs} (79%) rename src/Kestrel.Core/Internal/Http2/FlowControl/{Http2OutputFlowControlAwaitable.cs => OutputFlowControlAwaitable.cs} (90%) rename src/Kestrel.Core/Internal/Http2/FlowControl/{Http2StreamInputFlowControl.cs => StreamInputFlowControl.cs} (88%) rename src/Kestrel.Core/Internal/Http2/FlowControl/{Http2StreamOutputFlowControl.cs => StreamOutputFlowControl.cs} (83%) diff --git a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2FlowControl.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/FlowControl.cs similarity index 94% rename from src/Kestrel.Core/Internal/Http2/FlowControl/Http2FlowControl.cs rename to src/Kestrel.Core/Internal/Http2/FlowControl/FlowControl.cs index d857dca0d..779f9a988 100644 --- a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2FlowControl.cs +++ b/src/Kestrel.Core/Internal/Http2/FlowControl/FlowControl.cs @@ -5,9 +5,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.FlowControl { - public struct Http2FlowControl + public struct FlowControl { - public Http2FlowControl(uint initialWindowSize) + public FlowControl(uint initialWindowSize) { Debug.Assert(initialWindowSize <= Http2PeerSettings.MaxWindowSize, $"{nameof(initialWindowSize)} too large."); diff --git a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/InputFlowControl.cs similarity index 94% rename from src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs rename to src/Kestrel.Core/Internal/Http2/FlowControl/InputFlowControl.cs index dedee593d..d4387fe8d 100644 --- a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2InputFlowControl.cs +++ b/src/Kestrel.Core/Internal/Http2/FlowControl/InputFlowControl.cs @@ -5,21 +5,21 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.FlowControl { - public class Http2InputFlowControl + public class InputFlowControl { private readonly int _initialWindowSize; private readonly int _minWindowSizeIncrement; - private Http2FlowControl _flow; + private FlowControl _flow; private int _pendingUpdateSize; private bool _windowUpdatesDisabled; private readonly object _flowLock = new object(); - public Http2InputFlowControl(uint initialWindowSize, uint minWindowSizeIncrement) + public InputFlowControl(uint initialWindowSize, uint minWindowSizeIncrement) { Debug.Assert(initialWindowSize >= minWindowSizeIncrement, "minWindowSizeIncrement is greater than the window size."); - _flow = new Http2FlowControl(initialWindowSize); + _flow = new FlowControl(initialWindowSize); _initialWindowSize = (int)initialWindowSize; _minWindowSizeIncrement = (int)minWindowSizeIncrement; } diff --git a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2OutputFlowControl.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/OutputFlowControl.cs similarity index 79% rename from src/Kestrel.Core/Internal/Http2/FlowControl/Http2OutputFlowControl.cs rename to src/Kestrel.Core/Internal/Http2/FlowControl/OutputFlowControl.cs index 75c7ed93c..5b3282a55 100644 --- a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2OutputFlowControl.cs +++ b/src/Kestrel.Core/Internal/Http2/FlowControl/OutputFlowControl.cs @@ -6,20 +6,20 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.FlowControl { - public class Http2OutputFlowControl + public class OutputFlowControl { - private Http2FlowControl _flow; - private Queue _awaitableQueue; + private FlowControl _flow; + private Queue _awaitableQueue; - public Http2OutputFlowControl(uint initialWindowSize) + public OutputFlowControl(uint initialWindowSize) { - _flow = new Http2FlowControl(initialWindowSize); + _flow = new FlowControl(initialWindowSize); } public int Available => _flow.Available; public bool IsAborted => _flow.IsAborted; - public Http2OutputFlowControlAwaitable AvailabilityAwaitable + public OutputFlowControlAwaitable AvailabilityAwaitable { get { @@ -28,10 +28,10 @@ public Http2OutputFlowControlAwaitable AvailabilityAwaitable if (_awaitableQueue == null) { - _awaitableQueue = new Queue(); + _awaitableQueue = new Queue(); } - var awaitable = new Http2OutputFlowControlAwaitable(); + var awaitable = new OutputFlowControlAwaitable(); _awaitableQueue.Enqueue(awaitable); return awaitable; } diff --git a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2OutputFlowControlAwaitable.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/OutputFlowControlAwaitable.cs similarity index 90% rename from src/Kestrel.Core/Internal/Http2/FlowControl/Http2OutputFlowControlAwaitable.cs rename to src/Kestrel.Core/Internal/Http2/FlowControl/OutputFlowControlAwaitable.cs index 5da9115e6..48d15e345 100644 --- a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2OutputFlowControlAwaitable.cs +++ b/src/Kestrel.Core/Internal/Http2/FlowControl/OutputFlowControlAwaitable.cs @@ -8,13 +8,13 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.FlowControl { - public class Http2OutputFlowControlAwaitable : ICriticalNotifyCompletion + public class OutputFlowControlAwaitable : ICriticalNotifyCompletion { private static readonly Action _callbackCompleted = () => { }; private Action _callback; - public Http2OutputFlowControlAwaitable GetAwaiter() => this; + public OutputFlowControlAwaitable GetAwaiter() => this; public bool IsCompleted => ReferenceEquals(_callback, _callbackCompleted); public void GetResult() diff --git a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamInputFlowControl.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/StreamInputFlowControl.cs similarity index 88% rename from src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamInputFlowControl.cs rename to src/Kestrel.Core/Internal/Http2/FlowControl/StreamInputFlowControl.cs index ef2de73d4..e85b2bbe2 100644 --- a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamInputFlowControl.cs +++ b/src/Kestrel.Core/Internal/Http2/FlowControl/StreamInputFlowControl.cs @@ -5,23 +5,23 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.FlowControl { - public class Http2StreamInputFlowControl + public class StreamInputFlowControl { - private readonly Http2InputFlowControl _connectionLevelFlowControl; - private readonly Http2InputFlowControl _streamLevelFlowControl; + private readonly InputFlowControl _connectionLevelFlowControl; + private readonly InputFlowControl _streamLevelFlowControl; private readonly int _streamId; private readonly Http2FrameWriter _frameWriter; - public Http2StreamInputFlowControl( + public StreamInputFlowControl( int streamId, Http2FrameWriter frameWriter, - Http2InputFlowControl connectionLevelFlowControl, + InputFlowControl connectionLevelFlowControl, uint initialWindowSize, uint minWindowSizeIncrement) { _connectionLevelFlowControl = connectionLevelFlowControl; - _streamLevelFlowControl = new Http2InputFlowControl(initialWindowSize, minWindowSizeIncrement); + _streamLevelFlowControl = new InputFlowControl(initialWindowSize, minWindowSizeIncrement); _streamId = streamId; _frameWriter = frameWriter; diff --git a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamOutputFlowControl.cs b/src/Kestrel.Core/Internal/Http2/FlowControl/StreamOutputFlowControl.cs similarity index 83% rename from src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamOutputFlowControl.cs rename to src/Kestrel.Core/Internal/Http2/FlowControl/StreamOutputFlowControl.cs index ebcd2f3e2..99e58077f 100644 --- a/src/Kestrel.Core/Internal/Http2/FlowControl/Http2StreamOutputFlowControl.cs +++ b/src/Kestrel.Core/Internal/Http2/FlowControl/StreamOutputFlowControl.cs @@ -7,17 +7,17 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.FlowControl { - public class Http2StreamOutputFlowControl + public class StreamOutputFlowControl { - private readonly Http2OutputFlowControl _connectionLevelFlowControl; - private readonly Http2OutputFlowControl _streamLevelFlowControl; + private readonly OutputFlowControl _connectionLevelFlowControl; + private readonly OutputFlowControl _streamLevelFlowControl; - private Http2OutputFlowControlAwaitable _currentConnectionLevelAwaitable; + private OutputFlowControlAwaitable _currentConnectionLevelAwaitable; - public Http2StreamOutputFlowControl(Http2OutputFlowControl connectionLevelFlowControl, uint initialWindowSize) + public StreamOutputFlowControl(OutputFlowControl connectionLevelFlowControl, uint initialWindowSize) { _connectionLevelFlowControl = connectionLevelFlowControl; - _streamLevelFlowControl = new Http2OutputFlowControl(initialWindowSize); + _streamLevelFlowControl = new OutputFlowControl(initialWindowSize); } public int Available => Math.Min(_connectionLevelFlowControl.Available, _streamLevelFlowControl.Available); @@ -30,7 +30,7 @@ public void Advance(int bytes) _streamLevelFlowControl.Advance(bytes); } - public int AdvanceUpToAndWait(long bytes, out Http2OutputFlowControlAwaitable awaitable) + public int AdvanceUpToAndWait(long bytes, out OutputFlowControlAwaitable awaitable) { var leastAvailableFlow = _connectionLevelFlowControl.Available < _streamLevelFlowControl.Available ? _connectionLevelFlowControl : _streamLevelFlowControl; diff --git a/src/Kestrel.Core/Internal/Http2/Http2Connection.cs b/src/Kestrel.Core/Internal/Http2/Http2Connection.cs index 756207ea1..1267a0fc2 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2Connection.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2Connection.cs @@ -62,8 +62,8 @@ private enum PseudoHeaderFields private readonly Http2ConnectionContext _context; private readonly Http2FrameWriter _frameWriter; private readonly HPackDecoder _hpackDecoder; - private readonly Http2InputFlowControl _inputFlowControl = new Http2InputFlowControl(Http2PeerSettings.DefaultInitialWindowSize, Http2PeerSettings.DefaultInitialWindowSize / 2); - private readonly Http2OutputFlowControl _outputFlowControl = new Http2OutputFlowControl(Http2PeerSettings.DefaultInitialWindowSize); + private readonly InputFlowControl _inputFlowControl = new InputFlowControl(Http2PeerSettings.DefaultInitialWindowSize, Http2PeerSettings.DefaultInitialWindowSize / 2); + private readonly OutputFlowControl _outputFlowControl = new OutputFlowControl(Http2PeerSettings.DefaultInitialWindowSize); private readonly Http2PeerSettings _serverSettings = new Http2PeerSettings(); private readonly Http2PeerSettings _clientSettings = new Http2PeerSettings(); diff --git a/src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs b/src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs index 5c5703308..066e88d06 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs @@ -26,7 +26,7 @@ public class Http2FrameWriter private readonly HPackEncoder _hpackEncoder = new HPackEncoder(); private readonly PipeWriter _outputWriter; private readonly PipeReader _outputReader; - private readonly Http2OutputFlowControl _connectionOutputFlowControl; + private readonly OutputFlowControl _connectionOutputFlowControl; private readonly StreamSafePipeFlusher _flusher; private bool _completed; @@ -34,7 +34,7 @@ public class Http2FrameWriter public Http2FrameWriter( PipeWriter outputPipeWriter, PipeReader outputPipeReader, - Http2OutputFlowControl connectionOutputFlowControl, + OutputFlowControl connectionOutputFlowControl, ITimeoutControl timeoutControl) { _outputWriter = outputPipeWriter; @@ -129,7 +129,7 @@ public void WriteResponseHeaders(int streamId, int statusCode, IHeaderDictionary } } - public Task WriteDataAsync(int streamId, Http2StreamOutputFlowControl flowControl, ReadOnlySequence data, bool endStream) + public Task WriteDataAsync(int streamId, StreamOutputFlowControl flowControl, ReadOnlySequence data, bool endStream) { // The Length property of a ReadOnlySequence can be expensive, so we cache the value. var dataLength = data.Length; @@ -194,11 +194,11 @@ private Task WriteDataUnsynchronizedAsync(int streamId, ReadOnlySequence d return _flusher.FlushAsync(); } - private async Task WriteDataAsyncAwaited(int streamId, Http2StreamOutputFlowControl flowControl, ReadOnlySequence data, long dataLength, bool endStream) + private async Task WriteDataAsyncAwaited(int streamId, StreamOutputFlowControl flowControl, ReadOnlySequence data, long dataLength, bool endStream) { while (dataLength > 0) { - Http2OutputFlowControlAwaitable availabilityAwaitable; + OutputFlowControlAwaitable availabilityAwaitable; var writeTask = Task.CompletedTask; lock (_writeLock) @@ -315,7 +315,7 @@ public bool TryUpdateConnectionWindow(int bytes) } } - public bool TryUpdateStreamWindow(Http2StreamOutputFlowControl flowControl, int bytes) + public bool TryUpdateStreamWindow(StreamOutputFlowControl flowControl, int bytes) { lock (_writeLock) { @@ -323,7 +323,7 @@ public bool TryUpdateStreamWindow(Http2StreamOutputFlowControl flowControl, int } } - public void AbortPendingStreamDataWrites(Http2StreamOutputFlowControl flowControl) + public void AbortPendingStreamDataWrites(StreamOutputFlowControl flowControl) { lock (_writeLock) { diff --git a/src/Kestrel.Core/Internal/Http2/Http2OutputProducer.cs b/src/Kestrel.Core/Internal/Http2/Http2OutputProducer.cs index 8c1e49f95..1a9f88b4c 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2OutputProducer.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2OutputProducer.cs @@ -23,7 +23,7 @@ public class Http2OutputProducer : IHttpOutputProducer // This should only be accessed via the FrameWriter. The connection-level output flow control is protected by the // FrameWriter's connection-level write lock. - private readonly Http2StreamOutputFlowControl _flowControl; + private readonly StreamOutputFlowControl _flowControl; private readonly object _dataWriterLock = new object(); private readonly Pipe _dataPipe; @@ -35,7 +35,7 @@ public class Http2OutputProducer : IHttpOutputProducer public Http2OutputProducer( int streamId, Http2FrameWriter frameWriter, - Http2StreamOutputFlowControl flowControl, + StreamOutputFlowControl flowControl, ITimeoutControl timeoutControl, MemoryPool pool) { diff --git a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs index 9ef161f0e..a0828e7ed 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs @@ -21,8 +21,8 @@ public partial class Http2Stream : HttpProtocol { private readonly Http2StreamContext _context; private readonly Http2OutputProducer _http2Output; - private readonly Http2StreamInputFlowControl _inputFlowControl; - private readonly Http2StreamOutputFlowControl _outputFlowControl; + private readonly StreamInputFlowControl _inputFlowControl; + private readonly StreamOutputFlowControl _outputFlowControl; private StreamCompletionFlags _completionState; private readonly object _completionLock = new object(); @@ -32,14 +32,14 @@ public Http2Stream(Http2StreamContext context) { _context = context; - _inputFlowControl = new Http2StreamInputFlowControl( + _inputFlowControl = new StreamInputFlowControl( _context.StreamId, _context.FrameWriter, context.ConnectionInputFlowControl, Http2PeerSettings.DefaultInitialWindowSize, Http2PeerSettings.DefaultInitialWindowSize / 2); - _outputFlowControl = new Http2StreamOutputFlowControl(context.ConnectionOutputFlowControl, context.ClientPeerSettings.InitialWindowSize); + _outputFlowControl = new StreamOutputFlowControl(context.ConnectionOutputFlowControl, context.ClientPeerSettings.InitialWindowSize); _http2Output = new Http2OutputProducer(context.StreamId, context.FrameWriter, _outputFlowControl, context.TimeoutControl, context.MemoryPool); RequestBodyPipe = CreateRequestBodyPipe(); diff --git a/src/Kestrel.Core/Internal/Http2/Http2StreamContext.cs b/src/Kestrel.Core/Internal/Http2/Http2StreamContext.cs index 7f47bb9c7..698b29f74 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2StreamContext.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2StreamContext.cs @@ -22,8 +22,8 @@ public class Http2StreamContext : IHttpProtocolContext public IHttp2StreamLifetimeHandler StreamLifetimeHandler { get; set; } public Http2PeerSettings ClientPeerSettings { get; set; } public Http2FrameWriter FrameWriter { get; set; } - public Http2InputFlowControl ConnectionInputFlowControl { get; set; } - public Http2OutputFlowControl ConnectionOutputFlowControl { get; set; } + public InputFlowControl ConnectionInputFlowControl { get; set; } + public OutputFlowControl ConnectionOutputFlowControl { get; set; } public ITimeoutControl TimeoutControl { get; set; } } }