Improve reliability of RequestTimesOutWhenRequestBodyNotReceivedAtSpecifiedMinimumRate

test/Kestrel.FunctionalTests/RequestBodyTimeoutTests.cs

@@ -7,7 +7,6 @@
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Server.Kestrel.Core;
 using Microsoft.AspNetCore.Server.Kestrel.Core.Features;
-using Microsoft.AspNetCore.Server.Kestrel.Core.Internal;
 using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http;
 using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure;
 using Microsoft.AspNetCore.Testing;
@@ -37,8 +36,32 @@ public async Task RequestTimesOutWhenRequestBodyNotReceivedAtSpecifiedMinimumRat
                 context.Features.Get<IHttpMinRequestBodyDataRateFeature>().MinDataRate =
                     new MinDataRate(bytesPerSecond: 1, gracePeriod: gracePeriod);
 
+                // The server must call Request.Body.ReadAsync() *before* the test sets systemClock.UtcNow (which is triggered by the
+                // server calling appRunningEvent.Set()).  If systemClock.UtcNow is set first, it's possible for the test to fail
+                // due to the following race condition:
+                //
+                // 1. [test]    systemClock.UtcNow += gracePeriod + TimeSpan.FromSeconds(1);
+                // 2. [server]  Heartbeat._timer is triggered, which calls HttpConnection.Tick()
+                // 3. [server]  HttpConnection.Tick() calls HttpConnection.CheckForReadDataRateTimeout()
+                // 4. [server]  HttpConnection.CheckForReadDataRateTimeout() is a no-op, since _readTimingEnabled is false,
+                //              since Request.Body.ReadAsync() has not been called yet
+                // 5. [server]  HttpConnection.Tick() sets _lastTimestamp = timestamp
+                // 6. [server]  Request.Body.ReadAsync() is called
+                // 6. [test]    systemClock.UtcNow is never updated again, so server timestamp is never updated,
+                //              so HttpConnection.CheckForReadDataRateTimeout() is always a no-op until test fails
+                //
+                // This is a pretty tight race, since the once-per-second Heartbeat._timer needs to fire between the test updating
+                // systemClock.UtcNow and the server calling Request.Body.ReadAsync().  But it happened often enough to cause
+                // test flakiness in our CI (https://github.com/aspnet/KestrelHttpServer/issues/2539).
+                //
+                // For verification, I was able to induce the race by adding a sleep in the RequestDelegate:
+                //     appRunningEvent.Set();
+                //     Thread.Sleep(5000);
+                //     return context.Request.Body.ReadAsync(new byte[1], 0, 1);
+
+                var readTask = context.Request.Body.ReadAsync(new byte[1], 0, 1);
                 appRunningEvent.Set();
-                return context.Request.Body.ReadAsync(new byte[1], 0, 1);
+                return readTask;
             }, serviceContext))
             {
                 using (var connection = server.CreateConnection())