Skip to content

Make HttpClientFactory implement IDisposable to clean up resources #115826

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 54 commits into
base: main
Choose a base branch
from

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented May 21, 2025

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:


💡 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 dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label May 21, 2025
@Copilot Copilot AI changed the title [WIP] HttpClientFactory doesn't clean up after itself Make HttpClientFactory implement IDisposable to clean up resources May 21, 2025
@Copilot Copilot AI requested a review from CarnaViire May 21, 2025 12:24
Copilot finished work on behalf of CarnaViire May 21, 2025 12:25
@CarnaViire CarnaViire added area-Extensions-HttpClientFactory and removed linkable-framework Issues associated with delivering a linker friendly framework labels May 21, 2025
Copy link
Contributor

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

Copilot finished work on behalf of CarnaViire May 21, 2025 17:33
Copilot finished work on behalf of CarnaViire May 21, 2025 18:27
Copy link
Contributor Author

Copilot AI commented May 27, 2025

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

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:

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

Copilot finished work on behalf of CarnaViire May 27, 2025 17:25
Copilot finished work on behalf of CarnaViire May 27, 2025 22:30
@CarnaViire
Copy link
Member

@copilot can you pls fix the races mentioned below:

1. Can Cleanup or Expiry Callbacks Run in Parallel With Dispose?

a. Cleanup Callback (CleanupTimer_Tick)

  • This is triggered by a Timer, which runs on ThreadPool and can be invoked at any time.
  • CleanupTimer_Tick uses Monitor.TryEnter(_cleanupActiveLock) to avoid concurrent cleanups, but it does NOT block for the lock—it simply skips if another cleanup is active.
  • If Dispose() starts while a cleanup is running, it will block and wait for the cleanup to finish because it uses lock (_cleanupActiveLock) (which is a blocking lock).
  • If CleanupTimer_Tick starts after Dispose() has entered the lock, it will fail to acquire the lock and exit early.

So: Dispose will wait for any currently-running cleanup to finish before proceeding.

b. Expiry Callback (ExpiryTimer_Tick)

  • This callback removes entries from _activeHandlers and enqueues expired handlers.
  • It does not take any locks except on the concurrent collections.
  • If Dispose() runs while an expiry callback is executing, there’s no explicit coordination; but since _activeHandlers and _expiredHandlers are concurrent collections, operations will be safe, though the expiry callback might run after Dispose() has cleared collections.

So: There is a risk that ExpiryTimer_Tick could enqueue an expired handler after Dispose has finished, resulting in a handler that’s never disposed.

2. Can the Cleanup Timer Be Recreated After Dispose Started?

  • Dispose() sets _disposed = true at the start.
  • All public API methods (including the timer callbacks) check _disposed and early-return if true.
  • However, StartCleanupTimer() can be called from the expiry callback, and it sets up the timer with:
    lock (_cleanupTimerLock)
    {
        _cleanupTimer ??= NonCapturingTimer.Create(_cleanupCallback, this, DefaultCleanupInterval, Timeout.InfiniteTimeSpan);
    }
  • If Dispose() is called and sets _disposed = true while an expiry callback runs, it is possible for StartCleanupTimer() to run after Dispose() has disposed the previous timer.
  • The newly created timer’s callback will check _disposed and do nothing, but the timer object will still be allocated, potentially leaking resources.

3. Is There a Race?

Yes, there are two races:

  • Expiry callback can enqueue expired handlers after Dispose has cleared the queue, causing resource leaks.
  • A cleanup timer can be recreated after Dispose begins, but its callback will be a no-op. The timer itself could linger.

Copy link
Contributor Author

Copilot AI commented Jul 24, 2025

@copilot can you pls fix the races mentioned below:

1. Can Cleanup or Expiry Callbacks Run in Parallel With Dispose?

...

Fixed the race conditions in commit 62c0678. The changes address both identified issues:

  1. ExpiryTimer_Tick race: Added _disposed checks to prevent enqueueing expired handlers after disposal starts
  2. StartCleanupTimer race: Added _disposed check to prevent timer recreation after disposal
  3. Enhanced CleanupTimer_Tick: Added additional _disposed checks for better thread safety

The implementation now ensures proper coordination between disposal and all timer callbacks, preventing the scenarios where handlers could be leaked or timers recreated after disposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HttpClientFactory doesn't clean up after itself
6 participants