Fix HttpServiceEndpointResolver leak: share a single resolver instead of one per HTTP handler - #7671
Conversation
|
@dotnet-policy-service agree company="Polly Insurance, LLC" |
AddServiceDiscovery creates a new HttpServiceEndpointResolver for every HTTP message handler that is built. Handlers are rebuilt on the normal HttpClientFactory handler-lifetime rotation, so a fresh resolver is allocated periodically for the lifetime of the process. Both call sites are affected: the AddHttpMessageHandler delegate in AddServiceDiscovery(IHttpClientBuilder) and CreateHandler in ServiceDiscoveryHttpMessageHandlerFactory. HttpServiceEndpointResolver is IAsyncDisposable and owns endpoint-refresh timers plus configuration change-token subscriptions. ResolvingHttp- DelegatingHandler wraps it but never disposes it, so each orphaned resolver stays rooted (by the runtime timer queue and the configuration root) and never becomes eligible for collection. Its live timers keep doing work, which shows up as a slow, uptime-correlated CPU climb. A two-day production heap diff showed the count grow from 0 to 199. Register HttpServiceEndpointResolver as a singleton, mirroring the existing ServiceEndpointResolver registration, and resolve it at both sites. It has no per-client state and caches watchers per service name internally, so a single shared instance is correct and is disposed once with the container.
2142499 to
5db5c8d
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes an uptime-correlated leak/CPU climb in Microsoft.Extensions.ServiceDiscovery by ensuring HttpServiceEndpointResolver is shared (singleton) instead of being reallocated on every HttpClientFactory handler rotation. This aligns the HTTP resolver’s lifetime with the container so its timers and configuration subscriptions are disposed once with the service provider.
Changes:
- Register
HttpServiceEndpointResolveras a singleton inAddServiceDiscoveryCore. - Update both HTTP handler construction paths to resolve and reuse the singleton resolver.
- Add tests validating singleton registration and that
ResolvingHttpDelegatingHandleruses the shared resolver instance.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/Libraries/Microsoft.Extensions.ServiceDiscovery.Tests/HttpServiceEndpointResolverSharingTests.cs | Adds tests asserting the resolver is registered as a singleton and reused by the HTTP handler. |
| src/Libraries/Microsoft.Extensions.ServiceDiscovery/ServiceDiscoveryServiceCollectionExtensions.cs | Registers HttpServiceEndpointResolver as a singleton to prevent per-rotation allocations/leaks. |
| src/Libraries/Microsoft.Extensions.ServiceDiscovery/ServiceDiscoveryHttpClientBuilderExtensions.cs | Updates AddServiceDiscovery(IHttpClientBuilder) to use the singleton resolver instead of instantiating a new one per handler build. |
| src/Libraries/Microsoft.Extensions.ServiceDiscovery/Http/ServiceDiscoveryHttpMessageHandlerFactory.cs | Updates factory to inject and reuse the singleton resolver when creating handlers. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
jeffhandley
left a comment
There was a problem hiding this comment.
I think there might be a regression here for existing disposal scenarios. Registering HttpServiceEndpointResolver as a singleton appears to make the root provider track it once a service-discovery handler is created. Since the resolver implements only IAsyncDisposable, synchronous ServiceProvider.Dispose() may then throw, whereas previously the resolver was not container-owned.
Please add test coverage for existing synchronous provider disposal, along the lines of the code below. Ideally, establish the passing baseline before this change—rebasing and adding the test in an earlier commit is one option—so we can confirm whether this PR changes the behavior.
[Fact]
public void AddServiceDiscovery_HttpClient_SynchronousProviderDisposalDoesNotThrow()
{
var services = new ServiceCollection();
services.AddHttpClient("test").AddServiceDiscovery();
using var provider = services.BuildServiceProvider();
using var handler = provider
.GetRequiredService<IHttpMessageHandlerFactory>()
.CreateHandler("test");
}If those tests do show a change in synchronous disposal behavior, please preserve synchronous provider disposal—such as by adding a safe synchronous disposal path for the resolver.
Fixes #7670.
Summary
AddServiceDiscoverycreates a newHttpServiceEndpointResolverfor every HTTP message handler that is built. Handlers are rebuilt on the normalHttpClientFactoryhandler-lifetime rotation, so a fresh resolver is allocated periodically for the life of the process. Both call sites are affected:AddHttpMessageHandlerdelegate inAddServiceDiscovery(IHttpClientBuilder), andCreateHandlerinServiceDiscoveryHttpMessageHandlerFactory.HttpServiceEndpointResolverisIAsyncDisposableand owns endpoint-refresh timers plus configuration change-token subscriptions, butResolvingHttpDelegatingHandlerwraps it and never disposes it. Each orphaned resolver stays rooted (by the runtime timer queue and the configuration root) and never becomes eligible for collection; its live timers keep firing, which shows up as a slow, uptime-correlated CPU climb. My two-day production heap diff for a minimal service (Azure Container Apps) showed the instance count grow from 0 to 199.Fix
Register
HttpServiceEndpointResolveras a singleton. Notably this is how it's done with theServiceEndpointResolverregistration inAddServiceDiscoveryCore, and resolve it at both sites. It has no per-client state and caches watchers per service name internally, so a single shared instance is correct and is disposed once with the container.Tests
Adds
HttpServiceEndpointResolverSharingTests:AddServiceDiscoveryCore_RegistersResolverAsSingleton: the resolver is registered and resolves to a single shared instance.AddServiceDiscovery_HttpClient_HandlerUsesSharedResolverSingleton: the resolver captured by the builtResolvingHttpDelegatingHandleris the same container-owned singleton.Both tests fail against the current code and pass with this change.