Should IHttpClientBuilder.AddTypedClient<TClient> register the TClient as Scoped?

[dotnetjunkie]

The current implementation of the HttpClientBuilderExtensions.AddTypedClient<TClient> extension method of the Microsoft.Extensions.Http project registers the TClient with the Transient lifestyle. I think this is incorrect and would like drop this here for discussion.

With the introduction of ASP.NET Core v2.1, the new IHttpClientFactory interface and its related extension methods were added to address the problems with reuse of HttpClient instances, as discussed here, here, and here, as referenced by Steve Gordon in his blog post on this matter.

Problem being that HttpClients that are stored for the duration of the app domain can cause several issues, most obvious one being that a Singleton HttpClient doesn't respect DNS changes. This, of course, is exactly why the new extension infrastructure is put in place.

What strikes me, though, is that, although there is seemingly the observation that caching of HttpClient for the duration of the application is bad, AddTypedClient registers the HttpClient consumers as Transient, as can be seen in the AddTypedClient implementation:

public static IHttpClientBuilder AddTypedClient<TClient>(this IHttpClientBuilder builder)
    where TClient : class
{
    ...
    builder.Services.AddTransient<TClient>(s =>
    {
        var httpClientFactory = s.GetRequiredService<IHttpClientFactory>();
        var httpClient = httpClientFactory.CreateClient(builder.Name);
        var typedClientFactory = s.GetRequiredService<ITypedHttpClientFactory<TClient>>();
        return typedClientFactory.CreateClient(httpClient);
    });

    return builder;
}

The HttpClientFactory sample project shows a usage of this method:

services.AddHttpClient("github", c =>
  {
      c.BaseAddress = new Uri("https://api.github.com/");
  })
  .AddHttpMessageHandler(() => new RetryHandler())
  .AddTypedClient<GitHubClient>();//GitHubClient depends on HttpClient and gets the "github" client

services.AddSingleton<IMyService, MyService>(); // Depends on GitHubClient

I think the registration of Transient consumers is problematic, as Transient—in term of Microsoft.Extensions.DependencyInjection—means stateless; not short lived. This means that the container allows those stateless consumers of HttpClient to be injected into Singleton consumers without the container's Scope Verfication feature to get tripped. For instance, in the case of the Singleton MyService registration shown above, it will keep the GitHubClient alive for the duration of the application, and GitHubClient will keep its HttpClient alive for the duration of the application. This, as stated above, could lead to missing DNS changes, effectively causing the application to become corrupted in a way that can only be fixed with a restart of the application.

The injection of HttpClient instances into Singleton consumers could, therefore, lead to the type of bugs that you so hard tried to prevent using this new design. Because of the way AddHttpClient is designed, no warning signs will be given. Because of the way DNS caching works, problems only appear after the application is deployed to production; hardly ever on the developer's machine, making it crucial to warn developers in an early stage about this issue.

I think the solution to this problem is, therefore, rather straightforward (although, admittedly, a breaking change). Change the AddTypedClient method to change its TClient as Scoped instead, because in that case, MS.DI's Scope Validation feature will prevent the client to be resolved from a root scope:

public static IHttpClientBuilder AddTypedClient<TClient>(this IHttpClientBuilder builder)
    where TClient : class
{
    // Register client as Scoped, allowing Captive Dependencies being detected in case the
    // ServiceProvider is built using the validateScopes flag.
    builder.Services.AddScoped<TClient>(s =>
    {
        // same code as before
    });

    return builder;
}

Although changing the lifestyle from Transient to Scoped can be considered to be a breaking change, due to how Scope Validation is configured in ASP.NET Core applications, exceptions will only be thrown in development mode; not when the application is deployed.

[nicklundin08]

Rather than registering http clients as scoped, I think you would want to depend on IHttpClientFactory everywhere (including typed clients) and then you have the option register your typed clients in singleton or transient scope.

If you do not depend on IHttpClientFactory in your typed client, you have the chance of hanging onto the http client for the lifetime of whatever depends on the typed client.

One example of this could be pages in a mobile app. If your navigation framework re-uses page instances, and you depend directly on an http client, then you effectively have an http client that lasts the lifetime of the application.

This could result in the client not getting dns updates

Furthermore, if you have an application that where you need to swap between base urls at runtime via a settings page or something like that (a situation that I have seen while working on internal tools), then if you would have to restart the entire application every time you change base urls

In this scenario we had to roll our own TypedClientFactory and depend on that instead of the typed client directly. While this got us over the hurdle, I think a better solution would be for typed clients (especially typed client libraries like Refit/RestLess) to depend on the IHttpClientFactory

I imagine something like this for Refit

public static IHttpClientBuilder AddTransientRefitClient<T>(this IServiceCollection services,
    RefitSettings settings = null)
{
    var builder = services.AddHttpClient<T>();

    services.AddTransient<T>(serviceProvider => 
        Refit.RestService.For<T>(serviceProvider.GetRequiredServics<IHttpClientFactory>(), settings);

    return builder;
}

public static IHttpClientBuilder AddSingletonRefitClient<T>(this IServiceCollection services,
    RefitSettings settings = null)
{
    var builder = services.AddHttpClient<T>();

    services.AddSingleton<T>(serviceProvider => 
        Refit.RestService.For<T>(serviceProvider.GetRequiredServics<IHttpClientFactory>(), settings);

    return builder;
}

@dotnetjunkie what are your thoughts?

[dotnetjunkie]

@nicklundin08, not much to add. I think it is a good idea to inject an IHttpClientFactory instead of depending on HttpClient. Considering all its quirks, the HttpClient seems more like runtime data anyway.

But as Microsoft decided to introduce this AddTypedClient extension method and build their advice on using HttpClient around that method, we don't have to expect such extension method to be obsoleted soon. As long as that method is not deprecated, it is important, IMO, for Microsoft to fix this design flaw, even though, admittedly, this fix is a breaking change.

[ItsVeryWindy]

One thing I'd like to add is that as they are scoped as transient, scope validation won't pick it up and alert you.

https://docs.microsoft.com/en-us/aspnet/core/fundamentals/dependency-injection?view=aspnetcore-2.2#scope-validation

[rynowak]

We'll consider this during 5.0. We didn't put a ton of thought into whether this should be transient or scoped initially, and picked transient because it seemed "safe". It's clear that there's more inputs into this equation than we considered in the first draft.

[jan-johansson-mr]

Hmmm... I'm using typed HttpClient in my Blazor server app. I want my type to be tied to the Blazor circuit and thus it would be great if the instance could be scoped rather than transient (to maintain state as long as the circuit is alive). However, there are tons of workarounds if it can't be done due to some technical issue.

[ghost]

Tagging subscribers to this area: @dotnet/ncl Notify danmosemsft if you want to be subscribed.

[ericstj]

Given this fix is breaking I'm reluctant to take it this late in 5.0. Let's consider this for 6.0. One way to make it non-breaking is to add a new method that does the scoped registration, AddTypedClientScoped. @davidfowl @maryamariyan @dotnet/ncl any thoughts on this?

[halter73]

I agree this is too risky to take for 5.0. Anyone who was resolving a typed client from a the root provider will be broken.

For instance, in the case of the Singleton MyService registration shown above, it will keep the GitHubClient alive for the duration of the application, and GitHubClient will keep its HttpClient alive for the duration of the application.

Yep. And this is the scenario that would break if we make the typed client registrations scoped. What's wrong with a Signleton service keeping an HttpClient alive for the duration of the application? The IHttpClientFactory which owns all the HttpMessageHandlers and therefore the connection pool is already a singleton.

I don't think anyone mentioned IDisposable in this thread yet, but I wonder if that's really what's at the heart of this issue. Resolving transient services that happen to implement IDisposable from root provider is very problematic, and a typed client could implement IDisposable which could lead to problems where the container itself, not the singleton service, keeps the typed client alive for the duration of the application, but that's not unique to typed clients. See #36491.

One way to make it non-breaking is to add a new method that does the scoped registration, AddTypedClientScoped.

I think this would just create more confusion over which method to call. I don't see the problem with typed clients being transient unless the type implements IDisposable. Not implementing IDisposable is the easy way to work around leaking the typed clients. If there's some reason the typed client needs to be disposed at the end of the request/scope, manually registering it as a scoped service should be easy enough even without a new extension method.

[EnricoMassone]

@halter73 just a question. Above you said:

What's wrong with a Signleton service keeping an HttpClient alive for the duration of the application?

Based on my understanding, having a captive HttpClient keeped alive for the entire duration of the application could lead to missing reaction to DNS updates, as pointed out above by @nicklundin08 and @dotnetjunkie.

This seems to be confirmed by this Micorsoft docs on IHttpClientFactory.

My understanding is that the HttpMessageHandler contained in the captive HttpClient is kept alive for the entire application lifetime, which causes the DNS change issue. Put in other words, the whole HTTP client story works fine if and only if each time a consumer needs an HttpClient instance it goes to the IHttpClientFactory and asks for a new instance. At that point, the factory is able to pick up an HttpMessageHandler instance from its pool and to return a new HttpClient using the pooled handler. The pooled handlers have a limited lifetime (2 minutes by default if I remember correctly), so that the DNS change issue is avoided.

Is my understanding correct ?

[wfurt]

As far as I can tell, the HttpClient will not pro-actively pick up new nodes resolving to same name as far as there are available connections. But it does not cache any DNS lookups e.g. when time comes to create new connection, it will do new DNS lookup (subject to OS caching) and it will connect to some IP returned by the lookup. You can use PooledConnectionLifetime to force discovery of new nodes. It seems like the problem with DNS is valid only if given server stops serving content for given name without closing the connection, right?

[EnricoMassone]

As far as I can tell, the HttpClient will not pro-actively pick up new nodes resolving to same name as far as there are available connections. But it does not cache any DNS lookups e.g. when time comes to create new connection, it will do new DNS lookup (subject to OS caching) and it will connect to some IP returned by the lookup. You can use PooledConnectionLifetime to force discovery of new nodes. It seems like the problem with DNS is valid only if given server stops serving content for given name without closing the connection, right?

Hi,

I don't really know the underlying implementation details. My high level understanding is the following:

• the HttpClient delegates the HTTP request handling to the wrapped HttpMessageHandler
• the connections are handled at the HttpMessageHandler level
• HttpMessageHandler must be pooled because creating an arbitrary amount of them could lead to socket exhaustion under heavy loads
• the HttpMessageHandler, by default, keeps the connections open indefinitely and this is the root cause of the DNS change issue

This documentation seems to confirm that, by default, HttpMessageHandler implementations keeps the connections open indefinitely, so that they are not able to react to DNS changes:

Pooling of handlers is desirable as each handler typically manages its own underlying HTTP connections; creating more handlers than necessary can result in connection delays. Some handlers also keep connections open indefinitely, which can prevent the handler from reacting to DNS changes

So basically the issue is related to connections not being closed, as you pointed out above.

[CarnaViire]

Triage: 1) We should not change the current lifetime as it will be a breaking change 2) We shouldn't add a new method for scoped registration because there's not enough benefit in doing it that would justify extending API surface 3) we should update documentation with two messages:

• specify that the container will track IDisposable transient services so it is advised not to make your typed clients IDisposable, or you should dispose them yourself
• note that HttpClient instances will hold the connection indefinitely (unless you specify the primary handler to be SocketsHttpHandler with PooledConnectionLifetime - applies to .NET Core). If you want to leverage HttpClientFactory recreating the connection, you would need to ask for a new instance of HttpClient (and also your Typed client)

[AroglDarthu]

4. How about just deprecating the existing AddTypedClient and adding a new extension method that takes an additional, non-optional parameter to specify the desired lifetime? Do this in v6 and drop the original in v7.

This issue pre-dates v5 by nearly two years and now it is being delayed to after v6?! Seriously?

[karelz]

@AroglDarthu

This issue pre-dates v5 by nearly two years and now it is being delayed to after v6?! Seriously?

Yes, it is old issue and we don't plan to prioritize it for 6.0 from these reasons:

• The issue is fairly complicated.
• It does not block too many customers.
• The scenarios where it matters are not super clear yet (something to follow up on here).
• There seem to be workarounds.
• Solution does not seem to be super-clear yet.
• Our team (NCL) is still ramping up on the component (which was transferred to us recently) together with learning about DI for the first time, so expertise is limited (especially on edge cases).
• There are 500+ other Networking issues to compete for our investment.

If the problem was more impactful, or easier to solve, we would be happy to prioritize it for 6.0. Currently that is not the case.

[CarnaViire]

I admit that it is not great that injecting a Typed client into a singleton is not checked automatically and it may result in a situation with loss of DNS changes. It also make things worse that it is not obvious which lifetime you will get when you register a Typed client via AddHttpClient method (without looking into docs of course) so it is difficult to be aware of lifetimes when injecting a Typed client.

However, if there are other unsolvable pains that we did not consider, please let us know. @AroglDarthu what exactly is the problem that you would try to solve by registering a Typed client as scoped?

JFYI. There are 16 overloads for registering a Typed client. Adding 16 more is A LOT. We would need a really good justification for that.

P.S.: HttpClients produced by HttpClientFactory are transient-like objects, so for me it actually seems reasonable for Typed clients - that receive an instance of HttpClient in constructor - to have same lifetime as HttpClient.

[dotnetjunkie]

Our team (NCL) is still ... learning about DI for the first time, so expertise is limited

This is very honest... but still quite disturbing.

[davidfowl]

I think generally we need to deprecate/remove handler rotation from the IHttpClientFactory. Refreshing DNS should be left up to the innermost handler implementation. That would simplify the lifetime semantics of the client factory and remove some of the confusion around when to use it.

[CarnaViire]

@davidfowl That's right and we are planning to do that. However there's still a case when a custom PrimaryHandler is supplied - and we might still want to leave rotation for these ones. We may have a discussion in https://github.com/dotnet/runtime/issues/35987 if you wish.