Majority of feedback.

Author: jkotalik

src/Servers/Kestrel/Core/src/Internal/Http/Http1ChunkedEncodingMessageBody.cs

@@ -74,7 +74,9 @@ public override async ValueTask<ReadResult> ReadAsync(CancellationToken cancella
 
             try
             {
-                _readResult = await StartTimingReadAsync(cancellationToken);
+                var readAwaitable = _requestBodyPipe.Reader.ReadAsync(cancellationToken);
+
+                _readResult = await StartTimingReadAsync(readAwaitable, cancellationToken);
             }
             catch (ConnectionAbortedException ex)
             {
@@ -156,17 +158,7 @@ private async Task PumpAsync()
                         }
 
                         // Read() will have already have greedily consumed the entire request body if able.
-                        if (result.IsCompleted)
-                        {
-                            // OnInputOrOutputCompleted() is an idempotent method that closes the connection. Sometimes
-                            // input completion is observed here before the Input.OnWriterCompleted() callback is fired,
-                            // so we call OnInputOrOutputCompleted() now to prevent a race in our tests where a 400
-                            // response is written after observing the unexpected end of request content instead of just
-                            // closing the connection without a response as expected.
-                            _context.OnInputOrOutputCompleted();
-
-                            BadHttpRequestException.Throw(RequestRejectionReason.UnexpectedEndOfRequestContent);
-                        }
+                        CheckCompletedReadResult(result);
                     }
                     finally
                     {
@@ -218,77 +210,6 @@ private async Task StopAsyncAwaited()
             _requestBodyPipe.Reset();
         }
 
-        protected override Task OnConsumeAsync()
-        {
-            try
-            {
-                if (_requestBodyPipe.Reader.TryRead(out var readResult))
-                {
-                    _requestBodyPipe.Reader.AdvanceTo(readResult.Buffer.End);
-
-                    if (readResult.IsCompleted)
-                    {
-                        return Task.CompletedTask;
-                    }
-                }
-            }
-            catch (BadHttpRequestException ex)
-            {
-                // At this point, the response has already been written, so this won't result in a 4XX response;
-                // however, we still need to stop the request processing loop and log.
-                _context.SetBadRequestState(ex);
-                return Task.CompletedTask;
-            }
-            catch (InvalidOperationException ex)
-            {
-                var connectionAbortedException = new ConnectionAbortedException(CoreStrings.ConnectionAbortedByApplication, ex);
-                _context.ReportApplicationError(connectionAbortedException);
-
-                // Have to abort the connection because we can't finish draining the request
-                _context.StopProcessingNextRequest();
-                return Task.CompletedTask;
-            }
-
-            return OnConsumeAsyncAwaited();
-        }
-
-        private async Task OnConsumeAsyncAwaited()
-        {
-            Log.RequestBodyNotEntirelyRead(_context.ConnectionIdFeature, _context.TraceIdentifier);
-
-            _context.TimeoutControl.SetTimeout(Constants.RequestBodyDrainTimeout.Ticks, TimeoutReason.RequestBodyDrain);
-
-            try
-            {
-                ReadResult result;
-                do
-                {
-                    result = await _requestBodyPipe.Reader.ReadAsync();
-                    _requestBodyPipe.Reader.AdvanceTo(result.Buffer.End);
-                } while (!result.IsCompleted);
-            }
-            catch (BadHttpRequestException ex)
-            {
-                _context.SetBadRequestState(ex);
-            }
-            catch (ConnectionAbortedException)
-            {
-                Log.RequestBodyDrainTimedOut(_context.ConnectionIdFeature, _context.TraceIdentifier);
-            }
-            catch (InvalidOperationException ex)
-            {
-                var connectionAbortedException = new ConnectionAbortedException(CoreStrings.ConnectionAbortedByApplication, ex);
-                _context.ReportApplicationError(connectionAbortedException);
-
-                // Have to abort the connection because we can't finish draining the request
-                _context.StopProcessingNextRequest();
-            }
-            finally
-            {
-                _context.TimeoutControl.CancelTimeout();
-            }
-        }
-
         protected void Copy(ReadOnlySequence<byte> readableBuffer, PipeWriter writableBuffer)
         {
             if (readableBuffer.IsSingleSegment)
@@ -595,34 +516,6 @@ private int CalculateChunkSize(int extraHexDigit, int currentParsedSize)
             return -1; // can't happen, but compiler complains
         }
 
-        private ValueTask<ReadResult> StartTimingReadAsync(CancellationToken cancellationToken)
-        {
-            // The only difference is which reader to use. Let's do the following.
-            // Make an internal reader that will always be used for whatever operation is needed here
-            // Keep external one the same always.
-            var readAwaitable = _requestBodyPipe.Reader.ReadAsync(cancellationToken);
-
-            if (!readAwaitable.IsCompleted && _timingEnabled)
-            {
-                _backpressure = true;
-                _context.TimeoutControl.StartTimingRead();
-            }
-
-            return readAwaitable;
-        }
-
-        private void StopTimingRead(long bytesRead)
-        {
-            _context.TimeoutControl.BytesRead(bytesRead - _alreadyTimedBytes);
-            _alreadyTimedBytes = 0;
-
-            if (_backpressure)
-            {
-                _backpressure = false;
-                _context.TimeoutControl.StopTimingRead();
-            }
-        }
-
         private enum Mode
         {
             Prefix,

src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs

@@ -45,7 +45,7 @@ public override async ValueTask<ReadResult> ReadAsync(CancellationToken cancella
             // We internally track an int for that.
             while (true)
             {
-                // This isn't great. The issue is that TryRead can get a canceled read result
+                // _context.RequestTimedOut The issue is that TryRead can get a canceled read result
                 // which is unknown to StartTimingReadAsync. 
                 if (_context.RequestTimedOut)
                 {
@@ -54,7 +54,8 @@ public override async ValueTask<ReadResult> ReadAsync(CancellationToken cancella
 
                 try
                 {
-                    _readResult = await StartTimingReadAsync(cancellationToken);
+                    var readAwaitable = _context.Input.ReadAsync(cancellationToken);
+                    _readResult = await StartTimingReadAsync(readAwaitable, cancellationToken);
                 }
                 catch (ConnectionAbortedException ex)
                 {
@@ -63,43 +64,39 @@ public override async ValueTask<ReadResult> ReadAsync(CancellationToken cancella
 
                 if (_context.RequestTimedOut)
                 {
-                    Debug.Assert(_readResult.IsCanceled);
                     BadHttpRequestException.Throw(RequestRejectionReason.RequestBodyTimeout);
                 }
 
+                // Make sure to handle when this is canceled here.
                 if (_readResult.IsCanceled)
                 {
-                    if (Interlocked.CompareExchange(ref _userCanceled, 0, 1) == 1)
+                    if (Interlocked.Exchange(ref _userCanceled, 0) == 1)
                     {
                         // Ignore the readResult if it wasn't by the user.
                         break;
                     }
+                    else
+                    {
+                        // TODO should this reset the timing read?
+                        StopTimingRead(0);
+                        continue;
+                    }
                 }
 
                 var readableBuffer = _readResult.Buffer;
                 var readableBufferLength = readableBuffer.Length;
                 StopTimingRead(readableBufferLength);
 
-                if (_readResult.IsCompleted)
-                {
-                    // OnInputOrOutputCompleted() is an idempotent method that closes the connection. Sometimes
-                    // input completion is observed here before the Input.OnWriterCompleted() callback is fired,
-                    // so we call OnInputOrOutputCompleted() now to prevent a race in our tests where a 400
-                    // response is written after observing the unexpected end of request content instead of just
-                    // closing the connection without a response as expected.
-                    _context.OnInputOrOutputCompleted();
-
-                    BadHttpRequestException.Throw(RequestRejectionReason.UnexpectedEndOfRequestContent);
-                }
+                CheckCompletedReadResult(_readResult);
 
                 if (readableBufferLength > 0)
                 {
+                    CreateReadResultFromConnectionReadResult();
+
                     break;
                 }
             }
 
-            CreateReadResultFromConnectionReadResult();
-
             return _readResult;
         }
 
@@ -115,13 +112,27 @@ public override bool TryRead(out ReadResult readResult)
 
             TryStart();
 
-            var boolResult = _context.Input.TryRead(out _readResult);
+            if (!_context.Input.TryRead(out _readResult))
+            {
+                readResult = default;
+                return false;
+            }
+
+            if (_readResult.IsCanceled)
+            {
+                if (Interlocked.Exchange(ref _userCanceled, 0) == 0)
+                {
+                    // Cancellation wasn't by the user, return default ReadResult
+                    readResult = default;
+                    return false;
+                }
+            }
 
             CreateReadResultFromConnectionReadResult();
 
             readResult = _readResult;
 
-            return boolResult;
+            return true;
         }
 
         private void ThrowIfCompleted()
@@ -178,33 +189,6 @@ protected override void OnReadStarting()
             }
         }
 
-        private ValueTask<ReadResult> StartTimingReadAsync(CancellationToken cancellationToken)
-        {
-            var readAwaitable = _context.Input.ReadAsync(cancellationToken);
-
-            if (!readAwaitable.IsCompleted && _timingEnabled)
-            {
-                TryProduceContinue();
-
-                _backpressure = true;
-                _context.TimeoutControl.StartTimingRead();
-            }
-
-            return readAwaitable;
-        }
-
-        private void StopTimingRead(long bytesRead)
-        {
-            _context.TimeoutControl.BytesRead(bytesRead - _alreadyTimedBytes);
-            _alreadyTimedBytes = 0;
-
-            if (_backpressure)
-            {
-                _backpressure = false;
-                _context.TimeoutControl.StopTimingRead();
-            }
-        }
-
         public override void Complete(Exception exception)
         {
             _context.ReportApplicationError(exception);
@@ -227,76 +211,5 @@ protected override Task OnStopAsync()
             Complete(null);
             return Task.CompletedTask;
         }
-
-        protected override Task OnConsumeAsync()
-        {
-            try
-            {
-                if (TryRead(out var readResult))
-                {
-                    AdvanceTo(readResult.Buffer.End);
-
-                    if (readResult.IsCompleted)
-                    {
-                        return Task.CompletedTask;
-                    }
-                }
-            }
-            catch (BadHttpRequestException ex)
-            {
-                // At this point, the response has already been written, so this won't result in a 4XX response;
-                // however, we still need to stop the request processing loop and log.
-                _context.SetBadRequestState(ex);
-                return Task.CompletedTask;
-            }
-            catch (InvalidOperationException ex)
-            {
-                var connectionAbortedException = new ConnectionAbortedException(CoreStrings.ConnectionAbortedByApplication, ex);
-                _context.ReportApplicationError(connectionAbortedException);
-
-                // Have to abort the connection because we can't finish draining the request
-                _context.StopProcessingNextRequest();
-                return Task.CompletedTask;
-            }
-
-            return OnConsumeAsyncAwaited();
-        }
-
-        private async Task OnConsumeAsyncAwaited()
-        {
-            Log.RequestBodyNotEntirelyRead(_context.ConnectionIdFeature, _context.TraceIdentifier);
-
-            _context.TimeoutControl.SetTimeout(Constants.RequestBodyDrainTimeout.Ticks, TimeoutReason.RequestBodyDrain);
-
-            try
-            {
-                ReadResult result;
-                do
-                {
-                    result = await ReadAsync();
-                    AdvanceTo(result.Buffer.End);
-                } while (!result.IsCompleted);
-            }
-            catch (BadHttpRequestException ex)
-            {
-                _context.SetBadRequestState(ex);
-            }
-            catch (ConnectionAbortedException)
-            {
-                Log.RequestBodyDrainTimedOut(_context.ConnectionIdFeature, _context.TraceIdentifier);
-            }
-            catch (InvalidOperationException ex)
-            {
-                var connectionAbortedException = new ConnectionAbortedException(CoreStrings.ConnectionAbortedByApplication, ex);
-                _context.ReportApplicationError(connectionAbortedException);
-
-                // Have to abort the connection because we can't finish draining the request
-                _context.StopProcessingNextRequest();
-            }
-            finally
-            {
-                _context.TimeoutControl.CancelTimeout();
-            }
-        }
     }
 }