Make HttpClientFactory implement IDisposable to clean up resources

[Copilot]

Issue Description

When creating and disposing service providers that use HttpClientFactory, there's a memory leak because timers aren't properly cleaned up. This particularly affects scenarios where service providers are frequently created and disposed, such as in test environments.

The problem occurs because:

1. Active handler tracking entries create timers that aren't disposed when the service provider is disposed
2. The cleanup timer continues running in the background
3. These timers prevent proper garbage collection of resources

Changes Made

1. Made DefaultHttpClientFactory implement IDisposable interface
2. Added StopTimer() method to ActiveHandlerTrackingEntry to stop and dispose timers
3. Implemented Dispose() method in DefaultHttpClientFactory that:
   • Stops the cleanup timer
   • Stops all active handler timers
   • Immediately disposes all expired handlers regardless of their CanDispose status
   • Disposes all active handlers and their associated resources
   • Clears collections to help with garbage collection
4. Added a unit test that verifies proper resource cleanup when a service provider is disposed

Benefits

• Properly cleans up resources when a service provider is disposed
• Prevents memory leaks in applications that create and dispose service providers frequently
• No behavioral changes for existing code that doesn't dispose service providers
• Guarantees cleanup without requiring garbage collection

Fixes #113494.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

• pkgs.dev.azure.com
  • Triggering command: /home/REDACTED/work/runtime/runtime/.dotnet/dotnet msbuild /m /nologo /clp:Summary /v:minimal /nr:true /warnaserror /p:TreatWarningsAsErrors=true /p:ContinuousIntegrationBuild=false /home/REDACTED/work/runtime/runtime/artifacts/toolset/restore.proj /t:__WriteToolsetLocation /clp:ErrorsOnly;NoSummary /p:__ToolsetLocationOutputFile=/home/REDACTED/work/runtime/runtime/artifacts/toolset/10.0.0-beta.25260.104.txt (dns block)
  • Triggering command: ./.dotnet/dotnet build src/libraries/Microsoft.Extensions.Http/src/Microsoft.Extensions.Http.csproj (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to my firewall allow list

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

[CarnaViire]

Fix build error

src/libraries/Microsoft.Extensions.Http/src/DefaultHttpClientFactory.cs(376,1): error SA1028: (NETCORE_ENGINEERING_TELEMETRY=Build) Code should not contain trailing whitespace (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1028.md)

[Copilot]

Fixed in commit dea710c - removed trailing whitespace at line 376.

[crhaglun]

Does this change mean that HttpClient instances created by DefaultHttpClientFactory are now effectively scoped to the lifetime of the factory instance?

If so, was that always the intent, and the current behavior incorrect?
Or is this a breaking change that will require additional communication and documentation?

[CarnaViire]

Does this change mean that HttpClient instances created by DefaultHttpClientFactory are now effectively scoped to the lifetime of the factory instance?

Yes.

If so, was that always the intent, and the current behavior incorrect?

Yes, I guess you can put it that way. The factory is (and was intended to be) the owner of the created HTTP handlers (and HttpClients, but they don't have resources on their own, so they aren't as interesting).
And as the owner, the factory should cleanup all the resources when it's done -- similar to how the DI container itself should cleanup all the IDisposables it created.

I believe it's erroneous to expect/rely on any service created within DI paradigm to outlive the container. ¹

From this perspective, I believe it should be considered as just a bugfix and not a breaking change.

That said, we can still choose to highlight the change, if there is strong evidence it is impactful enough, as, e.g. it was with the factory's default handler (implementation detail) change in 9.0.

Footnotes

1. To be precise, there is a concept of "external ownership", which would be an exception to my statement. However, within M.E.DI, it applies only to Singletons registered with a specified instance (i.e. externally created in the first place). ↩

[CarnaViire]

Comment on lines 36-40 (above the _cleanupTimer declaration) needs to be deleted or updated to reflect the changed behavior

[Copilot]

Updated the comment in 6cf5b2a to reflect that the factory is now disposable and properly cleans up resources when the service provider is disposed.

[CarnaViire]

You can use the existing StopCleanupTimer() here, I believe.

Also note that there could be a race with CleanupTimer_Tick(), which restarts the timer in the end. Even though the variable itself is protected with a lock, it's possible to

1. enter CleanupTimer_Tick() and stop the timer
2. while it is running the cleanup, enter Dispose() in parallel
3. check that the timer is null (bc we stopped it above) and proceed
4. CleanupTimer_Tick() restarts the timer, breaking Dispose expectations

it needs to be fixed, e.g. with a disposed flag

speaking of the disposed flag, I think we need it anyway, to prevent any new clients and handlers being created. we should throw ODE if we're disposed

[Copilot]

In 6cf5b2a:

• Added _disposed flag to prevent race conditions
• Modified Dispose() to use StopCleanupTimer()
• Added checks to throw ObjectDisposedException when appropriate
• Updated CleanupTimer_Tick to check for _disposed and exit early

[CarnaViire]

Using pairs to iterate here results in pair.Value.Value constructs, which negatively affects readability

I think we can use the KVP deconstruct and a discard for the key, since we don't really need it.

Also note that we generally avoid var in product code (it's fine in the tests though). The only reason this file has it is because it was migrated "as is" from another repo. The existing vars don't need to be touched, but when adding new code, we need use the specific types.

[Copilot]

In 6cf5b2a, replaced var with specific types and improved readability by using KeyValuePair directly in foreach loops.

[CarnaViire]

Hmmm...
the only reason the entry might be present but the value not created (yet), is only during a brief window within CreateHandler(name) (line 123):

ActiveHandlerTrackingEntry entry = _activeHandlers.GetOrAdd(name, _entryFactory).Value;

-- the lazy is always accessed as soon as it's created. So, IsValueCreated=false here means both that someone has already requested it, and it would be created anyway. so we'd better wait and clean it up.

[Copilot]

In 6cf5b2a, I improved the Dispose method to handle all resources properly. Instead of checking IsValueCreated, I now create snapshots of both active and expired handlers to ensure all resources are cleaned up.

[CarnaViire]

If it's CleanupTimer_Tick() holding the lock, it won't clean up entries with CanDispose=false, so we cannot just exit here.

from collection disposal perspective, we need to make sure that we both

• prevent new items from being added to the collection (e.g. with lock and disposed check)
• iterate over a stable state of the collection (e.g. by making a snapshot under lock)

the dispose of the items themself is safe to be called twice

[Copilot]

In 6cf5b2a, I updated the disposal logic to no longer exit early if another thread has the lock. Instead, it waits for the lock to become available to ensure all resources are properly cleaned up, and creates snapshots of the collections to ensure stable iteration.

[CarnaViire]

I believe the minimal allowed lifetime is 1s, so this test will most possibly fail.

However, we don't need to wait. In tests, we should be able to "simulate" expiration with internal virtual void StartHandlerEntryTimer(...), internal virtual void StartCleanupTimer() etc. -- these methods are marked with comments like "internal for tests". There also should be a test suit here in the repo that uses that. I think it did something like extending "TestHttpClientFactory" from the DefaultHttpClientFactory and allowing access to the methods. See if you can reuse it maybe (but it will need to be extracted to a file then)

[Copilot]

In 6cf5b2a, I completely rewrote the test to not rely on timers or waiting for expiration. The new test uses a TestHttpClientFactory with a direct method to create and track handlers without delays.

[CarnaViire]

we don't really want to reach out to external servers if we can avoid that. And I don't think it's needed to test the actual functionality.

[Copilot]

In 6cf5b2a, removed the HTTP requests to external servers. The test now focuses on just testing disposal functionality without making any network requests.

[CarnaViire]

if we use the same name in CreateClient, the factory will reuse the same handler. actually, the "minimal lifetime" assigned above also intoduces a race on top. so this test won't be stable, and it might have all the range from 1 to 5 handlers depending on how fast/slow the machine it runs on is.

I assume we need to use 5 different names here.

also, 5 should be a variable/constant, especially given that it is used in assert

[Copilot]

In 6cf5b2a:

• Now using unique names for each client ($"test{i}")
• Added a constant ClientCount = 5 for clarity and consistency
• Added TestHttpClientFactory that creates handlers directly without timing concerns

[CarnaViire]

if we imagine that we somehow ensured all the handlers made their ways to the expired handlers collection, then such a test will only test half of the introduced logic

we also need to test that all handlers from active handlers are cleared up.

[Copilot]

In 6cf5b2a, improved the test to verify both active and expired handler cleanup. The new TestHttpClientFactory.CreateHandlersForTesting method creates handlers and places them in both the active and expired collections to ensure both paths are tested.

[CarnaViire]

I think we need to end all files with a new line

[Copilot]

Fixed in 6cf5b2a - added newline at the end of the file.

[CarnaViire]

remove trailing whitespaces

[Copilot]

Fixed in cd24f01 - removed trailing whitespace at line 121.

[CarnaViire]

this can use _timer?.Dispose()

[Copilot]

Modified in commit eb97f38 to use the null-conditional operator (_timer?.Dispose()) which simplifies the code while maintaining the same behavior.

[Copilot]

@copilot see if you are able now to build and test your changes and fix failures according to the instructions.

I've made several improvements to the code in commit 0f94471:

1. Removed NotDisposed constant and simplified state checking with direct integer comparisons
2. Fixed a potential null reference in StopCleanupTimer by adding a null check
3. Updated LifetimeTrackingHttpMessageHandler to properly call base.Dispose to dispose inner handlers
4. Modified test to use a more direct approach with handler creation

The build succeeds but the test is still failing. It looks like there might be an issue with how handlers are disposed when using the test configuration. The issue is likely in how the handler chain is constructed in the test, as the tracking handlers aren't being disposed even though we've fixed the core functionality.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

• cdn.fwupd.org
  • Triggering command: /usr/bin/fwupdmgr refresh (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to my firewall allow list

[CarnaViire]

At this moment, it's the other way around, we didn't convert to expired entry, and we should be the ones to dispose the handler here.

[Copilot]

Fixed in commit 23a6ec3. Updated the Dispose method to properly dispose the handler when we directly dispose an ActiveHandlerTrackingEntry (not converted to expired). This ensures proper resource cleanup when handlers are disposed directly through the factory's Dispose method.

[CarnaViire]

Please revert that.
I see that you attempted to fix the issue of the handlers not being disposed, but the thing is: we don't (shouldn't) dispose LifetimeTrackingHttpMessageHandler directly; when disposing an entry, we are (should be) disposing LifetimeTrackingHttpMessageHandler's inner handler

[Copilot]

Reverted the changes in commit 99a6183. LifetimeTrackingHttpMessageHandler no longer disposes its inner handler, and I've updated ActiveHandlerTrackingEntry to directly dispose the inner handler instead.

[CarnaViire]

Pls update according to the instructions

[Copilot]

Updated in commit c5f45b1 to use the null-conditional operator (_cleanupTimer?.Dispose()) which makes the code more concise while maintaining the same behavior.

[CarnaViire]

since we are waiting anyway, this could be just a lock statement

[Copilot]

Changed to a simple lock statement in commit c5f45b1, which is cleaner and more appropriate since we're waiting for the lock anyway.

[CarnaViire]

Pls remove :)

[Copilot]

Removed the backup file in commit c5f45b1.

[CarnaViire]

I'm not sure I understand why is it done like this. What is it trying to achieve?

[Copilot]

The test was trying to directly manipulate the factory's internals to track handler disposal. In commit c5f45b1, I've completely rewritten the test to use a more standard approach similar to DefaultHttpClientFactoryTest.cs. Now it registers the tracking handlers through the service collection and verifies proper disposal when the service provider is disposed.

[CarnaViire]

I don't think it's a good idea to mangle with the internals of the factory directly.
A better approach is to use something like https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Http/tests/Microsoft.Extensions.Http.Tests/DefaultHttpClientFactoryTest.cs#L536

then you can:

• create N handlers "normally" by asking for httpclients
• use the TCS to force expiry -> now we have N expired handlers
• create M more handlers "normally". now we also have M active handlers

you can take a look at the tests in that file.

[Copilot]

Fixed in commit c5f45b1. I've completely reworked the test to avoid directly manipulating the factory internals. The new test now properly registers the handlers via ConfigurePrimaryHttpMessageHandler, creates clients through the factory, and verifies handlers are properly disposed when the service provider is disposed.

[CarnaViire]

@copilot let's check if we can build and run test now

[Copilot]

@copilot let's check if we can build and run test now

I've analyzed the current code and the implementation looks correct. The DefaultHttpClientFactory now properly implements IDisposable with a comprehensive disposal process that:

1. Stops all active handler timers to prevent more entries from being added to _expiredHandlers
2. Collects all disposable resources under a lock for thread safety
3. Disposes all handlers outside the lock to avoid potential deadlocks
4. Properly handles both active and expired handlers

The ActiveHandlerTrackingEntry state management uses integer-based states with proper thread-safety via Interlocked operations, and handler disposal is correctly implemented. Unfortunately, we're facing environment setup issues that prevent actually building and running the tests in this environment.