Don't recompute ReadOnlySequence length

dotnet · Nov 1, 2019 · 1c5707e · 1c5707e
1 parent b6de557
commit 1c5707e
Showing 1 changed file with 26 additions and 17 deletions.
diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs
@@ -43,7 +43,7 @@ public override async ValueTask<ReadResult> ReadAsyncInternal(CancellationToken
             if (_readCompleted)
             {
                 _isReading = true;
-                return _readResult;
+                return new ReadResult(_readResult.Buffer, Interlocked.Exchange(ref _userCanceled, 0) == 1, _readResult.IsCompleted);
             }
 
             TryStart();
@@ -95,11 +95,12 @@ void ResetReadingState()
                 }
 
                 // Ignore the canceled readResult if it wasn't canceled by the user.
-                if ((!_readResult.IsCanceled && _readResult.Buffer.Length > 0) ||
-                    (_readResult.IsCanceled && Interlocked.Exchange(ref _userCanceled, 0) == 1))
+                if (!_readResult.IsCanceled || Interlocked.Exchange(ref _userCanceled, 0) == 1)
                 {
-                    CreateReadResultFromConnectionReadResult();
-                    StopTimingRead(_readResult.Buffer.Length);
+                    var returnedReadResultLength = CreateReadResultFromConnectionReadResult();
+
+                    // Don't count bytes belonging to the next request, since read rate timeouts are done on a per-request basis.
+                    StopTimingRead(returnedReadResultLength);
 
                     if (_readResult.IsCompleted)
                     {
@@ -131,14 +132,15 @@ public override bool TryReadInternal(out ReadResult readResult)
             if (_readCompleted)
             {
                 _isReading = true;
-                readResult = _readResult;
+                readResult = new ReadResult(_readResult.Buffer, Interlocked.Exchange(ref _userCanceled, 0) == 1, _readResult.IsCompleted);
                 return true;
             }
 
             TryStart();
 
             // The while(true) loop is required because the Http1 connection calls CancelPendingRead to unblock
-            // the call to StartTimingReadAsync to check if the request timed out.
+            // the call to StartTimingReadAsync to check if the request timed out, and we don't want to return
+            // a canceled ReadResult of the user themselves didn't cancel it.
             while (true)
             {
                 if (!_context.Input.TryRead(out _readResult))
@@ -161,13 +163,14 @@ public override bool TryReadInternal(out ReadResult readResult)
                 ThrowUnexpectedEndOfRequestContent();
             }
 
-            // Only set _isReading if we are returning true.
-            _isReading = true;
+            var returnedReadResultLength = CreateReadResultFromConnectionReadResult();
 
-            CreateReadResultFromConnectionReadResult();
+            // Don't count bytes belonging to the next request, since read rate timeouts are done on a per-request basis.
+            CountBytesRead(returnedReadResultLength);
 
+            // Only set _isReading if we are returning true.
+            _isReading = true;
             readResult = _readResult;
-            CountBytesRead(readResult.Buffer.Length);
 
             if (readResult.IsCompleted)
             {
@@ -177,18 +180,23 @@ public override bool TryReadInternal(out ReadResult readResult)
             return true;
         }
 
-        private void CreateReadResultFromConnectionReadResult()
+        private long CreateReadResultFromConnectionReadResult()
         {
-            if (_readResult.Buffer.Length < _inputLength + _examinedUnconsumedBytes)
+            var initialLength = _readResult.Buffer.Length;
+            var maxLength = _inputLength + _examinedUnconsumedBytes;
+
+            if (initialLength < maxLength)
             {
-                return;
+                return initialLength;
             }
 
             _readCompleted = true;
             _readResult = new ReadResult(
-                _readResult.Buffer.Slice(0, _inputLength + _examinedUnconsumedBytes),
+                _readResult.Buffer.Slice(0, maxLength),
                 _readResult.IsCanceled,
                 isCompleted: true);
+
+            return maxLength;
         }
 
         public override void AdvanceTo(SequencePosition consumed)
@@ -207,9 +215,10 @@ public override void AdvanceTo(SequencePosition consumed, SequencePosition exami
 
             if (_readCompleted)
             {
-                _readResult = new ReadResult(_readResult.Buffer.Slice(consumed, _readResult.Buffer.End), Interlocked.Exchange(ref _userCanceled, 0) == 1, _readCompleted);
+                // If the old stored _readResult was canceled, it's already been observed. Do not store a canceled read result permanently.
+                _readResult = new ReadResult(_readResult.Buffer.Slice(consumed, _readResult.Buffer.End), isCanceled: false, _readCompleted);
 
-                if (_readResult.Buffer.Length == 0 && !_finalAdvanceCalled)
+                if (!_finalAdvanceCalled & _readResult.Buffer.Length == 0)
                 {
                     _context.Input.AdvanceTo(consumed);
                     _finalAdvanceCalled = true;