Merge 62c06781cc into 02596ba8d9

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 @@ namespace Microsoft.Extensions.Http
 
         public void StartExpiryTimer(TimerCallback callback)
         {
+            if (_disposed > 0)
+            {
+                return;
+            }
+
             if (Lifetime == Timeout.InfiniteTimeSpan)
             {
                 return; // never expires.
@@ -62,7 +71,8 @@ namespace Microsoft.Extensions.Http
 
             lock (_lock)
             {
-                if (Volatile.Read(ref _timerInitialized))
+                if (Volatile.Read(ref _timerInitialized) ||
+                    _disposed > 0)
                 {
                     return;
                 }
@@ -76,7 +86,7 @@ namespace Microsoft.Extensions.Http
         private void Timer_Tick()
         {
             Debug.Assert(_callback != null);
-            Debug.Assert(_timer != null);
+            Debug.Assert(_timer != null || _disposed > 0);
 
             lock (_lock)
             {
@@ -85,9 +95,44 @@ namespace Microsoft.Extensions.Http
                     _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 @@ using Microsoft.Extensions.Options;
 
 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 @@ namespace Microsoft.Extensions.Http
         // 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 @@ namespace Microsoft.Extensions.Http
         {
             ArgumentNullException.ThrowIfNull(name);
 
+            ObjectDisposedException.ThrowIf(_disposed, nameof(DefaultHttpClientFactory));
+
             HttpMessageHandler handler = CreateHandler(name);
             var client = new HttpClient(handler, disposeHandler: false);
 
@@ -120,6 +123,8 @@ namespace Microsoft.Extensions.Http
         {
             ArgumentNullException.ThrowIfNull(name);
 
+            ObjectDisposedException.ThrowIf(_disposed, nameof(DefaultHttpClientFactory));
+
             ActiveHandlerTrackingEntry entry = _activeHandlers.GetOrAdd(name, _entryFactory).Value;
 
             StartHandlerEntryTimer(entry);
@@ -194,6 +199,12 @@ namespace Microsoft.Extensions.Http
         {
             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 @@ namespace Microsoft.Extensions.Http
             //
             // 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 @@ namespace Microsoft.Extensions.Http
         {
             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 @@ namespace Microsoft.Extensions.Http
         {
             lock (_cleanupTimerLock)
             {
-                _cleanupTimer!.Dispose();
+                _cleanupTimer?.Dispose();
                 _cleanupTimer = null;
             }
         }
@@ -252,6 +272,12 @@ namespace Microsoft.Extensions.Http
             // 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 @@ namespace Microsoft.Extensions.Http
 
             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 @@ namespace Microsoft.Extensions.Http
                     {
                         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 @@ namespace Microsoft.Extensions.Http
             }
 
             // 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 @@ using Microsoft.Extensions.DependencyInjection;
 namespace Microsoft.Extensions.Http
 {
     // Thread-safety: This class is immutable
-    internal sealed class ExpiredHandlerTrackingEntry
+    internal sealed class ExpiredHandlerTrackingEntry : IDisposable
     {
         private readonly WeakReference _livenessTracker;
 
@@ -23,6 +23,7 @@ namespace Microsoft.Extensions.Http
             InnerHandler = other.Handler.InnerHandler!;
         }
 
+        // Used during normal cleanup cycles
         public bool CanDispose => !_livenessTracker.IsAlive;
 
         public HttpMessageHandler InnerHandler { get; }
@@ -30,5 +31,22 @@ namespace Microsoft.Extensions.Http
         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
+            }
+        }
     }
 }

src/libraries/Microsoft.Extensions.Http/tests/Microsoft.Extensions.Http.Tests/HttpClientFactoryDisposeTests.cs

@@ -0,0 +1,280 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Collections.Concurrent;
+using System.Collections.Generic;
+using System.Linq;
+using System.Net.Http;
+using System.Threading;
+using System.Threading.Tasks;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Options;
+using Xunit;
+
+namespace Microsoft.Extensions.Http.Tests
+{
+    public class HttpClientFactoryDisposeTests
+    {
+        private const int ClientCount = 5;
+
+        [Fact]
+        public async Task DisposingServiceProvider_DisposesHttpClientFactory_ReleasesResources()
+        {
+            // Arrange
+            var disposeCounter = new DisposeCounter();
+            var services = new ServiceCollection();
+            services.AddSingleton(disposeCounter);
+            
+            // Add a scoped service that will be disposed when handlers are disposed
+            services.AddScoped<ITestScopedService, TestScopedService>();
+            
+            // Register handlers for multiple named clients
+            for (int i = 0; i < ClientCount; i++)
+            {
+                string clientName = $"test-client-{i}";
+                services.AddHttpClient(clientName, client => { })
+                    .ConfigurePrimaryHttpMessageHandler(provider => new DisposeTrackingHandler(disposeCounter, provider.GetRequiredService<ITestScopedService>()));
+            }
+            
+            // Build service provider and create test factory
+            var serviceProvider = services.BuildServiceProvider();
+            var scopeFactory = serviceProvider.GetRequiredService<IServiceScopeFactory>();
+            var optionsMonitor = serviceProvider.GetRequiredService<IOptionsMonitor<HttpClientFactoryOptions>>();
+            var filters = serviceProvider.GetServices<IHttpMessageHandlerBuilderFilter>();
+            
+            var factory = new TestHttpClientFactory(serviceProvider, scopeFactory, optionsMonitor, filters)
+            {
+                EnableExpiryTimer = true,
+                EnableCleanupTimer = true
+            };
+            
+            // Create clients to initialize active handlers
+            var clients = new List<HttpClient>();
+            for (int i = 0; i < ClientCount; i++)
+            {
+                string clientName = $"test-client-{i}";
+                var client = factory.CreateClient(clientName);
+                clients.Add(client);
+                
+                // Use the client to ensure the handler is created
+                var response = await client.SendAsync(new HttpRequestMessage(HttpMethod.Get, "http://example.com"));
+            }
+            
+            // Verify handlers were created
+            Assert.Equal(ClientCount, disposeCounter.Created);
+            Assert.Equal(ClientCount, disposeCounter.ScopedServicesCreated);
+            
+            // Force expiry of all active handlers
+            var activeEntries = new List<(ActiveHandlerTrackingEntry, TaskCompletionSource<ActiveHandlerTrackingEntry>, Task)>();
+            foreach (var kvp in factory.ActiveEntryState)
+            {
+                activeEntries.Add((kvp.Key, kvp.Value.Item1, kvp.Value.Item2));
+                kvp.Value.Item1.SetResult(kvp.Key);
+            }
+            
+            // Wait for all handlers to expire
+            foreach (var entry in activeEntries)
+            {
+                await entry.Item3;
+            }
+            
+            // Verify all handlers are now expired
+            Assert.Equal(ClientCount, factory._expiredHandlers.Count);
+            Assert.Empty(factory._activeHandlers);
+            
+            // Clear client references to allow GC
+            clients.Clear();
+            activeEntries.Clear();
+            
+            // Create new clients (these will be active handlers)
+            for (int i = 0; i < ClientCount; i++)
+            {
+                string clientName = $"test-client-{i}";
+                var client = factory.CreateClient(clientName);
+                
+                // Use the client to ensure the handler is created
+                var response = await client.SendAsync(new HttpRequestMessage(HttpMethod.Get, "http://example.com"));
+            }
+            
+            // Verify we now have both expired and active handlers
+            Assert.Equal(ClientCount, factory._expiredHandlers.Count); // expired handlers
+            Assert.Equal(ClientCount, factory._activeHandlers.Count); // active handlers
+            Assert.Equal(ClientCount * 2, disposeCounter.Created); // total handlers created
+            Assert.Equal(ClientCount * 2, disposeCounter.ScopedServicesCreated); // total scoped services created
+            
+            // Act - dispose the factory which should dispose all handlers and their scopes
+            factory.Dispose();
+            
+            // Assert - all handlers and scoped services should be disposed
+            Assert.Equal(disposeCounter.Created, disposeCounter.Disposed);
+            Assert.Equal(disposeCounter.ScopedServicesCreated, disposeCounter.ScopedServicesDisposed);
+        }
+
+        private interface ITestScopedService : IDisposable
+        {
+            void DoSomething();
+        }
+
+        private class TestScopedService : ITestScopedService
+        {
+            private readonly DisposeCounter _counter;
+            private bool _disposed;
+
+            public TestScopedService(DisposeCounter counter)
+            {
+                _counter = counter;
+                _counter.IncrementScopedServicesCreated();
+            }
+
+            public void DoSomething()
+            {
+                // Test method to ensure service is being used
+            }
+
+            public void Dispose()
+            {
+                if (!_disposed)
+                {
+                    _disposed = true;
+                    _counter.IncrementScopedServicesDisposed();
+                }
+            }
+        }
+
+        private class DisposeCounter
+        {
+            private int _created;
+            private int _disposed;
+            private int _scopedServicesCreated;
+            private int _scopedServicesDisposed;
+
+            public int Created => Interlocked.CompareExchange(ref _created, 0, 0);
+            public int Disposed => Interlocked.CompareExchange(ref _disposed, 0, 0);
+            public int ScopedServicesCreated => Interlocked.CompareExchange(ref _scopedServicesCreated, 0, 0);
+            public int ScopedServicesDisposed => Interlocked.CompareExchange(ref _scopedServicesDisposed, 0, 0);
+
+            public void IncrementCreated()
+            {
+                Interlocked.Increment(ref _created);
+            }
+
+            public void IncrementDisposed()
+            {
+                Interlocked.Increment(ref _disposed);
+            }
+
+            public void IncrementScopedServicesCreated()
+            {
+                Interlocked.Increment(ref _scopedServicesCreated);
+            }
+
+            public void IncrementScopedServicesDisposed()
+            {
+                Interlocked.Increment(ref _scopedServicesDisposed);
+            }
+        }
+
+        private class DisposeTrackingHandler : HttpMessageHandler
+        {
+            private readonly DisposeCounter _counter;
+            private readonly ITestScopedService _scopedService;
+
+            public DisposeTrackingHandler(DisposeCounter counter, ITestScopedService scopedService)
+            {
+                _counter = counter;
+                _scopedService = scopedService;
+                _counter.IncrementCreated();
+            }
+
+            protected override void Dispose(bool disposing)
+            {
+                if (disposing)
+                {
+                    _counter.IncrementDisposed();
+                }
+
+                base.Dispose(disposing);
+            }
+
+            protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
+            {
+                // Use the scoped service to create the dependency
+                _scopedService.DoSomething();
+                
+                // Just return a simple response for test purposes
+                return Task.FromResult(new HttpResponseMessage(System.Net.HttpStatusCode.OK));
+            }
+        }
+
+        // Test factory from DefaultHttpClientFactoryTest.cs
+        private class TestHttpClientFactory : DefaultHttpClientFactory
+        {
+            public TestHttpClientFactory(
+                IServiceProvider services,
+                IServiceScopeFactory scopeFactory,
+                IOptionsMonitor<HttpClientFactoryOptions> optionsMonitor,
+                IEnumerable<IHttpMessageHandlerBuilderFilter> filters)
+                : base(services, scopeFactory, optionsMonitor, filters)
+            {
+                ActiveEntryState = new Dictionary<ActiveHandlerTrackingEntry, (TaskCompletionSource<ActiveHandlerTrackingEntry>, Task)>();
+                CleanupTimerStarted = new ManualResetEventSlim(initialState: false);
+            }
+
+            public bool EnableExpiryTimer { get; set; }
+
+            public bool EnableCleanupTimer { get; set; }
+
+            public ManualResetEventSlim CleanupTimerStarted { get; }
+
+            public Dictionary<ActiveHandlerTrackingEntry, (TaskCompletionSource<ActiveHandlerTrackingEntry>, Task)> ActiveEntryState { get; }
+
+            internal override void StartHandlerEntryTimer(ActiveHandlerTrackingEntry entry)
+            {
+                if (EnableExpiryTimer)
+                {
+                    lock (ActiveEntryState)
+                    {
+                        if (ActiveEntryState.ContainsKey(entry))
+                        {
+                            // Timer already started.
+                            return;
+                        }
+
+                        // Rather than using the actual timer on the actual entry, let's fake it with async.
+                        var completionSource = new TaskCompletionSource<ActiveHandlerTrackingEntry>();
+                        var expiryTask = completionSource.Task.ContinueWith(t =>
+                        {
+                            var e = t.Result;
+                            ExpiryTimer_Tick(e);
+
+                            lock (ActiveEntryState)
+                            {
+                                ActiveEntryState.Remove(e);
+                            }
+                        });
+
+                        ActiveEntryState.Add(entry, (completionSource, expiryTask));
+                    }
+                }
+            }
+
+            internal override void StartCleanupTimer()
+            {
+                if (EnableCleanupTimer)
+                {
+                    CleanupTimerStarted.Set();
+                }
+            }
+
+            internal override void StopCleanupTimer()
+            {
+                if (EnableCleanupTimer)
+                {
+                    Assert.True(CleanupTimerStarted.IsSet, "Cleanup timer started");
+                    CleanupTimerStarted.Reset();
+                }
+            }
+        }
+    }
+}