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.

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?