review comments

Author: ladeak

src/Servers/Kestrel/Core/src/Internal/Http2/FlowControl/InputFlowControl.cs

@@ -24,16 +24,16 @@ private struct FlowControlState
         private const long AbortedBitMask = 1L << 32; // uint MaxValue + 1
         internal long _state;
 
-        public FlowControlState(uint initialWindowSize, bool isAborted)
+        public FlowControlState(int size, bool isAborted)
         {
-            _state = initialWindowSize;
+            _state = (uint)size; // Casted first to uint before assigning it to a long field to the address negative values
             if (isAborted)
             {
                 _state |= AbortedBitMask;
             }
         }
 
-        public uint Available => (uint)_state;
+        public int Available => (int)_state;
 
         public bool IsAborted => _state > uint.MaxValue;
     }
@@ -49,19 +49,20 @@ public InputFlowControl(uint initialWindowSize, uint minWindowSizeIncrement)
     {
         Debug.Assert(initialWindowSize >= minWindowSizeIncrement, "minWindowSizeIncrement is greater than the window size.");
 
-        _flow = new FlowControlState(initialWindowSize, false);
+        _flow = new FlowControlState((int)initialWindowSize, isAborted: false);
         _initialWindowSize = initialWindowSize;
         _minWindowSizeIncrement = (int)minWindowSizeIncrement;
     }
 
     public bool IsAvailabilityLow => _flow.Available < _minWindowSizeIncrement;
 
-    // Test hook, not participating in mutual exclusion
-    internal uint Available => _flow.Available;
+    // Test hook
+    internal int Available => _flow.Available;
+    internal bool IsAborted => _flow.IsAborted;
 
     public void Reset()
     {
-        _flow = new FlowControlState(_initialWindowSize, false);
+        _flow = new FlowControlState((int)_initialWindowSize, isAborted: false);
         _pendingUpdateSize = 0;
         _windowUpdatesDisabled = false;
     }
@@ -72,6 +73,7 @@ public bool TryAdvance(int bytes)
         do
         {
             currentFlow = _flow; // Copy
+
             // 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 > currentFlow.Available)
@@ -84,7 +86,7 @@ public bool TryAdvance(int bytes)
                 return false;
             }
 
-            computedFlow = new FlowControlState(currentFlow.Available - (uint)bytes, isAborted: false);
+            computedFlow = new FlowControlState(currentFlow.Available - bytes, isAborted: false);
         } while (currentFlow._state != Interlocked.CompareExchange(ref _flow._state, computedFlow._state, currentFlow._state));
 
         return true;
@@ -103,14 +105,12 @@ public bool TryUpdateWindow(int bytes, out int updateSize)
                 return false;
             }
 
-            var maxUpdate = int.MaxValue - currentFlow.Available;
-            if (bytes > maxUpdate)
-            {
-                // 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.");
-            }
-            computedFlow = new FlowControlState(currentFlow.Available + (uint)bytes, isAborted: false);
+            var maxUpdate = Http2PeerSettings.MaxWindowSize - currentFlow.Available;
+
+            // 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(bytes <= maxUpdate, $"{nameof(TryUpdateWindow)} attempted to grow window past max size.");
+            computedFlow = new FlowControlState(currentFlow.Available + bytes, isAborted: false);
         } while (currentFlow._state != Interlocked.CompareExchange(ref _flow._state, computedFlow._state, currentFlow._state));
 
         if (_windowUpdatesDisabled)

src/Servers/Kestrel/Core/test/Http2/FlowControl/InputFlowControlTests.cs

@@ -10,6 +10,43 @@ public class InputFlowControlTests
 {
     private const int IterationCount = 1;
 
+    [Theory]
+    [InlineData(-100, -100)]
+    [InlineData(100, 100)]
+    [InlineData(int.MaxValue, int.MaxValue)]
+    [InlineData(int.MinValue, int.MinValue)]
+    public void ValuesCanBeUpdatedToBoundaries(int update, int expected)
+    {
+        var sut = new InputFlowControl(0, 0);
+        sut.TryUpdateWindow(update, out _);
+        Assert.Equal(expected, sut.Available);
+    }
+
+    [Fact]
+    public void Available_CanBecomeNegative()
+    {
+        var sut = new InputFlowControl(10, 1);
+        sut.TryUpdateWindow(-100, out _);
+        Assert.Equal(-90, sut.Available);
+        sut.TryUpdateWindow(100, out _);
+        Assert.Equal(10, sut.Available);
+    }
+
+    [Theory]
+    [InlineData(0)]
+    [InlineData(1)]
+    [InlineData(-1)]
+    [InlineData(int.MaxValue)]
+    [InlineData(int.MinValue)]
+    public void IsAborted(int value)
+    {
+        var sut = new InputFlowControl(uint.MaxValue, 1);
+        sut.TryUpdateWindow(value, out _);
+        Assert.False(sut.IsAborted);
+        sut.Abort();
+        Assert.True(sut.IsAborted);
+    }
+
     [Fact]
     public async Task Threads1_Advance()
     {
@@ -82,7 +119,7 @@ public async Task Threads3_WindowUpdates()
             var t2 = Task.Run(() => WindowUpdate(sut));
             var t3 = Task.Run(() => WindowUpdate(sut));
             await Task.WhenAll(t1, t2, t3);
-            Assert.Equal((uint)15000 + 5000 * 3, sut.Available);
+            Assert.Equal(15000 + 5000 * 3, sut.Available);
         }
 
         static void WindowUpdate(InputFlowControl sut)
@@ -331,7 +368,7 @@ public async Task Threads3Advance_WindowUpdates()
             var t2 = Task.Run(() => Advance(sut));
             var t3 = Task.Run(() => Advance(sut));
             await Task.WhenAll(t0, t1, t2, t3);
-            Assert.Equal((uint)10000, sut.Available);
+            Assert.Equal(10000, sut.Available);
         }
 
         static void WindowUpdate(InputFlowControl sut)
@@ -364,7 +401,7 @@ public async Task Threads3WindowUpdates_AssertSize()
             var t2 = Task.Run(() => WindowUpdate(ref sizeSum, sut));
             var t3 = Task.Run(() => WindowUpdate(ref sizeSum, sut));
             await Task.WhenAll(t1, t2, t3);
-            Assert.Equal((uint)15000 + 5000 * 3, sut.Available);
+            Assert.Equal(15000 + 5000 * 3, sut.Available);
             sut.TryUpdateWindow(11, out var size); // To trigger anything less then 10 included
             sizeSum += size;
             Assert.Equal(5000 * 3 + 11, sizeSum);