Make HttpClientFactory implement IDisposable to clean up resources

src/libraries/Microsoft.Extensions.Http/src/ActiveHandlerTrackingEntry.cs

@@ -11,13 +11,17 @@ namespace Microsoft.Extensions.Http
 {
     // Thread-safety: We treat this class as immutable except for the timer. Creating a new object
     // for the 'expiry' pool simplifies the threading requirements significantly.
-    internal sealed class ActiveHandlerTrackingEntry
+    internal sealed class ActiveHandlerTrackingEntry : IDisposable
     {
         private static readonly TimerCallback _timerCallback = (s) => ((ActiveHandlerTrackingEntry)s!).Timer_Tick();
         private readonly object _lock;
         private bool _timerInitialized;
         private Timer? _timer;
         private TimerCallback? _callback;
+        // States for the handler tracking entry
+        private const int Disposed = 1;
+        private const int Expired = 2;
+        private int _disposed;
 
         public ActiveHandlerTrackingEntry(
             string name,
@@ -43,6 +47,11 @@ public ActiveHandlerTrackingEntry(
 
         public void StartExpiryTimer(TimerCallback callback)
         {
+            if (_disposed > 0)
+            {
+                return;
+            }
+
             if (Lifetime == Timeout.InfiniteTimeSpan)
             {
                 return; // never expires.
@@ -62,7 +71,8 @@ private void StartExpiryTimerSlow(TimerCallback callback)
 
             lock (_lock)
             {
-                if (Volatile.Read(ref _timerInitialized))
+                if (Volatile.Read(ref _timerInitialized) ||
+                    _disposed > 0)
                 {
                     return;
                 }
@@ -76,7 +86,7 @@ private void StartExpiryTimerSlow(TimerCallback callback)
         private void Timer_Tick()
         {
             Debug.Assert(_callback != null);
-            Debug.Assert(_timer != null);
+            Debug.Assert(_timer != null || _disposed > 0);
 
             lock (_lock)
             {
@@ -85,9 +95,44 @@ private void Timer_Tick()
                     _timer.Dispose();
                     _timer = null;
 
-                    _callback(this);
+                    // Only invoke the callback if we successfully transition from 0 to Expired
+                    // This ensures we don't convert to expired if already disposed
+                    if (Interlocked.CompareExchange(ref _disposed, Expired, 0) == 0)
+                    {
+                        _callback(this);
+                    }
                 }
             }
         }
+
+        public void StopTimer()
+        {
+            lock (_lock)
+            {
+                _timer?.Dispose();
+                _timer = null;
+            }
+        }
+
+        public void Dispose()
+        {
+            // Try to transition from 0 to Disposed state
+            // If already in another state (Expired), do nothing further with handlers
+            if (Interlocked.CompareExchange(ref _disposed, Disposed, 0) != 0)
+            {
+                // If the entry was already disposed or expired, exit
+                // If it was expired, the timer has already stopped and
+                // ExpiredHandlerTrackingEntry now owns both handler and scope
+                return;
+            }
+
+            StopTimer();
+
+            // When we're directly disposed (not converted to an expired entry),
+            // we need to dispose the inner handler (not the LifetimeTrackingHttpMessageHandler itself)
+            // and the scope
+            Handler.InnerHandler?.Dispose();
+            Scope?.Dispose();
+        }
     }
 }

src/libraries/Microsoft.Extensions.Http/src/DefaultHttpClientFactory.cs

@@ -16,7 +16,7 @@
 
 namespace Microsoft.Extensions.Http
 {
-    internal class DefaultHttpClientFactory : IHttpClientFactory, IHttpMessageHandlerFactory
+    internal class DefaultHttpClientFactory : IHttpClientFactory, IHttpMessageHandlerFactory, IDisposable
     {
         private static readonly TimerCallback _cleanupCallback = (s) => ((DefaultHttpClientFactory)s!).CleanupTimer_Tick();
         private readonly IServiceProvider _services;
@@ -33,14 +33,15 @@ internal class DefaultHttpClientFactory : IHttpClientFactory, IHttpMessageHandle
         // This seems frequent enough. We also rely on GC occurring to actually trigger disposal.
         private readonly TimeSpan DefaultCleanupInterval = TimeSpan.FromSeconds(10);
 
-        // We use a new timer for each regular cleanup cycle, protected with a lock. Note that this scheme
-        // doesn't give us anything to dispose, as the timer is started/stopped as needed.
+        // We use a new timer for each regular cleanup cycle, protected with a lock.
         //
-        // There's no need for the factory itself to be disposable. If you stop using it, eventually everything will
-        // get reclaimed.
+        // The factory implements IDisposable to ensure that resources are properly cleaned up when the hosting
+        // service provider is disposed. This prevents memory leaks in applications that create and dispose
+        // service providers frequently.
         private Timer? _cleanupTimer;
         private readonly object _cleanupTimerLock;
         private readonly object _cleanupActiveLock;
+        private bool _disposed;
 
         // Collection of 'active' handlers.
         //
@@ -104,6 +105,8 @@ public HttpClient CreateClient(string name)
         {
             ArgumentNullException.ThrowIfNull(name);
 
+            ObjectDisposedException.ThrowIf(_disposed, nameof(DefaultHttpClientFactory));
+
             HttpMessageHandler handler = CreateHandler(name);
             var client = new HttpClient(handler, disposeHandler: false);
 
@@ -120,6 +123,8 @@ public HttpMessageHandler CreateHandler(string name)
         {
             ArgumentNullException.ThrowIfNull(name);
 
+            ObjectDisposedException.ThrowIf(_disposed, nameof(DefaultHttpClientFactory));
+
             ActiveHandlerTrackingEntry entry = _activeHandlers.GetOrAdd(name, _entryFactory).Value;
 
             StartHandlerEntryTimer(entry);
@@ -194,6 +199,12 @@ internal void ExpiryTimer_Tick(object? state)
         {
             var active = (ActiveHandlerTrackingEntry)state!;
 
+            // If factory is disposed, don't process expiry
+            if (_disposed)
+            {
+                return;
+            }
+
             // The timer callback should be the only one removing from the active collection. If we can't find
             // our entry in the collection, then this is a bug.
             bool removed = _activeHandlers.TryRemove(active.Name, out Lazy<ActiveHandlerTrackingEntry>? found);
@@ -206,12 +217,17 @@ internal void ExpiryTimer_Tick(object? state)
             //
             // We use a different state object to track expired handlers. This allows any other thread that acquired
             // the 'active' entry to use it without safety problems.
-            var expired = new ExpiredHandlerTrackingEntry(active);
-            _expiredHandlers.Enqueue(expired);
 
-            Log.HandlerExpired(_logger, active.Name, active.Lifetime);
+            // Check again after removal to prevent race with Dispose
+            if (!_disposed)
+            {
+                var expired = new ExpiredHandlerTrackingEntry(active);
+                _expiredHandlers.Enqueue(expired);
 
-            StartCleanupTimer();
+                Log.HandlerExpired(_logger, active.Name, active.Lifetime);
+
+                StartCleanupTimer();
+            }
         }
 
         // Internal so it can be overridden in tests
@@ -225,7 +241,11 @@ internal virtual void StartCleanupTimer()
         {
             lock (_cleanupTimerLock)
             {
-                _cleanupTimer ??= NonCapturingTimer.Create(_cleanupCallback, this, DefaultCleanupInterval, Timeout.InfiniteTimeSpan);
+                // Don't start cleanup timer if factory is disposed
+                if (!_disposed)
+                {
+                    _cleanupTimer ??= NonCapturingTimer.Create(_cleanupCallback, this, DefaultCleanupInterval, Timeout.InfiniteTimeSpan);
+                }
             }
         }
 
@@ -234,7 +254,7 @@ internal virtual void StopCleanupTimer()
         {
             lock (_cleanupTimerLock)
             {
-                _cleanupTimer!.Dispose();
+                _cleanupTimer?.Dispose();
                 _cleanupTimer = null;
             }
         }
@@ -252,6 +272,12 @@ internal void CleanupTimer_Tick()
             // whether we need to start the timer.
             StopCleanupTimer();
 
+            // If factory is disposed, don't perform any cleanup
+            if (_disposed)
+            {
+                return;
+            }
+
             if (!Monitor.TryEnter(_cleanupActiveLock))
             {
                 // We don't want to run a concurrent cleanup cycle. This can happen if the cleanup cycle takes
@@ -266,6 +292,12 @@ internal void CleanupTimer_Tick()
 
             try
             {
+                // Check again after acquiring the lock in case Dispose was called
+                if (_disposed)
+                {
+                    return;
+                }
+
                 int initialCount = _expiredHandlers.Count;
                 Log.CleanupCycleStart(_logger, initialCount);
 
@@ -282,8 +314,8 @@ internal void CleanupTimer_Tick()
                     {
                         try
                         {
-                            entry.InnerHandler.Dispose();
-                            entry.Scope?.Dispose();
+                            // During normal cleanup, let the entry check CanDispose itself
+                            entry.Dispose();
                             disposedCount++;
                         }
                         catch (Exception ex)
@@ -307,12 +339,76 @@ internal void CleanupTimer_Tick()
             }
 
             // We didn't totally empty the cleanup queue, try again later.
-            if (!_expiredHandlers.IsEmpty)
+            // But only if not disposed
+            if (!_expiredHandlers.IsEmpty && !_disposed)
             {
                 StartCleanupTimer();
             }
         }
 
+        public void Dispose()
+        {
+            if (_disposed)
+            {
+                return;
+            }
+
+            _disposed = true;
+
+            // Stop the cleanup timer
+            StopCleanupTimer();
+
+            // Stop all active handler timers to prevent more entries being added to _expiredHandlers
+            List<IDisposable> disposables = new List<IDisposable>();
+
+            // Lock to safely collect handlers from active and expired collections
+            lock (_cleanupActiveLock)
+            {
+                // Stop active handler timers and collect expired handlers
+                foreach (KeyValuePair<string, Lazy<ActiveHandlerTrackingEntry>> entry in _activeHandlers)
+                {
+                    ActiveHandlerTrackingEntry activeEntry = entry.Value.Value;
+                    activeEntry.StopTimer();
+                    disposables.Add(activeEntry);
+                }
+
+                // Collect all expired handlers for disposal
+                while (_expiredHandlers.TryDequeue(out ExpiredHandlerTrackingEntry? entry))
+                {
+                    if (entry != null)
+                    {
+                        // During explicit disposal, we force disposal of all handlers regardless of CanDispose status
+                        disposables.Add(entry);
+
+                        // Force dispose the scope even if the handler is still alive
+                        if (entry.Scope != null)
+                        {
+                            disposables.Add(entry.Scope);
+                        }
+                    }
+                }
+
+                // Clear the collections to help with garbage collection
+                _activeHandlers.Clear();
+            }
+
+            // Dispose all handlers outside the lock
+            foreach (IDisposable disposable in disposables)
+            {
+                try
+                {
+                    disposable.Dispose();
+                }
+                catch (Exception ex)
+                {
+                    Log.CleanupItemFailed(_logger,
+                        disposable is ActiveHandlerTrackingEntry active ? active.Name :
+                        disposable is ExpiredHandlerTrackingEntry expired ? expired.Name :
+                        "Unknown", ex);
+                }
+            }
+        }
+
         private static class Log
         {
             public static class EventIds

src/libraries/Microsoft.Extensions.Http/src/ExpiredHandlerTrackingEntry.cs

@@ -8,7 +8,7 @@
 namespace Microsoft.Extensions.Http
 {
     // Thread-safety: This class is immutable
-    internal sealed class ExpiredHandlerTrackingEntry
+    internal sealed class ExpiredHandlerTrackingEntry : IDisposable
     {
         private readonly WeakReference _livenessTracker;
 
@@ -23,12 +23,30 @@ public ExpiredHandlerTrackingEntry(ActiveHandlerTrackingEntry other)
             InnerHandler = other.Handler.InnerHandler!;
         }
 
+        // Used during normal cleanup cycles
         public bool CanDispose => !_livenessTracker.IsAlive;
 
         public HttpMessageHandler InnerHandler { get; }
 
         public string Name { get; }
 
         public IServiceScope? Scope { get; }
+
+        public void Dispose()
+        {
+            try
+            {
+                InnerHandler.Dispose();
+            }
+            finally
+            {
+                if (!_livenessTracker.IsAlive)
+                {
+                    Scope?.Dispose();
+                }
+                // If IsAlive is true, it means the handler is still in use
+                // Don't dispose the scope as it's still being used with the handler
+            }
+        }
     }
 }