Bugfix/113494 fix defaulthttpclientfactory memory leaks

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

@@ -10,37 +10,37 @@
 using System.Net.Http;
 using System.Threading;
 using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Http.LifetimeProcessing.Handlers;
+using Microsoft.Extensions.Http.LifetimeProcessing.Handlers.Entries;
 using Microsoft.Extensions.Internal;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
+using Timer = System.Timers.Timer;
 
 namespace Microsoft.Extensions.Http
 {
     internal class DefaultHttpClientFactory : IHttpClientFactory, IHttpMessageHandlerFactory
     {
-        private static readonly TimerCallback _cleanupCallback = (s) => ((DefaultHttpClientFactory)s!).CleanupTimer_Tick();
+        // Default time of 10s for cleanup seems reasonable.
+        // Quick math:
+        // 10 distinct named clients * expiry time >= 1s = approximate cleanup queue of 100 items
+        //
+        // This seems frequent enough. We also rely on GC occurring to actually trigger disposal.
+        private const double DefaultCleanupInterval = 10000;
+
         private readonly IServiceProvider _services;
         private readonly IServiceScopeFactory _scopeFactory;
         private readonly IOptionsMonitor<HttpClientFactoryOptions> _optionsMonitor;
         private readonly IHttpMessageHandlerBuilderFilter[] _filters;
         private readonly Func<string, Lazy<ActiveHandlerTrackingEntry>> _entryFactory;
         private readonly Lazy<ILogger> _logger;
 
-        // Default time of 10s for cleanup seems reasonable.
-        // Quick math:
-        // 10 distinct named clients * expiry time >= 1s = approximate cleanup queue of 100 items
-        //
-        // 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.
         //
         // There's no need for the factory itself to be disposable. If you stop using it, eventually everything will
         // get reclaimed.
-        private Timer? _cleanupTimer;
-        private readonly object _cleanupTimerLock;
-        private readonly object _cleanupActiveLock;
+        private Timer _cleanupTimer;
 
         // Collection of 'active' handlers.
         //
@@ -57,7 +57,6 @@ internal class DefaultHttpClientFactory : IHttpClientFactory, IHttpMessageHandle
         //
         // internal for tests
         internal readonly ConcurrentQueue<ExpiredHandlerTrackingEntry> _expiredHandlers;
-        private readonly TimerCallback _expiryCallback;
 
         public DefaultHttpClientFactory(
             IServiceProvider services,
@@ -86,10 +85,12 @@ public DefaultHttpClientFactory(
             };
 
             _expiredHandlers = new ConcurrentQueue<ExpiredHandlerTrackingEntry>();
-            _expiryCallback = ExpiryTimer_Tick;
 
-            _cleanupTimerLock = new object();
-            _cleanupActiveLock = new object();
+            _cleanupTimer = new Timer(DefaultCleanupInterval)
+            {
+                AutoReset = false
+            };
+            _cleanupTimer.Elapsed += (_, _) => CleanupTimer_Tick();
 
             // We want to prevent a circular dependency between ILoggerFactory and IHttpClientFactory, in case
             // any of ILoggerProvider instances use IHttpClientFactory to send logs to an external server.
@@ -190,53 +191,45 @@ void Configure(HttpMessageHandlerBuilder b)
         }
 
         // Internal for tests
-        internal void ExpiryTimer_Tick(object? state)
+        internal void ExpiryTimer_Tick(ActiveHandlerTrackingEntry activeHandler)
         {
-            var active = (ActiveHandlerTrackingEntry)state!;
-
             // 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);
+            bool removed = _activeHandlers.TryRemove(activeHandler.Name, out Lazy<ActiveHandlerTrackingEntry>? found);
             Debug.Assert(removed, "Entry not found. We should always be able to remove the entry");
-            Debug.Assert(object.ReferenceEquals(active, found!.Value), "Different entry found. The entry should not have been replaced");
+            Debug.Assert(object.ReferenceEquals(activeHandler, found!.Value), "Different entry found. The entry should not have been replaced");
 
             // At this point the handler is no longer 'active' and will not be handed out to any new clients.
             // However we haven't dropped our strong reference to the handler, so we can't yet determine if
             // there are still any other outstanding references (we know there is at least one).
             //
             // 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);
+            var expired = new ExpiredHandlerTrackingEntry(activeHandler);
             _expiredHandlers.Enqueue(expired);
+            activeHandler.Dispose();
 
-            Log.HandlerExpired(_logger, active.Name, active.Lifetime);
+            Log.HandlerExpired(_logger, activeHandler.Name, activeHandler.Lifetime);
 
             StartCleanupTimer();
         }
 
         // Internal so it can be overridden in tests
         internal virtual void StartHandlerEntryTimer(ActiveHandlerTrackingEntry entry)
         {
-            entry.StartExpiryTimer(_expiryCallback);
+            entry.StartExpiryTimer(ExpiryTimer_Tick);
         }
 
         // Internal so it can be overridden in tests
         internal virtual void StartCleanupTimer()
         {
-            lock (_cleanupTimerLock)
-            {
-                _cleanupTimer ??= NonCapturingTimer.Create(_cleanupCallback, this, DefaultCleanupInterval, Timeout.InfiniteTimeSpan);
-            }
+            _cleanupTimer.Start();
         }
 
         // Internal so it can be overridden in tests
         internal virtual void StopCleanupTimer()
         {
-            lock (_cleanupTimerLock)
-            {
-                _cleanupTimer!.Dispose();
-                _cleanupTimer = null;
-            }
+            _cleanupTimer.Stop();
         }
 
         // Internal for tests
@@ -252,65 +245,60 @@ internal void CleanupTimer_Tick()
             // whether we need to start the timer.
             StopCleanupTimer();
 
-            if (!Monitor.TryEnter(_cleanupActiveLock))
-            {
-                // We don't want to run a concurrent cleanup cycle. This can happen if the cleanup cycle takes
-                // a long time for some reason. Since we're running user code inside Dispose, it's definitely
-                // possible.
-                //
-                // If we end up in that position, just make sure the timer gets started again. It should be cheap
-                // to run a 'no-op' cleanup.
-                StartCleanupTimer();
-                return;
-            }
-
             try
             {
                 int initialCount = _expiredHandlers.Count;
                 Log.CleanupCycleStart(_logger, initialCount);
 
                 var stopwatch = ValueStopwatch.StartNew();
 
-                int disposedCount = 0;
-                for (int i = 0; i < initialCount; i++)
-                {
-                    // Since we're the only one removing from _expired, TryDequeue must always succeed.
-                    _expiredHandlers.TryDequeue(out ExpiredHandlerTrackingEntry? entry);
-                    Debug.Assert(entry != null, "Entry was null, we should always get an entry back from TryDequeue");
-
-                    if (entry.CanDispose)
-                    {
-                        try
-                        {
-                            entry.InnerHandler.Dispose();
-                            entry.Scope?.Dispose();
-                            disposedCount++;
-                        }
-                        catch (Exception ex)
-                        {
-                            Log.CleanupItemFailed(_logger, entry.Name, ex);
-                        }
-                    }
-                    else
-                    {
-                        // If the entry is still live, put it back in the queue so we can process it
-                        // during the next cleanup cycle.
-                        _expiredHandlers.Enqueue(entry);
-                    }
-                }
+                int disposedCount = CleanupExpiredHandlers();
 
                 Log.CleanupCycleEnd(_logger, stopwatch.GetElapsedTime(), disposedCount, _expiredHandlers.Count);
             }
             finally
             {
-                Monitor.Exit(_cleanupActiveLock);
+                // We didn't totally empty the cleanup queue, try again later.
+                if (!_expiredHandlers.IsEmpty)
+                {
+                    StartCleanupTimer();
+                }
             }
+        }
+
+        private int CleanupExpiredHandlers()
+        {
+            int initialCount = _expiredHandlers.Count;
 
-            // We didn't totally empty the cleanup queue, try again later.
-            if (!_expiredHandlers.IsEmpty)
+            int disposedCount = 0;
+            for (int i = 0; i < initialCount; i++)
             {
-                StartCleanupTimer();
+                // Since we're the only one removing from _expired, TryDequeue must always succeed.
+                _expiredHandlers.TryDequeue(out ExpiredHandlerTrackingEntry? entry);
+                Debug.Assert(entry != null, "Entry was null, we should always get an entry back from TryDequeue");
+
+                if (entry.CanDispose)
+                {
+                    try
+                    {
+                        entry.InnerHandler.Dispose();
+                        entry.Scope?.Dispose();
+                        disposedCount++;
+                    }
+                    catch (Exception ex)
+                    {
+                        Log.CleanupItemFailed(_logger, entry.Name, ex);
+                    }
+                }
+                else
+                {
+                    // If the entry is still live, put it back in the queue so we can process it
+                    // during the next cleanup cycle.
+                    _expiredHandlers.Enqueue(entry);
+                }
             }
+
+            return disposedCount;
         }
 
         private static class Log

src/libraries/Microsoft.Extensions.Http/src/DependencyInjection/HttpClientKeyedLifetime.cs

@@ -3,7 +3,7 @@
 
 using System;
 using System.Net.Http;
-using Microsoft.Extensions.Http;
+using Microsoft.Extensions.Http.LifetimeProcessing.Handlers;
 
 namespace Microsoft.Extensions.DependencyInjection
 {

src/libraries/Microsoft.Extensions.Http/src/ActiveHandlerTrackingEntry.cs → src/libraries/Microsoft.Extensions.Http/src/LifetimeProcessing/Handlers/Entries/ActiveHandlerTrackingEntry.cs

@@ -5,71 +5,71 @@
 using System.Diagnostics;
 using System.Threading;
 using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Http.LifetimeProcessing.Handlers.Entries.Base;
 using Microsoft.Extensions.Internal;
+using Timer = System.Timers.Timer;
 
-namespace Microsoft.Extensions.Http
+namespace Microsoft.Extensions.Http.LifetimeProcessing.Handlers.Entries
 {
     // 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 : HandlerTrackingEntryBase, IDisposable
     {
-        private static readonly TimerCallback _timerCallback = (s) => ((ActiveHandlerTrackingEntry)s!).Timer_Tick();
         private readonly object _lock;
-        private bool _timerInitialized;
+        private bool _isDisposed;
         private Timer? _timer;
-        private TimerCallback? _callback;
+        private Action<ActiveHandlerTrackingEntry>? _callback;
 
         public ActiveHandlerTrackingEntry(
             string name,
             LifetimeTrackingHttpMessageHandler handler,
             IServiceScope? scope,
             TimeSpan lifetime)
+            : base(name, scope)
         {
-            Name = name;
             Handler = handler;
-            Scope = scope;
             Lifetime = lifetime;
 
             _lock = new object();
+            _timer = new Timer(lifetime.TotalMilliseconds)
+            {
+                AutoReset = false
+            };
+            _timer.Elapsed += (_, _) => Timer_Tick();
         }
 
         public LifetimeTrackingHttpMessageHandler Handler { get; }
 
         public TimeSpan Lifetime { get; }
 
-        public string Name { get; }
-
-        public IServiceScope? Scope { get; }
+        public void Dispose()
+        {
+            Dispose(true);
+            GC.SuppressFinalize(this);
+        }
 
-        public void StartExpiryTimer(TimerCallback callback)
+        public void StartExpiryTimer(Action<ActiveHandlerTrackingEntry> callback)
         {
             if (Lifetime == Timeout.InfiniteTimeSpan)
             {
                 return; // never expires.
             }
 
-            if (Volatile.Read(ref _timerInitialized))
-            {
-                return;
-            }
-
-            StartExpiryTimerSlow(callback);
+            _callback = callback;
+            _timer!.Start();
         }
 
-        private void StartExpiryTimerSlow(TimerCallback callback)
+        private void Dispose(bool disposing)
         {
-            Debug.Assert(Lifetime != Timeout.InfiniteTimeSpan);
-
-            lock (_lock)
+            if (!_isDisposed)
             {
-                if (Volatile.Read(ref _timerInitialized))
+                _isDisposed = true;
+
+                if (disposing)
                 {
-                    return;
+                    _timer?.Dispose();
+                    _timer = null;
                 }
-
-                _callback = callback;
-                _timer = NonCapturingTimer.Create(_timerCallback, this, Lifetime, Timeout.InfiniteTimeSpan);
-                _timerInitialized = true;
             }
         }
 
@@ -78,16 +78,7 @@ private void Timer_Tick()
             Debug.Assert(_callback != null);
             Debug.Assert(_timer != null);
 
-            lock (_lock)
-            {
-                if (_timer != null)
-                {
-                    _timer.Dispose();
-                    _timer = null;
-
-                    _callback(this);
-                }
-            }
+            _callback(this);
         }
     }
 }

src/libraries/Microsoft.Extensions.Http/src/LifetimeProcessing/Handlers/Entries/Base/HandlerTrackingEntryBase.cs

@@ -0,0 +1,22 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using Microsoft.Extensions.DependencyInjection;
+
+namespace Microsoft.Extensions.Http.LifetimeProcessing.Handlers.Entries.Base
+{
+    internal abstract class HandlerTrackingEntryBase
+    {
+        protected HandlerTrackingEntryBase(
+            string name,
+            IServiceScope? scope)
+        {
+            Name = name;
+            Scope = scope;
+        }
+
+        public string Name { get; }
+
+        public IServiceScope? Scope { get; }
+    }
+}