Skip to content
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

Add API to provide existing DI scope to HttpClientFactory #47091

Open
CarnaViire opened this issue Jan 17, 2021 · 23 comments
Open

Add API to provide existing DI scope to HttpClientFactory #47091

CarnaViire opened this issue Jan 17, 2021 · 23 comments
Assignees
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-HttpClientFactory
Milestone

Comments

@CarnaViire
Copy link
Member

CarnaViire commented Jan 17, 2021

Updated

The scope can be provided via the keyed services infra. The API to opt-in into Keyed services registration is proposed in #89755.

AddAsKeyedScoped() API will automatically opt in into the scope-propagating behavior, but ONLY in case keyed services infra ([FromKeyedServices...] or GetRequiredKeyedService) is used to inject/resolve the client.

Opt-in API design considerations -- separate from keyed services
namespace Microsoft.Extensions.DependencyInjection;

public static partial class HttpClientBuilderExtensions
{
    public static IHttpClientBuilder SetPropagateContextScope(this IHttpClientBuilder builder, bool val) {}
}

Usage:

services.AddHttpClient("foo")
    //.AddAsKeyedScoped()
    .SetPropagateContextScope(true);

services.AddHttpClient("foo")
    //.AddAsKeyedTransient()
    .SetPropagateContextScope(true); // if there are any scoped dependencies, won't resolve in singletons

services.AddHttpClient("foo")
    .SetPropagateContextScope(false); // explicitly opt-out

Alternalive namings:

  • PropagateScope
  • PropagateExistingScope
  • SetPreserveExistingScope(true/false)
  • SetSuppressHandlerScope(true/false) (there is existing "hidden" option with that name, but the usage is a bit different, so technically it can clash with existing usages + and not self-evident name)

Original proposal

Background and Motivation

HttpClientFactory allows users to register one or several HttpClient configurations in DI container and then instantiate HttpClients according to the respective configuration. A configuration can specify that HttpClient should use a specific HttpMessageHandler or even a chain of such handlers. When creating a client, HttpClientFactory caches and reuses HttpMessageHandlers to avoid creating too many connections and exhausting sockets, so handlers will live for a configurable timespan HandlerLifetime.

The problem begins when message handlers forming a chain have dependencies on other services from DI. In case the user wants to inject a scoped service into the message handler, they expect the scoped service instance to be from their existing unit-of-work scope. However, the current behavior is different -- in the existing implementation, the service instance will be from a new scope bound to message handler lifetime, i.e. it will be a different instance from what the user would expect.

This scope mismatch is not only confusing to customers, but also produces unsolvable bugs in user code, e.g. when the scoped service is supposed to be stateful within the scope, but this state is impossible to access from the message handler.

There is a number of GH issues and StackOverflow questions from users suffering from scope mismatch:

The solution leverages the following idea:

If we want to cache/reuse the connection, it is enough to cache/reuse the bottom-most handler of the chain (aka PrimaryHandler). Other handlers in the chain may be re-instantiated for each unit-of-work scope, so they will have the correct instances of the scoped services injected into them (which is desired by customers).

I believe new behavior should be opt-in, as there will be more allocations than before.

However, in order to leverage existing scope, HttpClientFactory should know about it. In the current implementation, the factory is registered in DI a singleton, so it doesn't have access to scopes.

The easiest way to allow HttpClientFactory to capture existing scope is to change its lifetime from singleton to transient. Transient services can (as well as singletons) be injected into services of all lifetimes, so all existing code will continue to work.

To maintain existing caching behavior, cache part of the factory will be moved out to a separate singleton service, but this is an implementation detail that does not affect API.

Proposed API

namespace Microsoft.Extensions.DependencyInjection
{
    public static partial class HttpClientBuilderExtensions
    {
        ...
        public static IHttpClientBuilder SetHandlerLifetime(this IHttpClientBuilder builder, TimeSpan handlerLifetime) { ... }
+       public static IHttpClientBuilder SetPreserveExistingScope(this IHttpClientBuilder builder, bool preserveExistingScope) { ... }
    }
}

namespace Microsoft.Extensions.Http
{
    public partial class HttpClientFactoryOptions
    {
        ...
        public TimeSpan HandlerLifetime { get; set; }
+       public bool PreserveExistingScope { get; set; } // default is false = old behavior
    }
}

namespace System.Net.Http
{
-   // registered in DI as singleton
+   // registered in DI as transient
    public partial interface IHttpClientFactory
    {
        HttpClient CreateClient(string name);
    }
}

Usage Examples

The only change needed for both named and typed clients is to opt-in via callling SetPreserveExistingScope(true)

Named client example:

// registration
class Program
{
    private static void ConfigureServices(HostBuilderContext context, IServiceCollection services)
    {
        services.AddScoped<IWorker, NamedClientWorker>();
        services.AddScoped<HandlerWithScopedDependency>();
        services.AddHttpClient("github")
            .AddHttpMessageHandler<HandlerWithScopedDependency>()
+           .SetPreserveExistingScope(true);
        ...
    }
    ...
}

// usage
class NamedClientWorker : IWorker
{
   private IHttpClientFactory _clientFactory;

+   // HttpClientFactory will capture an existing scope where it was resolved
    public NamedClientWorker(IHttpClientFactory clientFactory)
    {
        _clientFactory = clientFactory;
    }

    public async Task DoWorkAsync()
    {
+       // HandlerWithScopedDependency inside HttpClient will be resolved
+       // within an existing scope 
        HttpClient client = _clientFactory.CreateClient("github");
        var response = await client.GetStringAsync(GetRepositoriesUrl(username));
        ...
    }
}

Typed client example:

// registration
class Program
{
    private static void ConfigureServices(HostBuilderContext context, IServiceCollection services)
    {
        services.AddScoped<IWorker, TypedClientWorker>();
        services.AddScoped<HandlerWithScopedDependency>();
        services.AddHttpClient<IGithubClient, GithubClient>
            .AddHttpMessageHandler<HandlerWithScopedDependency>()
+           .SetPreserveExistingScope(true);
        ...
    }
    ...
}

// usage
class TypedClientWorker : IWorker
{
    private IGithubClient _githubClient;

    public TypedClientWorker(IGithubClient githubClient)
    {
        _githubClient = githubClient;
    }

    public async Task DoWorkAsync()
    {
        var response = await _githubClient.GetRepositories(username);
        ...
    }
}

// typed client impl
class GithubClient : IGithubClient
{
    private HttpClient _client;

    // HttpClient is created by IHttpClientFactory  
+   // HttpClientFactory will capture an existing scope
+   // HandlerWithScopedDependency inside HttpClient will be resolved
+   // within an existing scope 
    public GithubClient(HttpClient client)
    {
        _client = client;
    }

    public async Task<string> GetRepositories(string username)
    {
        return await _client.GetStringAsync(GetRepositoriesUrl(username));
    }
}

Alternative Designs

If we don't want to change the current lifetime of HttpClientFactory (i.e. let it stay singleton), we should provide the scope to it in some other way.

In order to do that, we may have an additional scoped service, which will have access to the current unit-of-work scope and to the singleton HttpClientFactory.

Let me note that because of how DI works, we couldn't use existing interface IHttpClientFactory for a new scoped service, because a singleton service is already registered on it. That's why a new interface IScopedHttpClientFactory is added here.

namespace Microsoft.Extensions.DependencyInjection
{
    public static partial class HttpClientBuilderExtensions
    {
        ...
        public static IHttpClientBuilder SetHandlerLifetime(this IHttpClientBuilder builder, TimeSpan handlerLifetime) { ... }
+       public static IHttpClientBuilder SetPreserveExistingScope(this IHttpClientBuilder builder, bool preserveExistingScope) { ... }
    }
}

namespace Microsoft.Extensions.Http
{
    public partial class HttpClientFactoryOptions
    {
        ...
        public TimeSpan HandlerLifetime { get; set; }
+       public bool PreserveExistingScope { get; set; } // default is false = old behavior
    }
}

namespace System.Net.Http
{
    // registered in DI as singleton
    public partial interface IHttpClientFactory
    {
        HttpClient CreateClient(string name);
    }

+   // registered in DI as scoped
+   public partial interface IScopedHttpClientFactory
+   {
+       HttpClient CreateClient(string name);
+   }
}

Alternative design's usage examples:

For named clients, user will also need to change the injected factory after opt-in. For typed clients, just opting-in is enough, the magic will happen on its own.

Named client example:

// registration
class Program
{
    private static void ConfigureServices(HostBuilderContext context, IServiceCollection services)
    {
        services.AddScoped<IWorker, NamedClientWorker>();
        services.AddScoped<HandlerWithScopedDependency>();
        services.AddHttpClient("github")
            .AddHttpMessageHandler<HandlerWithScopedDependency>()
+           .SetPreserveExistingScope(true);
        ...
    }
    ...
}

// usage
class NamedClientWorker : IWorker
{
-   private IHttpClientFactory _clientFactory;
+   private IScopedHttpClientFactory _clientFactory;

    public NamedClientWorker(
-       IHttpClientFactory clientFactory)
+       IScopedHttpClientFactory clientFactory)
    {
        _clientFactory = clientFactory;
    }

    public async Task DoWorkAsync()
    {
        HttpClient client = _clientFactory.CreateClient("github");
        var response = await client.GetStringAsync(GetRepositoriesUrl(username));
        ...
    }
}

Typed client example:

// registration
class Program
{
    private static void ConfigureServices(HostBuilderContext context, IServiceCollection services)
    {
        services.AddScoped<IWorker, TypedClientWorker>();
        services.AddScoped<HandlerWithScopedDependency>();
        services.AddHttpClient<IGithubClient, GithubClient>
            .AddHttpMessageHandler<HandlerWithScopedDependency>()
+           .SetPreserveExistingScope(true);
        ...
    }
    ...
}

// usage
class TypedClientWorker : IWorker
{
    private IGithubClient _githubClient;

    public TypedClientWorker(IGithubClient githubClient)
    {
        _githubClient = githubClient;
    }

    public async Task DoWorkAsync()
    {
        var response = await _githubClient.GetRepositories(username);
        ...
    }
}

// typed client impl
class GithubClient : IGithubClient
{
    private HttpClient _client;

-   // HttpClient is created by IHttpClientFactory 
+   // HttpClient is automatically created by IScopedHttpClientFactory after opt-in
    public GithubClient(HttpClient client)
    {
        _client = client;
    }

    public async Task<string> GetRepositories(string username)
    {
        return await _client.GetStringAsync(GetRepositoriesUrl(username));
    }
}

Risks

For existing usages - the risk is low. Transient HttpClientFactory can be injected in all service lifetimes as well as a singleton, so all existing code will continue to work as before and will maintain old behavior for creating HttpMessageHandlers. The only thing that will change is that there will be more allocations (every injection of HttpClientFactory will create a new instance).

New opt-in behavior is only meaningful within a scope, so HttpClientFactory should be resolved within a scope for PreserveExistingScope=true to work. However, no need to add any additional checks, this will be checked by DI's Scope Validation.

Substituting or modifying PrimaryHandler in case of PreserveExistingScope=true will be currently forbidden (InvalidOperationException during DI configuration). This is due to inability to assess security risks and avoid unnesessary object creations. HttpMessageHandlerBuilder.Build() will be called for each scope and not once in a (primary) handler lifetime as before. If it contains substituting or modifying PrimaryHandler, it will not work as expected, but will produce potential security risk and impact performance by creating redundant PrimaryHandlers to be thrown away. Addressing the risks of allowing PrimaryHandler modification will require additional API change.


2021-02-11 Edit: Changed main proposal to be about transient HttpClientFactory. Moved IScopedHttpClientFactory to alternatives. Removed option with IServiceProvider completely as it is both a bad practice and inconvenient to use.

2021-01-21 Edit: I've removed IScopedHttpMessageHandlerFactory from the proposal. It was initially added to correlate with IHttpMessageHandlerFactory, but actual usage examples where only scoped message handler would be needed but not HttpClient are not clear. It can be easily added later if there will be any demand for that.

@CarnaViire CarnaViire added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-HttpClientFactory labels Jan 17, 2021
@ghost
Copy link

ghost commented Jan 17, 2021

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

Issue Details

Background and Motivation

HttpClientFactory allows users to register one or several HttpClient configurations in DI container and then instantiate HttpClients according to the respective configuration. A configuration can specify that HttpClient should use a specific HttpMessageHandler or even a chain of such handlers. When creating a client, HttpClientFactory caches and reuses HttpMessageHandlers to avoid creating too many connections and exhausting sockets, so handlers will live for a configurable timespan HandlerLifetime.

The problem begins when message handlers forming a chain have dependencies on other services from DI. In case the user wants to inject a scoped service into the message handler, they expect the scoped service instance to be from their existing unit-of-work scope. However, the current behavior is different -- in the existing implementation, the service instance will be from a new scope bound to message handler lifetime, i.e. it will be a different instance from what the user would expect.

This scope mismatch is not only confusing to customers, but also produces unsolvable bugs in user code, e.g. when the scoped service is supposed to be stateful within the scope, but this state is impossible to access from the message handler.

There is a number of GH issues and StackOverflow questions from users suffering from scope mismatch:

The solution leverages the following idea:

If we want to cache/reuse the connection, it is enough to cache/reuse the bottom-most handler of the chain (aka PrimaryHandler). Other handlers in the chain may be re-instantiated for each unit-of-work scope, so they will have the correct instances of the scoped services injected into them (which is desired by customers).

I believe new behavior should be opt-in, as there will be more allocations than before.

However, in order to leverage existing scope, it should be passed to HttpClientFactory from outside. The factory itself is a singleton, so it doesn't have access to scopes.

In order to do that, we may have an additional scoped service, which will have access to the current unit-of-work scope and to the singleton HttpClientFactory (which is proposed below). The alternative could be to make users provide the specific scoped IServiceProvider to the factory themselves.

Proposed API

namespace Microsoft.Extensions.DependencyInjection
{
    public static partial class HttpClientBuilderExtensions
    {
        ...
        public static IHttpClientBuilder SetHandlerLifetime(this IHttpClientBuilder builder, TimeSpan handlerLifetime) { ... }
+       public static IHttpClientBuilder SetPreserveExistingScope(this IHttpClientBuilder builder, bool preserveExistingScope) { ... }
    }
}

namespace Microsoft.Extensions.Http
{
    public partial class HttpClientFactoryOptions
    {
        ...
        public TimeSpan HandlerLifetime { get; set; }
+       public bool PreserveExistingScope { get; set; } // default is false = old behavior
    }
}

namespace System.Net.Http
{
    // registered in DI as singleton
    public partial interface IHttpClientFactory
    {
        HttpClient CreateClient(string name);
    }

    // registered in DI as singleton
    public partial interface IHttpMessageHandlerFactory
    {
        HttpMessageHandler CreateHandler(string name);
    }

+   // registered in DI as scoped
+   public partial interface IScopedHttpClientFactory
+   {
+       HttpClient CreateClient(string name);
+   }

+   // registered in DI as scoped
+   public partial interface IScopedHttpMessageHandlerFactory
+   {
+       HttpMessageHandler CreateHandler(string name);
+   }
}

Usage Examples

For named clients, user will also need to change the injected factory after opt-in. For typed clients, just opting-in is enough, the magic will happen on its own.

// registration
class Program
{
    private static void ConfigureServices(HostBuilderContext context, IServiceCollection services)
    {
        services.AddScoped<HandlerWithScopedDependency>();

        // named client
        services.AddHttpClient("github")
            .AddHttpMessageHandler<HandlerWithScopedDependency>()
+           .SetPreserveExistingScope(true);

        // typed client
        services.AddHttpClient<IGithubClient, GithubClient>
            .AddHttpMessageHandler<HandlerWithScopedDependency>()
+           .SetPreserveExistingScope(true);
        ...
    }
    ...
}

// usage
class Worker : IWorker
{
-   private IHttpClientFactory _clientFactory;
+   private IScopedHttpClientFactory _clientFactory;
    private IGithubClient _githubClient;

    public Worker(
-       IHttpClientFactory clientFactory,
+       IScopedHttpClientFactory clientFactory,
        IGithubClient githubClient)
    {
        _clientFactory = clientFactory;
        _githubClient = githubClient;
    }

    public async Task DoWorkWithNamedClientAsync()
    {
        HttpClient client = _clientFactory.CreateClient("github");
        var response = await client.GetStringAsync(GetRepositoriesUrl(username));
        ...
    }

    public async Task DoWorkWithTypedClientAsync()
    {
        var response = await _githubClient.GetRepositories(username);
        ...
    }
}

// typed client impl
class GithubClient : IGithubClient
{
    private HttpClient _client;

    // client is created by IScopedHttpClientFactory after opt-in
    public GithubClient(HttpClient client)
    {
        _client = client;
    }

    public async Task<string> GetRepositories(string username)
    {
        return await _client.GetStringAsync(GetRepositoriesUrl(username));
    }
}

Alternative Designs

The alternative could be to make users provide the specific scoped IServiceProvider to the factory themselves. This however will require them to inject IServiceProvider to every service they use the factory in. In my opinion, this is a bit messy and will dangerously tempt users to leverage a service locator antipattern.

namespace System.Net.Http
{
    public partial interface IHttpClientFactory
    {
        HttpClient CreateClient(string name);
+       HttpClient CreateClient(string name, IServiceProvider serviceProvider);
    }

    public partial interface IHttpMessageHandlerFactory
    {
        HttpMessageHandler CreateHandler(string name);
+       HttpMessageHandler CreateHandler(string name, IServiceProvider serviceProvider);
    }
}

Usage example:

class Worker : IWorker
{
+   private IServiceProvider _serviceProvider;
    private IHttpClientFactory _clientFactory;

    public Worker(
+       IServiceProvider serviceProvider,
        IHttpClientFactory clientFactory)
    {
+       _serviceProvider = serviceProvider;
        _clientFactory = clientFactory;
    }

    public async Task DoWorkWithNamedClientAsync()
    {
-       HttpClient client = _clientFactory.CreateClient("github");
+       HttpClient client = _clientFactory.CreateClient("github", _serviceProvider);
        var response = await client.GetStringAsync(GetRepositoriesUrl(username));
        ...
    }
}

I also want to mention that I did not consider changing HttpClientFactory's own lifetime (e.g. from singleton to scoped) as it will be too much of a breaking change, e.g. scoped services cannot be injected into singletons, but other singletons can, so if current singleton factory was injected into a singleton somewhere, it will break.

Risks

For existing usages - the risk is low, because new behavior is opt-in. Existing usages will maintain old behavior.

Using IHttpClientFactory with PreserveExistingScope=true, as well as using IScopedHttpClientFactory with PreserveExistingScope=false will produce an InvalidOperationException to avoid confusing behavior (e.g. no scope to preserve in singleton IHttpClientFactory).

Substituting or modifying PrimaryHandler in case of PreserveExistingScope=true will be currently forbidden (InvalidOperationException during DI configuration). This is due to inability to assess security risks and avoid unnesessary object creations. HttpMessageHandlerBuilder.Build() will be called for each scope and not once in a (primary) handler lifetime as before. If it contains substituting or modifying PrimaryHandler, it will not work as expected, but will produce potential security risk and impact performance by creating redundant PrimaryHandlers to be thrown away. Addressing the risks of allowing PrimaryHandler modification will require additional API change.

Author: CarnaViire
Assignees: -
Labels:

api-suggestion, area-Extensions-HttpClientFactory

Milestone: -

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jan 17, 2021
@davidfowl
Copy link
Member

👀

@halter73
Copy link
Member

IScopedHttpClientFactory and IScopedHttpMessageHandlerFactory are interesting. My knee-jerk reaction is that we shouldn't add these interfaces because there's no enforcement of the lifetime. What stops someone from registering an IScopedHttpClientFactory as a singleton or transient service?

If we want to cache/reuse the connection, it is enough to cache/reuse the bottom-most handler of the chain (aka PrimaryHandler). Other handlers in the chain may be re-instantiated for each unit-of-work scope, so they will have the correct instances of the scoped services injected into them (which is desired by customers).

I guess what makes this a bit unusual in that you want the ability to layer scoped decorators on top of singleton services. Both the singleton and scoped decorators need to implement what's functionally the same service interface, so we're not left with many options. [1] I would like to see an end-to-end example for how these scoped decorators would be defined and configured.

Service interfaces with a specific intended lifetime could be a good use case for a Roslyn analyzer. We could add attributes to service interfaces indicating what the intended lifetime is (if any) and warn if it's registered differently. We could do something similar for service implementations.

1: It would be nice if our DI system made decorating services easier in general. Today, you have to create custom interfaces/classes for registering the decorated type.

@CarnaViire
Copy link
Member Author

You may see the actual implementation of IScopedHttpClientFactory in my PR here if that's what you were asking about @halter73

I agree, the naming pains me a little bit in a way you say, too. But I honestly couldn't come up with anything better, as the purpose of this service was to be scoped - to be able to provide scope where it's needed, opposed to singleton IHttpClientFactory.

But that's also touching on an interesting topic. If a person purposely refuses our default implementation by registering their own, in what way and how are we responsible for it?

If we are against allowing to substitute this one, we can always forbid it by registering via AddScoped instead of TryAddScoped, isn't that right?

@karelz karelz added this to the 6.0.0 milestone Jan 20, 2021
@karelz karelz removed the untriaged New issue has not been triaged by the area owner label Jan 20, 2021
@zmj
Copy link

zmj commented Jan 20, 2021

+1 for the use case. I wanted to inject some request-scoped values into a delegating handler wrapping the base handler and didn't find a nice way to do that with the current configuration options for HttpClientFactory.

@CarnaViire
Copy link
Member Author

Edit: Changed main proposal to be about transient HttpClientFactory as there's no need for an additional interface in this case. Moved IScopedHttpClientFactory to alternatives. Removed option with IServiceProvider parameter completely as it is both a bad practice and inconvenient to use.

@CarnaViire
Copy link
Member Author

While discussing this API proposal we've found some additional complications.

Users that want to use PreserveExistingScope scenario might also want to set some Primary Handler parameters like MaxConnectionsPerServer or SslOptions at the same time, so we need to address that.

For users to be able to do that, we will need to separate configuring a primary handler from configuring the rest of the chain. Right now, all handlers configuration is stored in a single collection in HttpClientFactoryOptions:
IList<Action<HttpMessageHandlerBuilder>> HttpMessageHandlerBuilderActions

All handler configuration from methods like AddHttpMessageHandler and ConfigurePrimaryHttpMessageHandler is translated into actions on HttpMessageHandlerBuilder. This HttpMessageHandlerBuilder contains both a list of additional message handlers and a primary handler (to build a chain from them in the end). This way a chain is always created as a whole, but we want to be able to create the parts separately.

Additional API change no.1:
Separate primary handler changes in another place in HttpClientFactoryOptions. Add e.g.
IList<Action<HttpMessageHandlerBuilder>> PrimaryHttpMessageHandlerBuilderActions
or even
Function<IServiceProvider, HttpMessageHandler> PrimaryHttpMessageHandlerFactory.

After that, we want to put all primary handler changes into a new collection and additional handler changes into an old collection.

However, this separation is not straightforward. While AddHttpMessageHandler and ConfigurePrimaryHttpMessageHandler are easy, there are some APIs in HttpClientFactory that make it impossible to classify the changes. The APIs I'm speaking about are:

  1. public static IHttpClientBuilder ConfigureHttpMessageHandlerBuilder(this IHttpClientBuilder builder, Action<HttpMessageHandlerBuilder> configureBuilder)
    (this one can be called upon HttpClient configuration)
    and
  2. public interface IHttpMessageHandlerBuilderFilter with a method Action<HttpMessageHandlerBuilder> Configure(Action<HttpMessageHandlerBuilder> next);
    (for this, user can create their own implementation and register it in DI)

These both expose HttpMessageHandlerBuilder itself. As soon as any of these APIs are used, we can no longer decouple changes of the primary handler from changes of additional message handlers — because both of them may be changed in a single action.

That means we cannot support these APIs fully. There is no good way to treat them – see some options below:

Additional API change no.2:
The possible options are:

  • Deprecate ConfigureHttpMessageHandlerBuilder and IHttpMessageHandlerBuilderFilter and replace them with new APIs to provide functionality of modifying the list of additional handlers (e.g. put a handler into a specific position in a list)
  • Make ConfigureHttpMessageHandlerBuilder throw when per-request scope is enabled for HttpMessageHandlers - this doesn't fix IHttpMessageHandlerBuilderFilter problem
  • Make PrimaryHandler modifications throw at runtime when per-request scope is enabled for HttpMessageHandlers
  • Ignore any changes to PrimaryHandler – let the objects be allocated, but never used by us (we would overwrite it with the shared/cached primary handler)

Conclusion:
The combined API changes are quite extensive which is more than what we were planning for solving the initial issue. As a result, we are moving the proposal out of 6.0 to Future. We can reconsider the prioritization when there are more customers asking for this, or if we come up with better solution to the problem.

@CarnaViire CarnaViire modified the milestones: 6.0.0, Future Mar 2, 2021
@zmj
Copy link

zmj commented Jun 21, 2021

@CarnaViire given the difficulties you've outlined in modifying the builder methods, maybe it's worth considering what a consumer-side (meaning the code consuming an HttpClient produced by the factory) would look like? My workaround was an extension method public static void AddDelegatingHandler(this HttpClient httpClient, Func<HttpMessageHandler, DelegatingHandler> createDelegatingHandler) that is called by a scoped service to inject request-scoped handling.

@kaluznyt
Copy link

Hi, after some hours of struggling to make that work, I found this issue describing the thing I'm encountering, namely scoped service dependency in custom delegating handler. Is this something being worked on or abandoned ?

@CarnaViire
Copy link
Member Author

@kaluznyt it is not abandoned, but rather deprioritized at the moment, as there's not enough customer ask.

If you are interested in the feature, please upvote the top post, it would help us prioritize!

@kaluznyt
Copy link

@CarnaViire thanks for the response!

@Cobra86
Copy link

Cobra86 commented Apr 15, 2022

I've the same issue and my project is using blazor server .net 6

@DillonN
Copy link

DillonN commented Jul 6, 2022

Chiming in with another use case here, if it's useful

We have a message handler that sets some signature headers, and it requires state from a scoped service. Since we have lots of other configuration going through IHttpClientBuilder, we made a workaround that kept compatibility but required re-implementing some internal logic from Microsoft.Extensions.Http

The workaround is to use a custom method instead of AddHttpClient<T>, to capture services from when the typed client is resolved. Then we use IHttpClientFactory.CreateHandler to get the bulk of the handler pipeline and wrap it in the stateful handler (which can now use scoped services from the "good" service provider). After that it's pretty much just the same logic as in DefaultHttpClientFactory.CreateClient, and a call to ITypedHttpClientFactory<T>.CreateClient for the return

This is causing issues however if any of the standard handlers configured via IHttpClientBuilder need to mutate the outgoing request. Since the signature handler is outer-most, such mutations result in a signature mismatch. I've thought of modifying the workaround so that the outer-most handler just passes state via HttpRequestMessage.Options to an inner handler, which can calculate signatures at the end.. either that or just give in to reflection at this point. (We are not using ASP.NET Core so IHttpContextAccessor isn't an option). I did end up swapping to having a dedicated scope capturing outer handler that passes down scope via HttpRequestMessage.Options

Needless to say, official support for this would be very much appreciated!

@bfriesen
Copy link
Contributor

Like @Cobra86, I have the same issue. I have a Blazor server-side project that needs a message handler to have access to something registered as Scoped. Specifically, I need to add a message handler that has access to the scoped-registered TokenProvider described in the Blazor server-side documentation.

My workaround is to scoped-register a custom implementation of IHttpClientFactory that behaves like just like the default implementation, except it adds my custom message handler as the outer-most message handler of the resulting HttpClient.

public class AuthenticatingHttpClientFactory : IHttpClientFactory
{
    private readonly TokenProvider _tokenProvider;
    private readonly IHttpMessageHandlerFactory _messageHandlerFactory;
    private readonly IOptionsMonitor<HttpClientFactoryOptions> _optionsMonitor;

    public AuthenticatingHttpClientFactory(
        IHttpMessageHandlerFactory messageHandlerFactory,
        TokenProvider tokenProvider,
        IOptionsMonitor<HttpClientFactoryOptions> optionsMonitor)
    {
        _messageHandlerFactory = messageHandlerFactory;
        _tokenProvider = tokenProvider;
        _optionsMonitor = optionsMonitor;
    }

    public HttpClient CreateClient(string name)
    {
        ArgumentNullException.ThrowIfNull(name, nameof(name));

        var handler = _messageHandlerFactory.CreateHandler(name);
        
        // If we have an access token, add an outermost message handler that takes care of authentication.
        if (_tokenProvider.AccessToken != null)
            handler = new AuthenticatingMessageHandler(_tokenProvider, handler);
        
        var client = new HttpClient(handler, disposeHandler: false);

        HttpClientFactoryOptions options = _optionsMonitor.Get(name);
        for (int i = 0; i < options.HttpClientActions.Count; i++)
        {
            options.HttpClientActions[i](client);
        }

        return client;
    }
}

This extension method replaces the AddHttpClient extension method from Microsoft.Extensions.Http.

public static IHttpClientBuilder AddScopedAuthenticatingHttpClient(
    this IServiceCollection services,
     string name,
     Action<HttpClient> configureClient)
{
    services.AddHttpClient();

    // Replace the registration for IHttpClientFactory...
    var httpClientFactoryRegistration = services.FirstOrDefault(service => service.ServiceType == typeof(IHttpClientFactory));
    if (httpClientFactoryRegistration is not null)
        services.Remove(httpClientFactoryRegistration);

    // ...with a scoped registration for our authenticating factory.
    services.AddScoped<IHttpClientFactory, AuthenticatingHttpClientFactory>();

    var builder = new DefaultHttpClientBuilder(services, name);
    builder.ConfigureHttpClient(configureClient);
    return builder;
}

The custom message handler itself isn't very special, it just needs has access to the scoped-registered TokenProvider.

public class AuthenticatingMessageHandler : DelegatingHandler
{
    private readonly TokenProvider _tokenProvider;

    public AuthenticatingMessageHandler(TokenProvider tokenProvider, HttpMessageHandler innerHandler)
        : base(innerHandler)
    {
        _tokenProvider = tokenProvider;
    }

    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", _tokenProvider.AccessToken);

        return await base.SendAsync(request, cancellationToken);
    }
}

@brendonparker
Copy link

I ran into this today. I had some places in code that were building an entirely new ServiceProvider. I benchmarked and noticed the overhead, mostly in my loading of remote configuration. So thought to switch it to a single ServiceProvider and leverage scopes. This is for a multi-tenant scenario where I effectively want to control some "context" for the tenant. Then I noticed my use of IHttpClientFactory broke since values of this "context' were null. Then eventually figured out my issue.

Its basically the same use case that is described above by @bfriesen

@tallichet
Copy link

We are also facing the need to have "by Scope" authentication in the Message handler, to be able for our Blazor Server Side to use Refit and all the HttpClientFactory.

@CarnaViire
Copy link
Member Author

Thank you for your insights and workarounds, @DillonN, @bfriesen, @brendonparker, @tallichet!

This is rather complicated topic with a non-obvious solution. Even the implementation will be rather complex (once we have agreement on a solution).
Given all the workload on our team, I don't see this issue fitting into .NET 8.0. I still want to keep it open in Future and continue working on it after .NET 8.0.

As a short-term solution, we will add a workaround into HttpClientFactory docs. That will make it easier for other users to find it.

If you are interested in the feature, please upvote the top post, it would help us prioritize!

@CarnaViire
Copy link
Member Author

Workaround documented in https://learn.microsoft.com/en-us/dotnet/core/extensions/httpclient-factory#message-handler-scopes-in-ihttpclientfactory

@CarnaViire
Copy link
Member Author

API changes introduced in #87914 pretty much cover what was mentioned in #47091 (comment) 🥳 While the issue still remains in the Future bucket, this does bring it closer.

@CarnaViire
Copy link
Member Author

Given the discussion in #35987 I will optimistically put this to 9.0 as well.

@CarnaViire
Copy link
Member Author

Sorry folks -- this will not fit into 9.0 anymore, even the case when the Keyed DI infra is used 😢

Some info here #89755 (comment)

@julealgon
Copy link

Is this yet another MEDI-related hack for lack of functionality in the container: in this case, custom object lifetimes?

@Seabizkit
Copy link

Ironically;

I posted saying it should respect the life time of the scope it was created in, but:

When consuming via Lambda or other applications, I actually want/like the existing behavior.

but when consumed via plugin,

I want to basically ensure everything is disposed, it become confusing when things are still being held onto, and i cant figure out why

I think keep the existing behavior as the default, as to many existing apps, will break and some of opt in for, if you want it to change the life time to be scoped.

Some of the proposals seem neat.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-HttpClientFactory
Projects
None yet
Development

No branches or pull requests