From c3556da401b0f6f0ae8a857c129bdb7a21da4768 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Tue, 5 Mar 2019 09:54:43 -0800 Subject: [PATCH 01/10] Need a similar solution that http2 has --- .../Http/Http1ContentLengthMessageBody.cs | 6 +-- .../InMemory.FunctionalTests/RequestTests.cs | 37 +++++++++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs index 289ecac406f2..e78063e7e0f5 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs @@ -31,8 +31,7 @@ public override async ValueTask ReadAsync(CancellationToken cancella if (_inputLength == 0) { - _readResult = new ReadResult(default, isCanceled: false, isCompleted: true); - return _readResult; + _readResult = new ReadResult(_readResult.Buffer, isCanceled: false, isCompleted: true); } TryStart(); @@ -170,7 +169,8 @@ public override void AdvanceTo(SequencePosition consumed, SequencePosition exami return; } - var dataLength = _readResult.Buffer.Slice(_readResult.Buffer.Start, consumed).Length; + // Need prevExamined.... + var dataLength = _readResult.Buffer.Slice(_readResult.Buffer.Start, examined).Length; _inputLength -= dataLength; diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs index 16cd2dee2d84..f48fa95c3068 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs @@ -727,6 +727,43 @@ await connection.ReceiveEnd( } } + [Fact] + public async Task ContentLengthReadAsyncPipeReaderBufferRequestBody() + { + var testContext = new TestServiceContext(LoggerFactory); + + using (var server = new TestServer(async httpContext => + { + var readResult = await httpContext.Request.BodyPipe.ReadAsync(); + // This will hang if 0 content length is not assumed by the server + Assert.Equal(5, readResult.Buffer.Length); + httpContext.Request.BodyPipe.AdvanceTo(readResult.Buffer.Start, readResult.Buffer.End); + readResult = await httpContext.Request.BodyPipe.ReadAsync(); + Assert.Equal(5, readResult.Buffer.Length); + + }, testContext)) + { + using (var connection = server.CreateConnection()) + { + await connection.SendAll( + "POST / HTTP/1.0", + "Host:", + "Content-Length: 5", + "", + "hello"); + await connection.ReceiveEnd( + "HTTP/1.1 200 OK", + "Connection: close", + $"Date: {testContext.DateHeaderValue}", + "Content-Length: 0", + "", + ""); + } + + await server.StopAsync(); + } + } + [Fact] public async Task ConnectionClosesWhenFinReceivedBeforeRequestCompletes() { From 62cbe3251a2ad1e51dcca7f82a964152a7088d93 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Tue, 5 Mar 2019 19:27:33 -0800 Subject: [PATCH 02/10] Prototype of fix --- .../Http/Http1ContentLengthMessageBody.cs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs index e78063e7e0f5..dc96de7ac1a9 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs @@ -13,6 +13,7 @@ public class Http1ContentLengthMessageBody : Http1MessageBody { private readonly long _contentLength; private long _inputLength; + private bool _readCompleted; private ReadResult _readResult; private bool _completed; private int _userCanceled; @@ -29,9 +30,9 @@ public override async ValueTask ReadAsync(CancellationToken cancella { ThrowIfCompleted(); - if (_inputLength == 0) + if (_readCompleted) { - _readResult = new ReadResult(_readResult.Buffer, isCanceled: false, isCompleted: true); + return _readResult; } TryStart(); @@ -144,11 +145,13 @@ private void CreateReadResultFromConnectionReadResult() { if (_readResult.Buffer.Length > _inputLength) { - _readResult = new ReadResult(_readResult.Buffer.Slice(0, _inputLength), _readResult.IsCanceled, isCompleted: true); + _readCompleted = true; + _readResult = new ReadResult(_readResult.Buffer.Slice(0, _inputLength), _readResult.IsCanceled, _readCompleted); } else if (_readResult.Buffer.Length == _inputLength) { - _readResult = new ReadResult(_readResult.Buffer, _readResult.IsCanceled, isCompleted: true); + _readCompleted = true; + _readResult = new ReadResult(_readResult.Buffer, _readResult.IsCanceled, _readCompleted); } if (_readResult.IsCompleted) @@ -169,7 +172,11 @@ public override void AdvanceTo(SequencePosition consumed, SequencePosition exami return; } - // Need prevExamined.... + if (_readCompleted) + { + _readResult = new ReadResult(_readResult.Buffer.Slice(consumed, _readResult.Buffer.End), isCanceled: false, _readCompleted); + } + var dataLength = _readResult.Buffer.Slice(_readResult.Buffer.Start, examined).Length; _inputLength -= dataLength; From b9634055a506a2dbe52b34698576b743aef1ab20 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Wed, 6 Mar 2019 12:31:44 -0800 Subject: [PATCH 03/10] I hate this solution with a passion but whatever --- .../Http/Http1ContentLengthMessageBody.cs | 39 +++++++++++-------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs index dc96de7ac1a9..584c17692b40 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs @@ -14,9 +14,11 @@ public class Http1ContentLengthMessageBody : Http1MessageBody private readonly long _contentLength; private long _inputLength; private bool _readCompleted; + private bool _firstAdvance; private ReadResult _readResult; private bool _completed; private int _userCanceled; + private long _totalExaminedInPreviousReadResult; public Http1ContentLengthMessageBody(bool keepAlive, long contentLength, Http1Connection context) : base(context) @@ -102,9 +104,9 @@ public override bool TryRead(out ReadResult readResult) { ThrowIfCompleted(); - if (_inputLength == 0) + if (_readCompleted) { - readResult = new ReadResult(default, isCanceled: false, isCompleted: true); + readResult = _readResult; return true; } @@ -143,16 +145,11 @@ private void ThrowIfCompleted() private void CreateReadResultFromConnectionReadResult() { - if (_readResult.Buffer.Length > _inputLength) + if (_readResult.Buffer.Length >= _inputLength) { _readCompleted = true; _readResult = new ReadResult(_readResult.Buffer.Slice(0, _inputLength), _readResult.IsCanceled, _readCompleted); } - else if (_readResult.Buffer.Length == _inputLength) - { - _readCompleted = true; - _readResult = new ReadResult(_readResult.Buffer, _readResult.IsCanceled, _readCompleted); - } if (_readResult.IsCompleted) { @@ -167,23 +164,31 @@ public override void AdvanceTo(SequencePosition consumed) public override void AdvanceTo(SequencePosition consumed, SequencePosition examined) { - if (_inputLength == 0) - { - return; - } + var examinedLength = _readResult.Buffer.Slice(_readResult.Buffer.Start, examined).Length; + var consumedLength = _readResult.Buffer.Slice(_readResult.Buffer.Start, consumed).Length; if (_readCompleted) { _readResult = new ReadResult(_readResult.Buffer.Slice(consumed, _readResult.Buffer.End), isCanceled: false, _readCompleted); + if (!_firstAdvance) + { + _context.Input.AdvanceTo(consumed, examined); + _firstAdvance = true; + } + return; } - var dataLength = _readResult.Buffer.Slice(_readResult.Buffer.Start, examined).Length; - - _inputLength -= dataLength; - _context.Input.AdvanceTo(consumed, examined); - OnDataRead(dataLength); + var newlyExamined = examinedLength - _totalExaminedInPreviousReadResult; + if (newlyExamined > 0) + { + OnDataRead(newlyExamined); + _totalExaminedInPreviousReadResult += newlyExamined; + _inputLength -= newlyExamined; + } + + _totalExaminedInPreviousReadResult -= consumedLength; } protected override void OnReadStarting() From 781f611a756d466908d55fba1296d9cfb2a86bc6 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Wed, 6 Mar 2019 15:39:02 -0800 Subject: [PATCH 04/10] bodyReader --- .../Kestrel/test/InMemory.FunctionalTests/RequestTests.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs index f48fa95c3068..e12b67933855 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs @@ -734,11 +734,11 @@ public async Task ContentLengthReadAsyncPipeReaderBufferRequestBody() using (var server = new TestServer(async httpContext => { - var readResult = await httpContext.Request.BodyPipe.ReadAsync(); + var readResult = await httpContext.Request.BodyReader.ReadAsync(); // This will hang if 0 content length is not assumed by the server Assert.Equal(5, readResult.Buffer.Length); - httpContext.Request.BodyPipe.AdvanceTo(readResult.Buffer.Start, readResult.Buffer.End); - readResult = await httpContext.Request.BodyPipe.ReadAsync(); + httpContext.Request.BodyReader.AdvanceTo(readResult.Buffer.Start, readResult.Buffer.End); + readResult = await httpContext.Request.BodyReader.ReadAsync(); Assert.Equal(5, readResult.Buffer.Length); }, testContext)) From 6cd36efaca7c2647bbc8624ec06ca956d1f32442 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Thu, 7 Mar 2019 18:27:07 -0800 Subject: [PATCH 05/10] Make Read/Advance throw appropriately --- .../Http/Http1ContentLengthMessageBody.cs | 43 ++++++++++++--- .../Kestrel/Core/test/MessageBodyTests.cs | 4 +- .../InMemory.FunctionalTests/RequestTests.cs | 52 +++++++++++++++++++ 3 files changed, 90 insertions(+), 9 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs index 584c17692b40..1209cac65af3 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs @@ -11,12 +11,13 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http { public class Http1ContentLengthMessageBody : Http1MessageBody { + private ReadResult _readResult; private readonly long _contentLength; private long _inputLength; private bool _readCompleted; - private bool _firstAdvance; - private ReadResult _readResult; + private bool _finalAdvanceAfterReadingEntireContentLength; private bool _completed; + private bool _isReading; private int _userCanceled; private long _totalExaminedInPreviousReadResult; @@ -32,8 +33,14 @@ public override async ValueTask ReadAsync(CancellationToken cancella { ThrowIfCompleted(); + if (_isReading) + { + throw new InvalidOperationException("Reading is already in progress."); + } + if (_readCompleted) { + _isReading = true; return _readResult; } @@ -55,6 +62,8 @@ public override async ValueTask ReadAsync(CancellationToken cancella try { var readAwaitable = _context.Input.ReadAsync(cancellationToken); + + _isReading = true; _readResult = await StartTimingReadAsync(readAwaitable, cancellationToken); } catch (ConnectionAbortedException ex) @@ -104,6 +113,11 @@ public override bool TryRead(out ReadResult readResult) { ThrowIfCompleted(); + if (_isReading) + { + throw new InvalidOperationException("Reading is already in progress."); + } + if (_readCompleted) { readResult = _readResult; @@ -112,6 +126,7 @@ public override bool TryRead(out ReadResult readResult) TryStart(); + if (!_context.Input.TryRead(out _readResult)) { readResult = default; @@ -128,6 +143,9 @@ public override bool TryRead(out ReadResult readResult) } } + // Only set _isReading if we are returing a ReadResult. + _isReading = true; + CreateReadResultFromConnectionReadResult(); readResult = _readResult; @@ -145,10 +163,10 @@ private void ThrowIfCompleted() private void CreateReadResultFromConnectionReadResult() { - if (_readResult.Buffer.Length >= _inputLength) + if (_readResult.Buffer.Length >= _inputLength + _totalExaminedInPreviousReadResult) { _readCompleted = true; - _readResult = new ReadResult(_readResult.Buffer.Slice(0, _inputLength), _readResult.IsCanceled, _readCompleted); + _readResult = new ReadResult(_readResult.Buffer.Slice(0, _inputLength + _totalExaminedInPreviousReadResult), _readResult.IsCanceled, _readCompleted); } if (_readResult.IsCompleted) @@ -164,20 +182,29 @@ public override void AdvanceTo(SequencePosition consumed) public override void AdvanceTo(SequencePosition consumed, SequencePosition examined) { - var examinedLength = _readResult.Buffer.Slice(_readResult.Buffer.Start, examined).Length; - var consumedLength = _readResult.Buffer.Slice(_readResult.Buffer.Start, consumed).Length; + if (!_isReading) + { + throw new InvalidOperationException("No reading operation to complete."); + } + + _isReading = false; if (_readCompleted) { _readResult = new ReadResult(_readResult.Buffer.Slice(consumed, _readResult.Buffer.End), isCanceled: false, _readCompleted); - if (!_firstAdvance) + + if (!_finalAdvanceAfterReadingEntireContentLength) { _context.Input.AdvanceTo(consumed, examined); - _firstAdvance = true; + _finalAdvanceAfterReadingEntireContentLength = true; } + return; } + var consumedLength = _readResult.Buffer.Slice(_readResult.Buffer.Start, consumed).Length; + var examinedLength = consumedLength + _readResult.Buffer.Slice(consumed, examined).Length; + _context.Input.AdvanceTo(consumed, examined); var newlyExamined = examinedLength - _totalExaminedInPreviousReadResult; diff --git a/src/Servers/Kestrel/Core/test/MessageBodyTests.cs b/src/Servers/Kestrel/Core/test/MessageBodyTests.cs index bd11ad9d81f5..e46adb9534b7 100644 --- a/src/Servers/Kestrel/Core/test/MessageBodyTests.cs +++ b/src/Servers/Kestrel/Core/test/MessageBodyTests.cs @@ -778,8 +778,10 @@ public async Task CopyToAsyncThrowsOnTimeout() // Add some input and read it to start PumpAsync input.Add("a"); - Assert.Equal(1, (await body.ReadAsync()).Buffer.Length); + var readResult = await body.ReadAsync(); + Assert.Equal(1, readResult.Buffer.Length); + body.AdvanceTo(readResult.Buffer.End); // Time out on the next read input.Http1Connection.SendTimeoutResponse(); diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs index e12b67933855..0b41882ecfe7 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs @@ -764,6 +764,58 @@ await connection.ReceiveEnd( } } + [Fact] + public async Task ContentLengthReadAsyncSingleBytesAtATime() + { + var testContext = new TestServiceContext(LoggerFactory); + var tcs = new TaskCompletionSource(); + var tcs2 = new TaskCompletionSource(); + using (var server = new TestServer(async httpContext => + { + var readResult = await httpContext.Request.BodyReader.ReadAsync(); + Assert.Equal(3, readResult.Buffer.Length); + tcs.SetResult(null); + + httpContext.Request.BodyReader.AdvanceTo(readResult.Buffer.Start, readResult.Buffer.End); + + // Check AdvanceTo(Start, Start) doesn't cause negative issues. + readResult = await httpContext.Request.BodyReader.ReadAsync(); + httpContext.Request.BodyReader.AdvanceTo(readResult.Buffer.Start, readResult.Buffer.Start); + tcs2.SetResult(null); + + readResult = await httpContext.Request.BodyReader.ReadAsync(); + Assert.Equal(5, readResult.Buffer.Length); + + }, testContext)) + { + using (var connection = server.CreateConnection()) + { + await connection.Send( + "POST / HTTP/1.0", + "Host:", + "Content-Length: 5", + "", + "fun"); + await tcs.Task; + await connection.Send( + "n"); + await tcs2.Task; + await connection.Send( + "y"); + await connection.ReceiveEnd( + "HTTP/1.1 200 OK", + "Connection: close", + $"Date: {testContext.DateHeaderValue}", + "Content-Length: 0", + "", + ""); + } + + await server.StopAsync(); + } + } + + [Fact] public async Task ConnectionClosesWhenFinReceivedBeforeRequestCompletes() { From 6d09e1407a6ac1ca8029fef83e616319bc6b4b87 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Fri, 8 Mar 2019 09:38:00 -0800 Subject: [PATCH 06/10] Set isReading in one more place --- .../Core/src/Internal/Http/Http1ContentLengthMessageBody.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs index 1209cac65af3..d5fd1c1f5c59 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs @@ -120,13 +120,13 @@ public override bool TryRead(out ReadResult readResult) if (_readCompleted) { + _isReading = true; readResult = _readResult; return true; } TryStart(); - if (!_context.Input.TryRead(out _readResult)) { readResult = default; @@ -143,7 +143,7 @@ public override bool TryRead(out ReadResult readResult) } } - // Only set _isReading if we are returing a ReadResult. + // Only set _isReading if we are returing true. _isReading = true; CreateReadResultFromConnectionReadResult(); From e43f889cb4d41431a6cbc29ce3ab55e0634f527a Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Tue, 12 Mar 2019 11:06:51 -0700 Subject: [PATCH 07/10] React to corefx changes --- .../Internal/Http/Http1ContentLengthMessageBody.cs | 11 +++++------ .../test/InMemory.FunctionalTests/RequestTests.cs | 3 +-- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs index d5fd1c1f5c59..875c557911c0 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs @@ -208,12 +208,11 @@ public override void AdvanceTo(SequencePosition consumed, SequencePosition exami _context.Input.AdvanceTo(consumed, examined); var newlyExamined = examinedLength - _totalExaminedInPreviousReadResult; - if (newlyExamined > 0) - { - OnDataRead(newlyExamined); - _totalExaminedInPreviousReadResult += newlyExamined; - _inputLength -= newlyExamined; - } + + // Newly examined can never be negative, pipereader.AdvanceTo will throw. + OnDataRead(newlyExamined); + _totalExaminedInPreviousReadResult += newlyExamined; + _inputLength -= newlyExamined; _totalExaminedInPreviousReadResult -= consumedLength; } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs index 0b41882ecfe7..cc78455fae13 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs @@ -778,9 +778,8 @@ public async Task ContentLengthReadAsyncSingleBytesAtATime() httpContext.Request.BodyReader.AdvanceTo(readResult.Buffer.Start, readResult.Buffer.End); - // Check AdvanceTo(Start, Start) doesn't cause negative issues. readResult = await httpContext.Request.BodyReader.ReadAsync(); - httpContext.Request.BodyReader.AdvanceTo(readResult.Buffer.Start, readResult.Buffer.Start); + httpContext.Request.BodyReader.AdvanceTo(readResult.Buffer.Start, readResult.Buffer.End); tcs2.SetResult(null); readResult = await httpContext.Request.BodyReader.ReadAsync(); From 6eaeb7e0a27d8d271f6608b9fa8051195e505508 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Fri, 15 Mar 2019 15:38:47 -0700 Subject: [PATCH 08/10] Feedback --- .../Http/Http1ContentLengthMessageBody.cs | 27 ++++++++++---- .../Kestrel/Core/test/MessageBodyTests.cs | 3 -- .../InMemory.FunctionalTests/RequestTests.cs | 36 +++++++++++++++++++ 3 files changed, 56 insertions(+), 10 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs index 875c557911c0..2ae81d4e3266 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs @@ -15,11 +15,11 @@ public class Http1ContentLengthMessageBody : Http1MessageBody private readonly long _contentLength; private long _inputLength; private bool _readCompleted; - private bool _finalAdvanceAfterReadingEntireContentLength; private bool _completed; private bool _isReading; private int _userCanceled; private long _totalExaminedInPreviousReadResult; + private bool _finalAdvanceCalled; public Http1ContentLengthMessageBody(bool keepAlive, long contentLength, Http1Connection context) : base(context) @@ -153,6 +153,17 @@ public override bool TryRead(out ReadResult readResult) return true; } + public override Task ConsumeAsync() + { + TryStart(); + + if (!_readResult.Buffer.IsEmpty && _inputLength == 0) + { + _context.Input.AdvanceTo(_readResult.Buffer.End); + } + + return OnConsumeAsync(); + } private void ThrowIfCompleted() { if (_completed) @@ -166,7 +177,10 @@ private void CreateReadResultFromConnectionReadResult() if (_readResult.Buffer.Length >= _inputLength + _totalExaminedInPreviousReadResult) { _readCompleted = true; - _readResult = new ReadResult(_readResult.Buffer.Slice(0, _inputLength + _totalExaminedInPreviousReadResult), _readResult.IsCanceled, _readCompleted); + _readResult = new ReadResult( + _readResult.Buffer.Slice(0, _inputLength + _totalExaminedInPreviousReadResult), + _readResult.IsCanceled && Interlocked.Exchange(ref _userCanceled, 0) == 1, + _readCompleted); } if (_readResult.IsCompleted) @@ -191,12 +205,12 @@ public override void AdvanceTo(SequencePosition consumed, SequencePosition exami if (_readCompleted) { - _readResult = new ReadResult(_readResult.Buffer.Slice(consumed, _readResult.Buffer.End), isCanceled: false, _readCompleted); + _readResult = new ReadResult(_readResult.Buffer.Slice(consumed, _readResult.Buffer.End), Interlocked.Exchange(ref _userCanceled, 0) == 1, _readCompleted); - if (!_finalAdvanceAfterReadingEntireContentLength) + if (_readResult.Buffer.Length == 0 && !_finalAdvanceCalled) { - _context.Input.AdvanceTo(consumed, examined); - _finalAdvanceAfterReadingEntireContentLength = true; + _context.Input.AdvanceTo(consumed); + _finalAdvanceCalled = true; } return; @@ -209,7 +223,6 @@ public override void AdvanceTo(SequencePosition consumed, SequencePosition exami var newlyExamined = examinedLength - _totalExaminedInPreviousReadResult; - // Newly examined can never be negative, pipereader.AdvanceTo will throw. OnDataRead(newlyExamined); _totalExaminedInPreviousReadResult += newlyExamined; _inputLength -= newlyExamined; diff --git a/src/Servers/Kestrel/Core/test/MessageBodyTests.cs b/src/Servers/Kestrel/Core/test/MessageBodyTests.cs index e46adb9534b7..6560bf0f175d 100644 --- a/src/Servers/Kestrel/Core/test/MessageBodyTests.cs +++ b/src/Servers/Kestrel/Core/test/MessageBodyTests.cs @@ -778,10 +778,7 @@ public async Task CopyToAsyncThrowsOnTimeout() // Add some input and read it to start PumpAsync input.Add("a"); - var readResult = await body.ReadAsync(); - Assert.Equal(1, readResult.Buffer.Length); - body.AdvanceTo(readResult.Buffer.End); // Time out on the next read input.Http1Connection.SendTimeoutResponse(); diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs index cc78455fae13..9a39154327fb 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs @@ -814,6 +814,42 @@ await connection.ReceiveEnd( } } + [Fact] + public async Task ContentLengthDoesNotConsumeEntireBufferDoesNotThrow() + { + var testContext = new TestServiceContext(LoggerFactory); + using (var server = new TestServer(async httpContext => + { + var readResult = await httpContext.Request.BodyReader.ReadAsync(); + + httpContext.Request.BodyReader.AdvanceTo(readResult.Buffer.Start, readResult.Buffer.End); + + readResult = await httpContext.Request.BodyReader.ReadAsync(); + httpContext.Request.BodyReader.AdvanceTo(readResult.Buffer.Slice(1).Start, readResult.Buffer.End); + }, testContext)) + { + using (var connection = server.CreateConnection()) + { + await connection.SendAll( + "POST / HTTP/1.0", + "Host:", + "Content-Length: 5", + "", + "funny"); + + await connection.ReceiveEnd( + "HTTP/1.1 200 OK", + "Connection: close", + $"Date: {testContext.DateHeaderValue}", + "Content-Length: 0", + "", + ""); + } + + await server.StopAsync(); + } + } + [Fact] public async Task ConnectionClosesWhenFinReceivedBeforeRequestCompletes() From 14af05dc90513e691446faa5c5205e155fef0861 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Fri, 15 Mar 2019 15:39:52 -0700 Subject: [PATCH 09/10] Feedback: --- .../Kestrel/test/InMemory.FunctionalTests/RequestTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs index 9a39154327fb..ff83e5a20dc2 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs @@ -850,7 +850,6 @@ await connection.ReceiveEnd( } } - [Fact] public async Task ConnectionClosesWhenFinReceivedBeforeRequestCompletes() { From 6c9b43edfaba66090cf17feef1dab691b0ebb01b Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Mon, 18 Mar 2019 13:47:44 -0700 Subject: [PATCH 10/10] fb --- .../Http/Http1ContentLengthMessageBody.cs | 1 + .../InMemory.FunctionalTests/RequestTests.cs | 41 +++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs index 2ae81d4e3266..c867a7663ad9 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs @@ -164,6 +164,7 @@ public override Task ConsumeAsync() return OnConsumeAsync(); } + private void ThrowIfCompleted() { if (_completed) diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs index ff83e5a20dc2..bca9b9d1b139 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs @@ -764,6 +764,47 @@ await connection.ReceiveEnd( } } + [Fact] + public async Task ContentLengthReadAsyncPipeReaderBufferRequestBodyMultipleTimes() + { + var testContext = new TestServiceContext(LoggerFactory); + + using (var server = new TestServer(async httpContext => + { + var readResult = await httpContext.Request.BodyReader.ReadAsync(); + // This will hang if 0 content length is not assumed by the server + Assert.Equal(5, readResult.Buffer.Length); + httpContext.Request.BodyReader.AdvanceTo(readResult.Buffer.Start, readResult.Buffer.End); + + for (var i = 0; i < 2; i++) + { + readResult = await httpContext.Request.BodyReader.ReadAsync(); + Assert.Equal(5, readResult.Buffer.Length); + httpContext.Request.BodyReader.AdvanceTo(readResult.Buffer.Start, readResult.Buffer.End); + } + }, testContext)) + { + using (var connection = server.CreateConnection()) + { + await connection.SendAll( + "POST / HTTP/1.0", + "Host:", + "Content-Length: 5", + "", + "hello"); + await connection.ReceiveEnd( + "HTTP/1.1 200 OK", + "Connection: close", + $"Date: {testContext.DateHeaderValue}", + "Content-Length: 0", + "", + ""); + } + + await server.StopAsync(); + } + } + [Fact] public async Task ContentLengthReadAsyncSingleBytesAtATime() {