Port HttpClient Factory test fixes

Porting some improvements to flaky tests that were made in master


Commit migrated from dotnet/extensions@4985850

src/libraries/Microsoft.Extensions.Http/tests/DefaultHttpClientFactoryTest.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.Net.Http;
+using System.Runtime.CompilerServices;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.Extensions.DependencyInjection;
@@ -328,6 +329,40 @@ public async Task Factory_CleanupCycle_DisposesEligibleHandler()
                 EnableCleanupTimer = true,
             };
 
+            var cleanupEntry = await SimulateClientUse_Factory_CleanupCycle_DisposesEligibleHandler(factory);
+
+            // Being pretty conservative here because we want this test to be reliable,
+            // and it depends on the GC and timing.
+            for (var i = 0; i < 3; i++)
+            {
+                GC.Collect();
+                GC.WaitForPendingFinalizers();
+                GC.Collect();
+
+                if (cleanupEntry.CanDispose)
+                {
+                    break;
+                }
+
+                await Task.Delay(TimeSpan.FromSeconds(1));
+            }
+
+            Assert.True(cleanupEntry.CanDispose, "Cleanup entry disposable");
+
+            // Act
+            factory.CleanupTimer_Tick(state: null);
+
+            // Assert
+            Assert.Empty(factory._expiredHandlers);
+            Assert.Equal(1, disposeHandler.DisposeCount);
+            Assert.False(factory.CleanupTimerStarted.IsSet, "Cleanup timer not started");
+        }
+
+        // Seprate to avoid the HttpClient getting its lifetime extended by
+        // the state machine of the test.
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private async Task<ExpiredHandlerTrackingEntry> SimulateClientUse_Factory_CleanupCycle_DisposesEligibleHandler(TestHttpClientFactory factory)
+        {
             // Create a handler and move it to the expired state
             var client1 = factory.CreateClient("github");
 
@@ -344,19 +379,8 @@ public async Task Factory_CleanupCycle_DisposesEligibleHandler()
             // the factory isn't keeping any references.
             kvp = default;
             client1 = null;
-            GC.Collect();
-            GC.WaitForPendingFinalizers();
-            GC.Collect();
-
-            Assert.True(cleanupEntry.CanDispose, "Cleanup entry disposable");
-
-            // Act
-            factory.CleanupTimer_Tick(null);
 
-            // Assert
-            Assert.Empty(factory._expiredHandlers);
-            Assert.Equal(1, disposeHandler.DisposeCount);
-            Assert.False(factory.CleanupTimerStarted.IsSet, "Cleanup timer not started");
+            return cleanupEntry;
         }
 
         [Fact]
@@ -375,6 +399,42 @@ public async Task Factory_CleanupCycle_DisposesLiveHandler()
                 EnableCleanupTimer = true,
             };
 
+            var cleanupEntry = await SimulateClientUse_Factory_CleanupCycle_DisposesLiveHandler(factory, disposeHandler);
+
+            // Being pretty conservative here because we want this test to be reliable,
+            // and it depends on the GC and timing.
+            for (var i = 0; i < 3; i++)
+            {
+                GC.Collect();
+                GC.WaitForPendingFinalizers();
+                GC.Collect();
+
+                if (cleanupEntry.CanDispose)
+                {
+                    break;
+                }
+
+                await Task.Delay(TimeSpan.FromSeconds(1));
+            }
+
+            Assert.True(cleanupEntry.CanDispose, "Cleanup entry disposable");
+
+            // Act - 2
+            factory.CleanupTimer_Tick(state: null);
+
+            // Assert
+            Assert.Empty(factory._expiredHandlers);
+            Assert.Equal(1, disposeHandler.DisposeCount);
+            Assert.False(factory.CleanupTimerStarted.IsSet, "Cleanup timer not started");
+        }
+
+        // Seprate to avoid the HttpClient getting its lifetime extended by
+        // the state machine of the test.
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private async Task<ExpiredHandlerTrackingEntry> SimulateClientUse_Factory_CleanupCycle_DisposesLiveHandler(
+            TestHttpClientFactory factory,
+            DisposeTrackingHandler disposeHandler)
+        {
             // Create a handler and move it to the expired state
             var client1 = factory.CreateClient("github");
 
@@ -386,12 +446,16 @@ public async Task Factory_CleanupCycle_DisposesLiveHandler()
             var cleanupEntry = Assert.Single(factory._expiredHandlers);
             Assert.True(factory.CleanupTimerStarted.IsSet, "Cleanup timer started");
 
-            // Nulling out the refernces to the internal state of the factory since they wouldn't exist in the non-test
+            // Nulling out the references to the internal state of the factory since they wouldn't exist in the non-test
             // scenario. We're holding on the client to prevent disposal - like a real use case.
-            kvp = default;
+            lock (this)
+            {
+                // Prevent reordering
+                kvp = default;
+            }
 
             // Act - 1
-            factory.CleanupTimer_Tick(null);
+            factory.CleanupTimer_Tick(state: null);
 
             // Assert
             Assert.Same(cleanupEntry, Assert.Single(factory._expiredHandlers));
@@ -401,20 +465,13 @@ public async Task Factory_CleanupCycle_DisposesLiveHandler()
             // We need to make sure that the outer handler actually gets GCed, so drop our references to it.
             // This is important because the factory relies on this possibility for correctness. We need to ensure that 
             // the factory isn't keeping any references.
-            client1 = null;
-            GC.Collect();
-            GC.WaitForPendingFinalizers();
-            GC.Collect();
-
-            Assert.True(cleanupEntry.CanDispose, "Cleanup entry disposable");
-
-            // Act - 2
-            factory.CleanupTimer_Tick(null);
+            lock (this)
+            {
+                // Prevent reordering
+                client1 = null;
+            }
 
-            // Assert
-            Assert.Empty(factory._expiredHandlers);
-            Assert.Equal(1, disposeHandler.DisposeCount);
-            Assert.False(factory.CleanupTimerStarted.IsSet, "Cleanup timer not started");
+            return cleanupEntry;
         }
 
         private class TestHttpClientFactory : DefaultHttpClientFactory