From 75f4159e5a1a96e4d859f0244bd5acca06397ca5 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Mon, 29 Jul 2019 10:14:14 -0700 Subject: [PATCH] Accurately count only newly examined bytes (#12639) - Ensure Kestrel count all bytes read using TryRead - Ensure Kestrel doesn't double count examined but not consumed bytes --- .../Http/Http1ChunkedEncodingMessageBody.cs | 4 +- .../Http/Http1ContentLengthMessageBody.cs | 20 ++--- .../Core/src/Internal/Http/MessageBody.cs | 73 ++++++++++++++++++- .../src/Internal/Http2/Http2MessageBody.cs | 45 +----------- 4 files changed, 80 insertions(+), 62 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ChunkedEncodingMessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ChunkedEncodingMessageBody.cs index b2c255c3df..5c981dd16a 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ChunkedEncodingMessageBody.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ChunkedEncodingMessageBody.cs @@ -44,9 +44,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http public override void AdvanceTo(SequencePosition consumed, SequencePosition examined) { - var dataLength = _readResult.Buffer.Slice(_readResult.Buffer.Start, consumed).Length; + OnAdvance(_readResult, consumed, examined); _requestBodyPipe.Reader.AdvanceTo(consumed, examined); - OnDataRead(dataLength); } public override bool TryRead(out ReadResult readResult) @@ -63,6 +62,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http var boolResult = _requestBodyPipe.Reader.TryRead(out _readResult); readResult = _readResult; + CountBytesRead(readResult.Buffer.Length); if (_readResult.IsCompleted) { diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs index 62c6dd331f..18d8b3e6c9 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs @@ -17,7 +17,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http private bool _readCompleted; private bool _isReading; private int _userCanceled; - private long _totalExaminedInPreviousReadResult; private bool _finalAdvanceCalled; public Http1ContentLengthMessageBody(bool keepAlive, long contentLength, Http1Connection context) @@ -85,6 +84,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http if (Interlocked.Exchange(ref _userCanceled, 0) == 1) { // Ignore the readResult if it wasn't by the user. + CreateReadResultFromConnectionReadResult(); + break; } else @@ -156,6 +157,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http CreateReadResultFromConnectionReadResult(); readResult = _readResult; + CountBytesRead(readResult.Buffer.Length); return true; } @@ -174,11 +176,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http private void CreateReadResultFromConnectionReadResult() { - if (_readResult.Buffer.Length >= _inputLength + _totalExaminedInPreviousReadResult) + if (_readResult.Buffer.Length >= _inputLength + _examinedUnconsumedBytes) { _readCompleted = true; _readResult = new ReadResult( - _readResult.Buffer.Slice(0, _inputLength + _totalExaminedInPreviousReadResult), + _readResult.Buffer.Slice(0, _inputLength + _examinedUnconsumedBytes), _readResult.IsCanceled && Interlocked.Exchange(ref _userCanceled, 0) == 1, _readCompleted); } @@ -217,18 +219,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http return; } - var consumedLength = _readResult.Buffer.Slice(_readResult.Buffer.Start, consumed).Length; - var examinedLength = consumedLength + _readResult.Buffer.Slice(consumed, examined).Length; - + _inputLength -= OnAdvance(_readResult, consumed, examined); _context.Input.AdvanceTo(consumed, examined); - - var newlyExamined = examinedLength - _totalExaminedInPreviousReadResult; - - OnDataRead(newlyExamined); - _totalExaminedInPreviousReadResult += newlyExamined; - _inputLength -= newlyExamined; - - _totalExaminedInPreviousReadResult -= consumedLength; } protected override void OnReadStarting() diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/MessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http/MessageBody.cs index c2c6def74d..bae364ca56 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/MessageBody.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/MessageBody.cs @@ -23,6 +23,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http protected bool _timingEnabled; protected bool _backpressure; protected long _alreadyTimedBytes; + protected long _examinedUnconsumedBytes; protected MessageBody(HttpProtocol context) { @@ -165,10 +166,19 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http return readAwaitable; } - protected void StopTimingRead(long bytesRead) + protected void CountBytesRead(long bytesInReadResult) { - _context.TimeoutControl.BytesRead(bytesRead - _alreadyTimedBytes); - _alreadyTimedBytes = 0; + var numFirstSeenBytes = bytesInReadResult - _alreadyTimedBytes; + + if (numFirstSeenBytes > 0) + { + _context.TimeoutControl.BytesRead(numFirstSeenBytes); + } + } + + protected void StopTimingRead(long bytesInReadResult) + { + CountBytesRead(bytesInReadResult); if (_backpressure) { @@ -176,5 +186,62 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http _context.TimeoutControl.StopTimingRead(); } } + + protected long OnAdvance(ReadResult readResult, SequencePosition consumed, SequencePosition examined) + { + // This code path is fairly hard to understand so let's break it down with an example + // ReadAsync returns a ReadResult of length 50. + // Advance(25, 40). The examined length would be 40 and consumed length would be 25. + // _totalExaminedInPreviousReadResult starts at 0. newlyExamined is 40. + // OnDataRead is called with length 40. + // _totalExaminedInPreviousReadResult is now 40 - 25 = 15. + + // The next call to ReadAsync returns 50 again + // Advance(5, 5) is called + // newlyExamined is 5 - 15, or -10. + // Update _totalExaminedInPreviousReadResult to 10 as we consumed 5. + + // The next call to ReadAsync returns 50 again + // _totalExaminedInPreviousReadResult is 10 + // Advance(50, 50) is called + // newlyExamined = 50 - 10 = 40 + // _totalExaminedInPreviousReadResult is now 50 + // _totalExaminedInPreviousReadResult is finally 0 after subtracting consumedLength. + + long examinedLength, consumedLength, totalLength; + + if (consumed.Equals(examined)) + { + examinedLength = readResult.Buffer.Slice(readResult.Buffer.Start, examined).Length; + consumedLength = examinedLength; + } + else + { + consumedLength = readResult.Buffer.Slice(readResult.Buffer.Start, consumed).Length; + examinedLength = consumedLength + readResult.Buffer.Slice(consumed, examined).Length; + } + + if (examined.Equals(readResult.Buffer.End)) + { + totalLength = examinedLength; + } + else + { + totalLength = readResult.Buffer.Length; + } + + var newlyExamined = examinedLength - _examinedUnconsumedBytes; + + if (newlyExamined > 0) + { + OnDataRead(newlyExamined); + _examinedUnconsumedBytes += newlyExamined; + } + + _examinedUnconsumedBytes -= consumedLength; + _alreadyTimedBytes = totalLength - consumedLength; + + return newlyExamined; + } } } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2MessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2MessageBody.cs index 4ef0cc24df..7edd167138 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2MessageBody.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2MessageBody.cs @@ -14,7 +14,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 { private readonly Http2Stream _context; private ReadResult _readResult; - private long _alreadyExaminedInNextReadResult; private Http2MessageBody(Http2Stream context) : base(context) @@ -64,55 +63,15 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 public override void AdvanceTo(SequencePosition consumed, SequencePosition examined) { - // This code path is fairly hard to understand so let's break it down with an example - // ReadAsync returns a ReadResult of length 50. - // Advance(25, 40). The examined length would be 40 and consumed length would be 25. - // _totalExaminedInPreviousReadResult starts at 0. newlyExamined is 40. - // OnDataRead is called with length 40. - // _totalExaminedInPreviousReadResult is now 40 - 25 = 15. - - // The next call to ReadAsync returns 50 again - // Advance(5, 5) is called - // newlyExamined is 5 - 15, or -10. - // Update _totalExaminedInPreviousReadResult to 10 as we consumed 5. - - // The next call to ReadAsync returns 50 again - // _totalExaminedInPreviousReadResult is 10 - // Advance(50, 50) is called - // newlyExamined = 50 - 10 = 40 - // _totalExaminedInPreviousReadResult is now 50 - // _totalExaminedInPreviousReadResult is finally 0 after subtracting consumedLength. - - long examinedLength; - long consumedLength; - if (consumed.Equals(examined)) - { - examinedLength = _readResult.Buffer.Slice(_readResult.Buffer.Start, examined).Length; - consumedLength = examinedLength; - } - else - { - consumedLength = _readResult.Buffer.Slice(_readResult.Buffer.Start, consumed).Length; - examinedLength = consumedLength + _readResult.Buffer.Slice(consumed, examined).Length; - } - + OnAdvance(_readResult, consumed, examined); _context.RequestBodyPipe.Reader.AdvanceTo(consumed, examined); - - var newlyExamined = examinedLength - _alreadyExaminedInNextReadResult; - - if (newlyExamined > 0) - { - OnDataRead(newlyExamined); - _alreadyExaminedInNextReadResult += newlyExamined; - } - - _alreadyExaminedInNextReadResult -= consumedLength; } public override bool TryRead(out ReadResult readResult) { var result = _context.RequestBodyPipe.Reader.TryRead(out readResult); _readResult = readResult; + CountBytesRead(readResult.Buffer.Length); return result; }