Bugfix/113494 fix defaulthttpclientfactory memory leaks

[Maximys]

Current PR fix memory leaks, linked with requirements of System.Threading.Timer recreation, but it does not fix root couse of #113494 , which linked with scope creation by this line.
I think, the most applicable way for fix root couse is change of IHttpClientFactory , which should be represented by two method groups:

1. for rent Client and Handler;
2. for return Client and Handler

like it represented by ArrayPool. Then we can dispose created scope inside second method group.

[Maximys]

@CarnaViire , can you review current PR?

[CarnaViire]

Current PR fix memory leaks, linked with requirements of System.Threading.Timer recreation

Can you please explain what "memory leak" are you referring to and how exactly your PR addresses it?

As far as I'm aware, there is no memory leak associated with the timers. The only problem is that the timers outlive the ServiceProvider (due to DefaultHttpClientFactory not implementing IDisposable), but they are not leaking, they properly clean up after themselves, as explained in #113494 (comment)

[Maximys]

If we'll add some additional async code to your code sample from here , like next:

for (int i = 0; i < 10; i++)
{
    var services = new ServiceCollection();
    services.AddLogging(builder =>
    {
        builder.AddConsole().SetMinimumLevel(LogLevel.Trace);
    });

    services.AddScoped<DisposableService>();
    services.AddTransient<TrackingHandler>();

    services.AddHttpClient("TestClient")
            .AddHttpMessageHandler<TrackingHandler>()
            .RemoveAllLoggers()
            .SetHandlerLifetime(TimeSpan.FromSeconds(1)); // Set a short lifetime to force handler to expire quickly.

    using var serviceProvider = services.BuildServiceProvider();
    var httpClientFactory = serviceProvider.GetRequiredService<IHttpClientFactory>();

    Parallel.For(0, 1000, async (int i) => {
        var client = httpClientFactory.CreateClient("TestClient");
        Console.WriteLine($"Iteration {i + 1}: HttpClient acquired");

        try
        {
            _ = await client.GetAsync("https://example.com");
        }
        catch (Exception)
        {
            // ignore
        }
    });
}

Console.WriteLine("Waiting for handlers to expire (lifetime = 1s)...");
await Task.Delay(TimeSpan.FromSeconds(2));

Console.WriteLine("Forcing garbage collection... collecting HttpClients to enable expired handler cleanup");
GC.Collect();
GC.WaitForPendingFinalizers();
Console.WriteLine("GC finished");

Console.WriteLine("Waiting for the cleanup timer to fire (every 10s)...");
await Task.Delay(TimeSpan.FromSeconds(15));
Console.WriteLine("15s wait completed");

// Output disposal statistics
DisposableService.PrintStats();
TrackingHandler.PrintStats();

we will get output like:

...
15s wait completed
DisposableService instances created: 164, disposed: 160
TrackingHandler instances created: 164, disposed: 160

and comparison of memory snapshot will display us something like:

which meen that not all cleanups was finished and not all Timer was disposed.
After investigation reason of this behaviour I decided that it's linked with that sometimes existed code work not so as expected.

[CarnaViire]

May I please ask you to revert all the refactorings that are not strictly necessary for the proposed functional change (both in the source code and the tests) -- this bloats up the PR, and blurs the value that it is supposed to introduce.

[CarnaViire]

This would unnecessary allocate each time an ActiveHandlerTrackingEntry is created.

In general, I am not sure the use of the heavy-weight System.Timers.Timer is justified here.

[CarnaViire]

Thanks for the snippet above, I will review it and return to you a bit later

[CarnaViire]

@Maximys I've checked your example, and the reason behind this behavior is simply that not all HttpClients were GC'ed with that single GC.Collect. The cleanup is only possible when the handlers are not held by any of the clients anymore. I assume that the delay might be related to the load of the additional tasks scheduled and other Parallel.For machinery, so now it just takes a little bit more time than the arbitrary chosen wait interval.

To make sure all the HttpClients are collected before we initiate the cleanup wait, we can e.g. track them with WeakRefs:

var clients = new ConcurrentBag<WeakReference>();
for (int i = 0; i < 10; i++)
{
    //...
    Parallel.For(0, 1000, async i =>
    {
        var client = httpClientFactory.CreateClient("TestClient");
        clients.Add(new WeakReference(client));
        //...
    });
}
//...
var aliveClients = clients.Where(wr => wr.IsAlive).ToList();
while (aliveClients.Count > 0)
{
    Console.WriteLine($"Remaining HttpClients: {aliveClients.Count}");
    await Task.Delay(TimeSpan.FromSeconds(1));
    Console.WriteLine("Forcing garbage collection... collecting HttpClients to enable expired handler cleanup");
    GC.Collect();
    GC.WaitForPendingFinalizers();
    Console.WriteLine("GC finished");
    aliveClients = [.. aliveClients.Where(wr => wr.IsAlive)];
}
Console.WriteLine("Waiting for the cleanup timer to fire (every 10s)...");
//...

and the output:

...
Waiting for handlers to expire (lifetime = 1s)...
Remaining HttpClients: 4885
Forcing garbage collection... collecting HttpClients to enable expired handler cleanup
GC finished
Remaining HttpClients: 1425
Forcing garbage collection... collecting HttpClients to enable expired handler cleanup
GC finished
Waiting for the cleanup timer to fire (every 10s)...
15s wait completed
DisposableService instances created: 10, disposed: 10
TrackingHandler instances created: 10, disposed: 10