Integrate keyed DI with HttpClientFactory #90272

CarnaViire · 2023-08-09T21:16:30Z

HttpClient (and HttpMessageHandler chain) are registered as keyed transients with client name as the key
Generic parameters from AddHttpMessageHandler, ConfigurePrimaryHttpMessageHandler and AddLogger overloads are resolved as keyed by client name if possible, otherwise fall back to ordinary resolve
Reduced unnecessary primary handler allocations

Transient lifetime was chosen to make this

var client1 = services.GetRequiredKeyedService<HttpClient>(name);
var client2 = services.GetRequiredKeyedService<HttpClient>(name); // new instance

functionally equivalent to this

var client1 = factory.CreateClient(name);
var client2 = factory.CreateClient(name); // new instance

Especially if needed to use in singletons.
I've considered scoped lifetime, but it would be too confusing with typed clients being transient and named clients created by factory being essentially transient.

Scenarios made possible by (1):

Constructor injection with the attribute (can substitute Typed clients):

class KeyedClientTestService
{
    public KeyedClientTestService([FromKeyedServices("test")] HttpClient httpClient)
    {
        HttpClient = httpClient;
    }

Resolving by name directly from service provider (needed when name is not a compile-time constant):

var client = services.GetRequiredKeyedService<HttpClient>(name);
// or
var handler = services.GetRequiredKeyedService<HttpMessageHandler>(name);

Scenarios made possible by (2):

Registering different implementations per name

serviceCollection.AddKeyedSingleton<BaseLogger, FirstClientLogger>("FirstClient");
serviceCollection.AddKeyedSingleton<BaseLogger, SecondClientLogger>("SecondClient");

serviceCollection.AddHttpClient("FirstClient")
    .AddLogger<BaseLogger>(); // will use FirstClientLogger

serviceCollection.AddHttpClient("SecondClient")
    .AddLogger<BaseLogger>(); // will use SecondClientLogger

Easy access to client name from implementation

serviceCollection.AddKeyedSingleton<KeyedHttpClientLogger>(KeyedService.AnyKey); // registered only once

serviceCollection.AddHttpClient("FirstClient")
    .AddLogger<KeyedHttpClientLogger>(); // "KeyedHttpClientLogger.FirstClient" category

serviceCollection.AddHttpClient("SecondClient")
    .AddLogger<KeyedHttpClientLogger>(); // "KeyedHttpClientLogger.SecondClient" category

class KeyedHttpClientLogger : IHttpClientLogger
{
    private readonly ILogger _logger;
    public KeyedHttpClientLogger(ILoggerFactory loggerFactory, [ServiceKey] string key)
    {
        _logger = loggerFactory.CreateLogger(nameof(KeyedHttpClientLogger) + "." + key);
    }

Fixes #89755

ghost · 2023-08-09T21:16:47Z

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

Issue Details

HttpClient (and HttpMessageHandler chain) are registered as keyed transients with client name as the key
Generic parameters from AddHttpMessageHandler, ConfigurePrimaryHttpMessageHandler and AddLogger overloads are resolved as keyed by client name if possible, otherwise fall back to ordinary resolve
Reduced unnecessary primary handler allocations

Transient lifetime was chosen to make this

var client1 = services.GetRequiredKeyedService<HttpClient>(name);
var client2 = services.GetRequiredKeyedService<HttpClient>(name); // new instance

functionally equivalent to this

var client1 = factory.CreateClient(name);
var client2 = factory.CreateClient(name); // new instance

Especially if needed to use in singletons.
I've considered scoped lifetime, but it would be too confusing with typed clients being transient and named clients created by factory being essentially transient.

Scenarios made possible by (1):

Constructor injection with the attribute (can substitute Typed clients):

class KeyedClientTestService
{
    public KeyedClientTestService([FromKeyedServices("test")] HttpClient httpClient)
    {
        HttpClient = httpClient;
    }

Resolving by name directly from service provider (needed when name is not a compile-time constant):

var client = services.GetRequiredKeyedService<HttpClient>(name);
// or
var handler = services.GetRequiredKeyedService<HttpMessageHandler>(name);

Scenarios made possible by (2):

Registering different implementations per name

serviceCollection.AddKeyedSingleton<BaseLogger, FirstClientLogger>("FirstClient");
serviceCollection.AddKeyedSingleton<BaseLogger, SecondClientLogger>("SecondClient");

serviceCollection.AddHttpClient("FirstClient")
    .AddLogger<BaseLogger>(); // will use FirstClientLogger

serviceCollection.AddHttpClient("SecondClient")
    .AddLogger<BaseLogger>(); // will use SecondClientLogger

Easy access to client name from implementation

serviceCollection.AddKeyedSingleton<KeyedHttpClientLogger>(KeyedService.AnyKey); // registered only once

serviceCollection.AddHttpClient("FirstClient")
    .AddLogger<KeyedHttpClientLogger>(); // "KeyedHttpClientLogger.FirstClient" category

serviceCollection.AddHttpClient("SecondClient")
    .AddLogger<KeyedHttpClientLogger>(); // "KeyedHttpClientLogger.SecondClient" category

class KeyedHttpClientLogger : IHttpClientLogger
{
    private readonly ILogger _logger;
    public KeyedHttpClientLogger(ILoggerFactory loggerFactory, [ServiceKey] string key)
    {
        _logger = loggerFactory.CreateLogger(nameof(KeyedHttpClientLogger) + "." + key);
    }

Fixes #89755

Author:	CarnaViire
Assignees:	-
Labels:	`area-Extensions-HttpClientFactory`
Milestone:	-

CarnaViire · 2023-08-13T23:37:40Z

@halter73 @JamesNK @stephentoub could you please take a look?

stephentoub · 2023-08-13T23:51:36Z

src/libraries/Microsoft.Extensions.Http/src/HttpClientFactoryOptions.cs

+        internal IReadOnlyList<Action<IPrimaryHandlerBuilder>> PrimaryHandlerActions => _primaryHandlerActions!;
+        internal IReadOnlyList<Action<IAdditionalHandlersBuilder>> AdditionalHandlersActions => _additionalHandlersActions!;


Why IReadOnlyList rather than List? The latter will be more efficient, especially if code ever enumerates these. And they're internal, so you don't need to worry about breaks if you ever decided to change the type.

stephentoub · 2023-08-13T23:52:44Z

src/libraries/Microsoft.Extensions.Http/src/HttpClientFactoryOptions.cs

+        private List<Action<IPrimaryHandlerBuilder>>? _primaryHandlerActions = new List<Action<IPrimaryHandlerBuilder>>();
+        private List<Action<IAdditionalHandlersBuilder>>? _additionalHandlersActions = new List<Action<IAdditionalHandlersBuilder>>();
+
+        internal IReadOnlyList<Action<IPrimaryHandlerBuilder>> PrimaryHandlerActions => _primaryHandlerActions!;


Why the nullable suppression? I'd have expected instead these would be nullable but then you'd annotate members like MergedToHandlerBuilderActions with [MemberNotNull(...)] to indicate that they can be used as a guard.

stephentoub · 2023-08-13T23:53:59Z

src/libraries/Microsoft.Extensions.Http/src/DefaultHttpClientFactory.cs

+                        for (int i = 0; i < options.PrimaryHandlerActions.Count; i++)
+                        {
+                            options.PrimaryHandlerActions[i](b);
+                        }
+
+                        for (int i = 0; i < options.AdditionalHandlersActions.Count; i++)
+                        {
+                            options.AdditionalHandlersActions[i](b);
+                        }


If you change PrimaryHandlerActions / AdditionalHandlersActions to be List<T> instead of IReadOnlyList<T>, then you can foreach these, instead.

stephentoub · 2023-08-13T23:55:41Z

src/libraries/Microsoft.Extensions.Http/src/DefaultHttpMessageHandlerBuilder.cs

@@ -28,7 +28,26 @@ public DefaultHttpMessageHandlerBuilder(IServiceProvider services)
            }
        }

-        public override HttpMessageHandler PrimaryHandler { get; set; } = new HttpClientHandler();
+        private HttpMessageHandler? _primaryHandler;
+        internal bool PrimaryHandlerIsSet { get; private set;}


Why is this needed? Why can't _primaryHandler just be checked directly to see whether it's null?

Because there are existing cases when primary handler is set explicitly to null. E.g. in gRPC. While I kinda consider this pattern erroneous, I still don't want to break the users...

stephentoub · 2023-08-14T00:00:44Z

src/libraries/Microsoft.Extensions.Http/src/HttpClientFactoryOptions.cs

+        /// Accessing this property will have a performance impact, as it will disable some optimizations.
+        /// Consider using configuration APIs like ConfigurePrimaryHttpMessageHandler or
+        /// ConfigureAdditionalHttpMessageHandlers instead of accessing this property directly.


Will this regress existing code, or are the optimizations this disables only relevant for code using new features?

stephentoub · 2023-08-14T00:01:31Z

src/libraries/Microsoft.Extensions.Http/src/HttpClientFactoryOptions.cs

+            Debug.Assert(_primaryHandlerActions is not null);
+            Debug.Assert(_additionalHandlersActions is not null);
+
+            List<Action<HttpMessageHandlerBuilder>> handlerBuilderActions = new List<Action<HttpMessageHandlerBuilder>>();


Suggested change

List<Action<HttpMessageHandlerBuilder>> handlerBuilderActions = new List<Action<HttpMessageHandlerBuilder>>();

List<Action<HttpMessageHandlerBuilder>> handlerBuilderActions = new List<Action<HttpMessageHandlerBuilder>>(_primaryHandlerActions.Count + _additionalHandlersActions.Count);

stephentoub · 2023-08-14T00:03:30Z

src/libraries/Microsoft.Extensions.Http/src/HttpClientFactoryOptions.cs

+            handlerBuilderActions.AddRange(_primaryHandlerActions);
+            handlerBuilderActions.AddRange(_additionalHandlersActions);
+
+            _primaryHandlerActions = null;
+            _additionalHandlersActions = null;


Have these lists been handed out to anyone externally? If not, rather than allocating a new list and throwing away both existing ones, you could do:

List<Action<HttpMessageHandlerBuilder>> handlerBuilderActions = _primaryHandlerActions; handlerBuilderActions .AddRange(_additionalHandlersActions); _primaryHandlerActions = null; _additionalHandlersActions = null; return handlerBuilderActions;

stephentoub · 2023-08-14T00:04:10Z

src/libraries/Microsoft.Extensions.Http/src/HttpMessageHandlerBuilder.cs

+
+    internal interface IAdditionalHandlersBuilder : IHandlerBuilder
+    {
+        IList<DelegatingHandler> AdditionalHandlers { get; }


For these internal types, can you use the concrete types instead of interfaces?

davidfowl · 2023-08-14T04:54:24Z

Is this opt in? What happens if I use a container that doesn't support keyed services? Will updating break me?

CarnaViire · 2023-08-14T17:06:54Z

Is this opt in? What happens if I use a container that doesn't support keyed services? Will updating break me?

Good catch @davidfowl, thanks. I naively thought that given that AddKeyed... exists in DependecyInjection.Abstractions package, it will just succeed and/or be a no-op, and the only thing I had to actually check was whether IServiceProvider instance implemented IKeyedServiceProvider as well.

Turns out, it's not so easy. AddKeyed... would succeed, but then when e.g. Autofac would get to import IServiceCollection's descriptors into itself, it will access the ImplementationType property without checking -- unknowingly, since how would you know? you didn't touch this code -- for IsKeyedService first, and boom! 😢😭 Because you should have accessed KeyedImplementationType instead here. I am not sure I'm happy about this API design, but it has its pros, and either way, what can you do now...

The saddest thing is that I see no way in checking whether it's a third-party container being used, let alone whether it supports our new keyed feature (I expect, for quite some time, none of them would). Because we are registering to our own ServiceCollection which just accepts keyed descriptors without any problems, and the problems happen within 3rd party container's code.

Given the point in time, I guess the only way forward is to add an app-context switch for the registration, which will be off by default. 😢

CarnaViire · 2023-08-14T17:58:57Z

Thinking more about this, and how we would have to have it off by default, I'm even considering not taking it into 8.0 at all...

The integration request came a bit too late in the game, but we decided to go with it, because it is a new shiny feature that brings a lot of value, and also to avoid the following: if this would not be part of 8.0 - then someone could make their own keyed HttpClient registrations, and we would clash when we adopt it later in e.g. 9.0... However, now in light that we still have to have some kind of opt-in API and can't have it on by default, the clash doesn't worry me anymore since it will be controlled by the same API.

Thoughts on punting this? @karelz @davidfowl @halter73 @JamesNK

karelz · 2023-08-14T19:00:20Z

Agreed, I don't think it makes sense to rush adding new API post RC1. We can't catch the RC1 train now.

davidfowl · 2023-08-14T21:59:14Z

I think it makes sense to do this early in 9 to better understand how it might break users.

ghost · 2023-10-12T17:01:17Z

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

ghost · 2023-11-11T23:01:27Z

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

davidfowl · 2023-11-11T23:13:47Z

@CarnaViire Lets do this early in .NET 9 so flesh out any issues that might be lurking 😄

CarnaViire · 2023-11-13T12:28:31Z

@davidfowl yes, that's the plan 👍 I have my plate full at the moment, but I will proceed as soon as I'm able to

davidfowl · 2023-11-13T14:59:34Z

Is there an issue outside of this PR?

CarnaViire · 2023-11-13T15:10:07Z

#89755 but I didn't update it to API proposal yet.

I may not be able to proceed in November -- I have a couple of important time-sensitive project, so it's possible we might have to move it to early January (as we might not have a quorum on API reviews in December). Just setting expectations. It would still be an early phase of 9.0, so I think it's fine.

CarnaViire added 5 commits August 3, 2023 22:04

Add keyed services support

8a4f650

Separate handler builder actions

3a7e53d

Add tests

7709396

Add tests

ac5d1af

Bring back default

cf9004b

CarnaViire requested review from halter73, JamesNK and a team August 9, 2023 21:16

dotnet-issue-labeler bot added the area-Extensions-HttpClientFactory label Aug 9, 2023

ghost assigned CarnaViire Aug 9, 2023

This comment was marked as off-topic.

Sign in to view

This was referenced Aug 11, 2023

[Test Failure] EncryptDecrypt_Span.RsaCryptRoundtrip_OaepSHA512 throws CryptographicException : Unknown error #90381

Closed

STATUS_UNSUCCESSFUL in RsaCryptRoundtrip_OaepSHA1 #29683

Open

stephentoub reviewed Aug 13, 2023

View reviewed changes

stephentoub reviewed Aug 14, 2023

View reviewed changes

karelz mentioned this pull request Aug 14, 2023

HttpClientFactory support for keyed DI #89755

Closed

CarnaViire marked this pull request as draft August 14, 2023 19:54

karelz added this to the 9.0.0 milestone Sep 12, 2023

ghost closed this Oct 12, 2023

CarnaViire reopened this Oct 12, 2023

ghost closed this Nov 11, 2023

github-actions bot locked and limited conversation to collaborators Dec 14, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate keyed DI with HttpClientFactory #90272

Integrate keyed DI with HttpClientFactory #90272

CarnaViire commented Aug 9, 2023

ghost commented Aug 9, 2023

This comment was marked as off-topic.

This comment was marked as off-topic.

CarnaViire commented Aug 13, 2023

stephentoub Aug 13, 2023

stephentoub Aug 13, 2023

stephentoub Aug 13, 2023

stephentoub Aug 13, 2023

CarnaViire Aug 14, 2023

stephentoub Aug 14, 2023

stephentoub Aug 14, 2023

stephentoub Aug 14, 2023

stephentoub Aug 14, 2023

davidfowl commented Aug 14, 2023

CarnaViire commented Aug 14, 2023 •

edited

Loading

CarnaViire commented Aug 14, 2023

karelz commented Aug 14, 2023

davidfowl commented Aug 14, 2023

ghost commented Oct 12, 2023

ghost commented Nov 11, 2023

davidfowl commented Nov 11, 2023

CarnaViire commented Nov 13, 2023

davidfowl commented Nov 13, 2023

CarnaViire commented Nov 13, 2023

		internal IReadOnlyList<Action<IPrimaryHandlerBuilder>> PrimaryHandlerActions => _primaryHandlerActions!;
		internal IReadOnlyList<Action<IAdditionalHandlersBuilder>> AdditionalHandlersActions => _additionalHandlersActions!;

	List<Action<HttpMessageHandlerBuilder>> handlerBuilderActions = new List<Action<HttpMessageHandlerBuilder>>();
	List<Action<HttpMessageHandlerBuilder>> handlerBuilderActions = new List<Action<HttpMessageHandlerBuilder>>(_primaryHandlerActions.Count + _additionalHandlersActions.Count);

Integrate keyed DI with HttpClientFactory #90272

Integrate keyed DI with HttpClientFactory #90272

Conversation

CarnaViire commented Aug 9, 2023

ghost commented Aug 9, 2023

This comment was marked as off-topic.

This comment was marked as off-topic.

CarnaViire commented Aug 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Aug 14, 2023

CarnaViire commented Aug 14, 2023 • edited Loading

CarnaViire commented Aug 14, 2023

karelz commented Aug 14, 2023

davidfowl commented Aug 14, 2023

ghost commented Oct 12, 2023

ghost commented Nov 11, 2023

davidfowl commented Nov 11, 2023

CarnaViire commented Nov 13, 2023

davidfowl commented Nov 13, 2023

CarnaViire commented Nov 13, 2023

CarnaViire commented Aug 14, 2023 •

edited

Loading