-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
HttpClientFactory support for keyed DI #89755
Comments
Tagging subscribers to this area: @dotnet/ncl Issue DetailsHttpClientFactory allows for named clients. The new keyed DI feature should be used to resolve clients by their name. Required today: services.AddHttpClient("adventureworks", c => c.BaseAddress = new Uri("https://www.adventureworks.com"));
services.AddHttpClient("contoso", c => c.BaseAddress = new Uri("https://www.contoso.com"));
public class MyController
{
public MyController(IHttpClientFactory httpClientFactory)
{
_adventureWorksClient = httpClientFactory.CreateClient("adventureworks");
_contosoClient = httpClientFactory.CreateClient("contoso");
}
} With keyed DI: services.AddHttpClient("adventureworks", c => c.BaseAddress = new Uri("https://www.adventureworks.com"));
services.AddHttpClient("contoso", c => c.BaseAddress = new Uri("https://www.contoso.com"));
public class MyController
{
public MyController(
[FromKeyedServices("adventureworks")] HttpClient adventureWorksClient,
[FromKeyedServices("contoso")] HttpClient contosoClient)
{
_adventureWorksClient = adventureWorksClient;
_contosoClient = contosoClient;
}
}
|
Triage: We should try to integrate with KeyedServices new feature if it is low cost. It will avoid introducing potential breaking changes in next release. |
Moving it to 9.0 per discussion in #90272 (comment) |
I think we might want to consider
This is sad. Ironically, our guidance for transient IDisposables is to "use the factory pattern to create an instance outside of the parent scope. In this situation, the app would generally have a Create method that calls the final type's constructor directly." Given that keyed services are intended to be replacements for simple factories, it'd be nice if service registrations could opt-out of automatic disposal. #36461 is tracking this as a global option, but for something like this feature to make use of it, it would need to be configurable per ServiceDesriptor. @davidfowl @benjaminpetit If we were designing something completely new, we'd just make sure the transient service and implementation types did not implement IDisposable, but that's not an option for HttpClient which brings me to my next thought...
What if we made the keyed HttpClient registration a singleton? I understand that it would not be able to participate in handler rotation as currently implemented. But as you point out, this is already broken when typed clients are used in singletons or when And we could try fixing this problem in the majority of cases by registering a keyed singleton HttpClient decorator that forwards SendAsync to the "real" HttpClient. If we updated IHttpClientFactory.CreateClient() to return this keyed singleton decorator, this would also fix existing bugs that people might not realize they have. I know that there's more than just SendAsync to consider such as the settable, non-virtual BaseAddress property, but I doubt it's common to modify these after the HttpCilent is resolved. We could throw if someone tries to change BaseAddress, Timeout, MaxResponseContentBufferSize, etc... after it's been resolved from DI to mitigate this. As much as I would love to make both keyed service and singleton rotation support opt-out rather than opt-in, I understand this would be pretty breaking. But if we're going to make it opt-in either way, we might as well make all the breaking changes we want to at once. |
I'm sad to see @JamesNK original proposal not be the one used in the end here and instead you keep piling on special casing and custom stuff into this nasty With each extra custom API that gets added like this, you make the whole DI system more and more complicated to use. At least James' original idea intended to remove some of the "custom" nature of the method and rely on the new keyed service support. Now it will be even more custom with extra API additions. I've made this point in a few other issues with a few different folks before, but I think you should really reconsider this for the long term here. What the DI framework needs is a way to indicate a dependent service registration/configuration that works not only for
You keep creating special cases around each and that will just not scale in the long term IMHO. |
It was actually the first name I considered, and then I forgot to include it in the list 🙈 The problem with services.AddHttpClient("foo").AsKeyedScoped(); // nice! but as soon as it's placed after any other configuration methods, it becomes super confusing, like it is no longer about the client, but about the previous item: services.AddHttpClient("foo")
.AddHttpMessageHandler(...).AsKeyedScoped(); // is it about the additional handler?
services.AddHttpClient("foo")
.UseSocketsHttpHandler(...).AsKeyedScoped(); // is it about SocketsHttpHandler?
services.AddHttpClient("foo")
.SetHandlerLifetime(...).AsKeyedScoped(); // ???
That would be PERFECT actually. I wonder if that's something that could be squeezed into 9.0 (sweet dreams 😅) This would make transient clients "safe" from the memory leak, so that would mean that we can (at least) bring back And then we even technically can consider
Some time ago, we actually did briefly entertain the idea of pushing the rotation logic down -- into some kind of a "magic handler". This sounds very close to what you were proposing, but with a "wrapper" handler instead of a "wrapper" client. I still would like to investigate this approach in the future. But this would most possibly require an untrivial amount of time 😢 So while the idea is tempting, it definitely will not fit in 9.0 😢 |
Oh I would love to have a keyed registration a default. (I even tried to do so last year) But with the current state of DI -- we cannot. As I mentioned in the Considerations section -- we can't make a Scoped HttpClient registration a default, and we can't have a Transient HttpClient registration at all at the moment. Also, even if we were able to implement a default Transient registration -- I still see a value in having an option to change the lifetime, and use Scoped instead. But then you need an API for that 😄
I'm not sure I understand the "contrast" between "ideas" you're trying to make. These are different things/"points in time". The initial ask was to be able to inject the clients via the keyed services infra. So the "custom nature" that is being "removed" in this case is the necessity to inject an IHttpClientFactory instance. The ask is still satisfied, and the "custom nature" is still being "removed", even with the presence of the opt-in API at the registration time. |
Of course there is value in being able to change the lifetime, I think everyone would agree with that. The problem is in the second part: the API to do so. The container's original API was badly designed from the beginning IMHO, with the hardcoded nature of the lifetime in the same call as the types to register. These APIs will never scale properly:
Because other APIs were introduced later to register specific things but because the lifetime was built into the original methods (instead of being chained, or just be a parameter, or whatever) you end up with things like:
If suddenly we want a Then if you ever need/want to introduce some sort of fourth lifetime, or a custom lifetime, you are screwed because you now have to go over a bunch of existing methods creating overloads for each of them to support the added lifetime. Many container libraries have solved this issue by decoupling the types to register from the lifetime, either by having the lifetime be a parameter to the registration, or be a fluent call with a separate method, but yet Microsoft still made this obvious mistake. Even the original Unity container didn't have this issue. I honestly believe a new more usable/composable API surface should be designed for MEDI while the old methods should be deprecated. It is the only way to move past these silly limitations. Creating a At that point, why not just have
Fair enough (based on the bolded). My concern was specifically with the addition of the completely custom (and outlandish based on the current API) My take has been that the team should stop and think about this more deeply. Stop introducing special cases for things and making the DI API more convoluted to use, and think about something that works more seamlessly, in a more orthogonal manner. The API as it is is doomed to fail. It will get worse and worse over time as more of these "special" requirements come in. It has already been tarnished with the original introduction of If the container supported dependent registrations, even most cases of named/keyed services would completely go away: services.Add<IHttpClientFactory, HttpClientFactory>(options =>
{
options
.WithSingletonLifetime()
// General purpose "factory" mechanism in the container.
// No need for special casing factories all over the place
// Allows fetching `HttpClient` from the container and use the factory to create them
.FactoryFor<HttpClient>()
.Configure(/* general purpose delegating handlers can be configured here */);
});
services.Add<IMyRepository, GenericRepository>(options => options.WithScopedLifetime());
services.Add<IMyService, MyService>(options =>
{
options
.WithTransientLifetime()
.DependsOn(services =>
{
// Allow for specific dependent registrations that only apply to this service.
// Other services requesting `IMyRepository` will still return `GenericRepository` from above
services.Add<IMyRepository, SomeSpecialRepositoryImplementation>();
// General-purpose `Configure` API that can be used for any object, not only `IOptions`
// this configuration only applies to this particular service since it is inside `DependsOn`.
// Keep in mind the `HttpClient` will still be constructed by the factory as thats defined outside
services.Configure<HttpClient, MyApiOptions>((httpClient, apiOptions) =>
{
httpClient.BaseAddress = apiOptions.BaseAddress;
httpClient.Timeout = apiOptions.GlobalTimeout;
));
services.Configure<IHttpClientFactory>(/* specific delegating handlers can be set here */);
},
});
services.Add<ILoggerFactory, LoggerFactory>(options =>
{
options
.WithTransientLifetime()
.FactoryFor<ILogger>()
.DependOn((services, injectionContext) =>
{
// Overload allows access to `injectionContext` object, which provides the target type
// Factory receives the type and uses it to populate the `Category` for the `ILogger`,
// eliminating the need for the `ILogger<T>` hack.
services.Add<Type>(injectionContext.TargetType);
});
}
public sealed class MyService(
HttpClient httpClient,
IMyRepository myRepository,
ILogger logger)
: IMyService
{
// specific `HttpClient` injected without named/keyed registrations, still created by factory
// custom `IMyRepository` injected without keyed registrations for single use case
// `ILogger` has proper `Category` set based on `MyService` type. No need for `ILogger<T>` to exist
...
}
|
Thanks for the detailed write up @julealgon! I can definitely see your points. I can relate to the frustration. But I must set the expectations -- what you say here
is not an option. We can't break the users like this. I've added a couple of alternative designs that take in a And it is a different API shape already ( |
Updated the top post:
|
Just wanted to record some thoughts regarding pros and cons of the automatic opt-in to the context Scope propagation (#47091), in case the client is registered with the Keyed DI: On one hand... ✅ Align with expectations from resolvingScope propagation will align with expectations from resolving a client, if it has any scoped dependencies. This will "automatically" fix the scope mismatch problem. If I resolve a service from a specific (scoped) service provider, I definitely would expect the service's scoped dependencies to be satisfied from this exact service provider. The existing scope mismatch problem is big enough already. While we technically could try arguing that "bla bla when you get a client from a singleton HttpClientFactory, the factory doesn't have any access to a scope, and that's why a client is created in a different one", this is moot when the factory is hidden "under the hood" of the keyed DI infra. The scope mismatch would become even more prominent and even more confusing. On the other hand...
|
namespace Microsoft.Extensions.DependencyInjection;
public static partial class HttpClientBuilderExtensions
{
public static IHttpClientBuilder AddAsKeyed(this IHttpClientBuilder builder, ServiceLifetime lifetime = ServiceLifetime.Scoped);
public static IHttpClientBuilder RemoveAsKeyed(this IHttpClientBuilder builder);
} |
Updated by @CarnaViire
UPD: Added Alternative Designs section, added opt-out API to the proposal
UPD2: Punted Scope Mismatch fix
HttpClientFactory allows for named clients. The new keyed DI feature can be used to resolve the clients by their name.
API
Usage
Also, should be able to do the following
Considerations
1. Why opt-in and not on by default
Opting in rather than out, because it changes the lifetime from "essentially Transient" to Scoped.
([UPD2: punted]
Plus we expect to fix the scope mismatch problem described by #47091, in case Keyed Services infra ()[FromKeyedServices...]
) is used to inject a client -- but this fix must be opt-in in one way or the other, at least in this releaseAlso I believe there's no straightforward API to un-register a service from service collection once added -- I believe it's done by manually removing the added descriptor from the service collection.
(We can consider making keyed registration a default in the next release, but we'd need to think about good way to opt-out then)
UPD: Added opt-out API to the proposal
2. Why only Keyed Scoped (and not Keyed Transient)
HttpClient
isIDisposable
, and we want to avoid multiple instances being captured by the service provider. Asking for a "new" client each time is a part ofHttpClientFactory
guidelines, but DI will capture allIDisposable
s and hold them until the end of the application (for a root provider; or the end of the scope for a scope provider). Captured Transients will break and/or delay rotation clean up, resulting in a memory leak. There's no way to avoid the capturing that I'm aware of.3. How it should be used in Singletons
By using the "old way" = using
IHttpClientFactory.CreateClient()
-- this will continue working as before = creating a scope per handler lifetime.4. What about Typed Clients
If opting into KeyedScoped, Typed clients
will[UPD2: can] be re-registered as scoped services, rather than transients. (This was actually a long-existing ask that we couldn't implement without some kind of opt-in #36067)This will mean that the Typed clients will stop working in Singletons -- but them "working" there is actually a pitfall, since they're captured for the whole app lifetime and thus not able to participate in handler rotation.
Given that a Typed client is tied with a single named client, it doesn't make much sense to register it as keyed. I see a Typed client as functionally analogous to a service with a
[FromKeyedServices...]
dependance on a named client with name = service type name. See the example below.FWIW I'd even suggest moving away from the Typed Client approach and substitute it with the keyed approach instead. It will also give more freedom if e.g. multiple instances of a "typed client" with different named clients are needed:
Alternative Designs
These are based around passing the lifetime as a parameter to avoid multiple methods. This is different from ordinary DI registrations, but then again, HttpClientFactory is already a different API set.
1. AsKeyed
AsKeyed(ServiceLifetime)
+DropKeyed()
DropKeyed
instead. Inspiration:SocketOptionName.DropMembership
(paired withAddMembership
)UdpClient.DropMulticastGroup
(paired withJoinMulticastGroup
)AsKeyed
alternatives:Keyed(ServiceLifetime)
-- ➕ even more minimalisticAddAsKeyed(ServiceLifetime)
-- ❔ pairs with the alternativeRemoveAsKeyed
DropKeyed
alternatives:RemoveAsKeyed
-- ❔ from main proposal, with "As" inside2. SetKeyedLifetime
SetKeyedLifetime(ServiceLifetime)
+DisableKeyedLifetime()
SetHandlerLifetime(TimeSpan)
-- though also might be a bit confusingSetKeyedLifetime(null)
orClearKeyedLifetime()
are not clear enoughEnableKeyedLifetime(ServiceLifetime)
-- ➕ aligns withDisableKeyedLifetime
AddKeyedLifetime(ServiceLifetime)
-- ❔ hints that a service descriptor will be added to the service collection (e.g. if called multiple times, it will be added multiple times)SetKeyedServiceLifetime(ServiceLifetime)
-- ❔ this one doesn't clash withSetHandlerLifetime
that much, but is more bulkySome dismissed alternative namings/designs
AddHttpClient(...., ServiceLifetime)
overload with a new parameter -- there are already 20 (!!) overloads ofAddHttpClient
, and we'd like to be able to opt-in in all configuration approaches (includingConfigureHttpClientDefaults
)AddKeyedScoped()
(without "As") -- conflicts with existing APIs likeAddHttpMessageHandler
which adds to the client, not to the service collectionAdd
, e.g.Register...
-- doesn't align with ServiceCollection APIs, onlyAdd..
/TryAdd..
is used thereAddKeyedClient()
,AddKeyedScopedHttpClient()
,AddScopedClient()
,AddHttpClientAsKeyedScoped()
,... -- not clear that not only HttpClient will be registered, but also the related HttpMessageHandler chain and, if present, a related Typed client.AddKeyedServices()
,AddToKeyedServices()
-- not clear that it will be scopedOld proposal by @CarnaViire
Usage:
Alternatives:
Original issue by @JamesNK
HttpClientFactory allows for named clients. The new keyed DI feature should be used to resolve clients by their name.
Required today:
With keyed DI:
Also would support multiple typed clients with different names. I think there is validation against doing that today, so need to consider how to change validation to allow it.
The text was updated successfully, but these errors were encountered: