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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 45 commits into
base: main
Choose a base branch
from
Draft

Make HttpClientFactory implement IDisposable to clean up resources #115826

wants to merge 45 commits into from
+533 −17

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented May 21, 2025 •
edited
Loading

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
@CarnaViire CarnaViire added area-Extensions-HttpClientFactory and removed linkable-framework Issues associated with delivering a linker friendly framework labels May 21, 2025
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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

@CarnaViire CarnaViire mentioned this pull request May 21, 2025
Merged
@build-analysis build-analysis bot mentioned this pull request May 21, 2025
Open
CarnaViire
CarnaViire requested changes May 21, 2025
View reviewed changes
This was referenced May 23, 2025
Open
Open
@calinpristavu

This comment was marked as off-topic.

Sign in to view
MihaZupan
MihaZupan reviewed May 23, 2025
View reviewed changes
CarnaViire
CarnaViire reviewed May 23, 2025
View reviewed changes
@CarnaViire
Copy link
Member

@copilot let's try again

@Copilot Copilot SWE Agent
Copy link
Contributor Author

Copilot AI commented May 23, 2025

@copilot let's try again

I've addressed all the code review feedback in commit 9c60237:

  1. Simplified the condition check in ActiveHandlerTrackingEntry.Dispose() to use != 0 directly
  2. Removed the redundant IsValueCreated check and used a variable for entry.Value.Value in DefaultHttpClientFactory
  3. Restructured ExpiredHandlerTrackingEntry.Dispose() to ensure handler and scope are disposed together with proper try-finally blocks

The code builds successfully and is ready for further review.

@CarnaViire
Copy link
Member

@copilot there was a test error reported in CI:

  Discovering: Microsoft.Extensions.Http.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  Microsoft.Extensions.Http.Tests (found 134 test cases)
  Starting:    Microsoft.Extensions.Http.Tests (parallel test collections = on [2 threads], stop on fail = off)
    Microsoft.Extensions.Http.Tests.HttpClientFactoryDisposeTests.DisposingServiceProvider_DisposesHttpClientFactory_ReleasesResources [FAIL]
      Assert.Equal() Failure: Values differ
      Expected: 5
      Actual:   1
      Stack Trace:
        /_/src/libraries/Microsoft.Extensions.Http/tests/Microsoft.Extensions.Http.Tests/HttpClientFactoryDisposeTests.cs(45,0): at Microsoft.Extensions.Http.Tests.HttpClientFactoryDisposeTests.DisposingServiceProvider_DisposesHttpClientFactory_ReleasesResources()
        --- End of stack trace from previous location ---
    ...
=== TEST EXECUTION SUMMARY ===
   Microsoft.Extensions.Http.Tests  Total: 185, Errors: 0, Failed: 1, Skipped: 3, Time: 2.280s
/root/helix/work/workitem/e
----- end Fri May 23 22:11:21 UTC 2025 ----- exit code 1 ----------------------------------------------------------

@Copilot Copilot SWE Agent
Copy link
Contributor Author

Copilot AI commented May 23, 2025

@copilot there was a test error reported in CI:

...

Fixed in commit b0216e5. The test was failing because all clients were being created with the same name "test-client", which causes the factory to reuse the same handler for all clients. I've updated the test to create clients with unique names, ensuring we get 5 distinct handlers that are properly tracked and disposed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CarnaViire CarnaViire CarnaViire requested changes

@MihaZupan MihaZupan MihaZupan left review comments

@crhaglun crhaglun crhaglun left review comments

@jeffhandley jeffhandley Awaiting requested review from jeffhandley jeffhandley will be requested when the pull request is marked ready for review jeffhandley is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@CarnaViire CarnaViire

Copilot code review Copilot

Labels
area-Extensions-HttpClientFactory
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HttpClientFactory doesn't clean up after itself
6 participants
@Copilot @CarnaViire @kasthack @calinpristavu @MihaZupan @crhaglun