-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[API Proposal] ConfigureHttpClientDefaults for HttpClientFactory #87914
Comments
Tagging subscribers to this area: @dotnet/ncl Issue DetailsBackground and motivation
services
.AddHttpClient("consoto", c => c.BaseAddress = new Uri("https://consoto.com/"))
.AddHttpMessageHandler<MyAuthHandler>();
services
.AddHttpClient("github", c => c.BaseAddress = new Uri("https://github.com/"))
.AddHttpMessageHandler<MyAuthHandler>(); Two named clients are defined in the example above. Unfortunately, there isn't a way to apply the default settings for all clients. Both need This issue proposes an API Proposalnamespace Microsoft.Extensions.DependencyInjection;
public static class HttpClientFactoryServiceCollectionExtensions
{
public static IHttpClientBuilder AddHttpClientDefaults(this IServiceCollection services);
} API Usage
services.AddHttpClientDefaults()
.AddHttpMessageHandler<MyAuthHandler>();
// Clients automatically have the handler specified as a default.
services.AddHttpClient("consoto", c => c.BaseAddress = new Uri("https://consoto.com/"));
services.AddHttpClient("github", c => c.BaseAddress = new Uri("https://github.com/")); The default configuration is run on a client before client-specific configuration: services.AddHttpClientDefaults()
.ConfigureHttpClient(c => c.BaseAddress = new Uri("https://consoto.com/"));
// Client a base address of https://consoto.com
services.AddHttpClient("consoto");
// Client has a base address of https://github.com/ (overrides default)
services.AddHttpClient("github", c => c.BaseAddress = new Uri("https://github.com/")); Default configuration can be added to multiple times, and its order compared to services.AddHttpClientDefaults()
.ConfigureHttpClient(c => c.BaseAddress = new Uri("https://consoto.com/"));
// Client a base address of https://consoto.com and has MyAuthHandler
services.AddHttpClient("consoto");
services.AddHttpClientDefaults()
.AddHttpMessageHandler<MyAuthHandler>(); Alternative DesignsWe want We want to avoid the situation we are seeing where people are adding two extension methods: one to add configuration to a builder, and another to apply configuration to all builders. Example of what we don't want: // Add handler to one client
services.AddHttpClient("myclient").AddMyCustomerLogging();
// Add handler to all clients
services.AddMyCustomerLoggingToAllClients(); RisksNo response
|
We had a discussion with @stephentoub yesterday about the API shape and we came up with a potentially more easy-to-undersand API: public static IServiceCollection ConfigureHttpClientDefaults(this IServiceCollection services, Action<IHttpClientBuilder> configure) {} One of the main points was that I strongly believe we need 2 additional APIs to be able to conveniently modify default configuration per-name: public static IHttpClientBuilder ConfigurePrimaryHttpMessageHandler(this IHttpClientBuilder builder, Action<HttpMessageHandler, IServiceProvider> configureHandler) {}
public static IHttpClientBuilder ConfigureAdditionalHttpMessageHandlers(this IHttpClientBuilder builder, Action<IList<DelegatingHandler>, IServiceProvider> configureAdditionalHandlers) {} While this can be achieved through I'd like to avoid encouraging users to use the API that forbids optimizations and that we are planning to eventually deprecate (as it prevents us from improvements and/or fixes like #35987 and #47091 that require separation of PrimaryHandler creation from additional handlers creation) -- and currently there's no other way. Usages: services.ConfigureHttpClientDefaults(b =>
b.AddHttpMessageHandler<MyAuthHandler>()
.ConfigurePrimaryHandler(() => new SocketsHttpHandler() { UseCookies = false }));
// will have MyAuthHandler and SocketsHttpHandler with UseCookies = false
services.AddHttpClient("foo");
// will have MyAuthHandler and SocketsHttpHandler with UseCookies = false and MaxConnectionsPerServer = 1
services.AddHttpClient("bar")
.ConfigurePrimaryHandler((handler, _) => ((SocketsHttpHandler)handler).MaxConnectionsPerServer = 1);
// will have MyLoggingHandler as a top-most wrapper handler, MyAuthHandler, and SocketsHttpHandler
// with UseCookies = false
services.AddHttpClient("baz")
.ConfigureAdditionalHttpMessageHandlers((handlerList, _) => handlerList.Insert(0, new MyLoggingHandler());
// will have SocketsHttpHandler with UseCookies = false and will not have MyAuthHandler
services.AddHttpClient("qux")
.ConfigureAdditionalHttpMessageHandlers((handlerList, _) =>
handlerList.Remove(handlerList.SingleOrDefault(h => h.GetType() == typeof(MyAuthHandler)))); |
Good point that Lines 78 to 86 in 3195fbb
I like how
Do they need to be done right now as part of this issue? As far as I can tell they don't block the customer scenarios in this issue, and they aren't blocked from being done in the future. Off topic, I noticed a problem in one of your sample usages:
In the proposed API, this example is missing a cast from // will have MyAuthHandler and SocketsHttpHandler with UseCookies = false and MaxConnectionsPerServer = 1
services.AddHttpClient("bar")
.ConfigurePrimaryHandler((handler, _) => ((SocketsHttpHandler)handler).MaxConnectionsPerServer = 1); |
I believe they need to be part of this release. While they don't block scenario of setting defaults, their absence subsequently encourages the use of
Good catch, thanks! I'll update it. |
Good point, even though that's not strictly true, it will add all the HttpClientFactory infra the first time it's called... so you can use the factory to create (unconfigured/default) clients right away. And while
Let me also add that they don't have to be optimized now, if you are worried about that. Non-optimized implementation is a straightforward one-liner. |
The concept of a parameterless method adding a new named client makes sense. The concept of a parameterless method adding defaults does not; it suggests without doing it there aren't defaults, which is both confusing and incorrect. |
Updating to use ConfigureHttpClientDefaults: Simple change. Feels good to use. Add extra configure methods for additional handlers and primary handler: I implemented these by using the existing What happens if someone does: services.AddHttpClient("bar")
.ConfigurePrimaryHandler((handler, _) => ((SocketsHttpHandler)handler).MaxConnectionsPerServer = 1);
services.AddHttpClient("bar")
.ConfigurePrimaryHandler(() => new SocketsHttpHandler() { UseCookies = false })); Exact thought needs to be put into when these methods run. Another example: services.AddHttpClient("bar")
.AddHttpMessageHandler(() => Mock.Of<DelegatingHandler>())
.ConfigureAdditionalHttpMessageHandlers((additionalHandlers, _) =>
{
additionalHandlers.Clear();
}); Someone might expect the code above to result in no additional handlers for the client. Actually the client still has two additional handlers. After the delegate added by |
I think the intent is these configuration methods is they run after public class HttpClientFactoryOptions
{
+ public IList<Action<HttpMessageHandler>> PrimaryMessageHandlerActions { get; }
+ public IList<Action<IList<HttpMessageHandler>> AdditionalHttpMessageHandlerActions { get; }
} |
Thanks @JamesNK!
That was exactly the intent, and it aligns with how usually in DI registrations the last one "wins". So in the first example, the second ConfigurePrimaryHandler overwrites the previous handler, and that's expected.
The idea is that they are run in the same time they would previously run when people used ConfigureHttpMessageHandlerBuilder and allow for simple transition with the similar and expected functionality. Basically, implementation via HttpMessageHandlerBuilderActions is a baseline, and it further can be optimized by splitting into two collections PrimaryMessageHandlerActions and AdditionalHttpMessageHandlerActions, etc.
The final goal is that there would be no HttpMessageHandlerBuilderActions, so both collections PrimaryMessageHandlerActions and AdditionalHttpMessageHandlerActions could be run independently (but still depending on the order of registration within each group). As soon as anything is added to HttpMessageHandlerBuilderActions, everything would need to fall back to single HttpMessageHandlerBuilderActions collection to retain order.
It's nice to have, but I believe it still can be done with them being internal.
I would consider logging to be something a bit separate from other additional handlers, and with separate configuration to be discussed in #85840. Also, the same would happen if you clear the collection via ConfigureHttpMessageHandlerBuilder so I would vote for them to have similar behavior. |
I know you want people to stop using the build Source: Lines 63 to 221 in cf50b7d
If the new methods aren't intended to run after all the |
I know. What I meant is something like the following that could be called instead of internal void AddPrimaryHandlerAction(Action<IPrimaryHandlerBuilder> action, bool disregardPreviousActions)
{
if (_primaryHandlerActions != null) // HttpMessageHandlerBuilderActions is empty, collections are separated
{
if (disregardPreviousActions)
{
_primaryHandlerActions.Clear();
}
_primaryHandlerActions.Add(action);
}
else
{
// can't optimize in case of a unified collection
HttpMessageHandlerBuilderActions.Add(action);
}
} Or you meant access to that functionality from the user side as well? |
I'm going to leave any more changes until after the API is approved. It's important to be clear about the configuration's order with This is a subtle but important detail. Right now there isn't enough information about the order of configuration for these methods and their interaction with older methods. |
@CarnaViire What's the status here? I'm going on leave in less than a week and there are outstanding questions. When will they be answered? When will this go to API review? Reminder: I can't go to API review because of timezones and you'll need to represent this. |
I'm preparing the description for the API review, together with #84075 and #77312
We don't have a date yet, but I've let Immo know, as I'd like to go over all HttpClientFactory APIs in one session
That's what I intended to do, yes
What exact questions are not answered? I might be missing something, but I believe I've commented on the expected behavior previously here #87914 (comment) (expected = exactly how it would behave if it was done by
I can take over the PR if needed, I believe if there would be any changes, they would be minor, like method names and such |
Thanks, I wasn’t sure if the order question was answered. |
namespace Microsoft.Extensions.DependencyInjection;
public static partial class HttpClientFactoryServiceCollectionExtensions
{
public static IServiceCollection ConfigureHttpClientDefaults(
this IServiceCollection services,
Action<IHttpClientBuilder> configure);
}
public static partial class HttpClientBuilderExtensions
{
// Existing:
//
// public static IHttpClientBuilder ConfigurePrimaryHttpMessageHandler(
// this IHttpClientBuilder builder,
// Func<HttpMessageHandler> configureHandler);
//
// public static IHttpClientBuilder AddHttpMessageHandler(
// this IHttpClientBuilder builder,
// Func<DelegatingHandler> configureHandler);
// new
public static IHttpClientBuilder ConfigurePrimaryHttpMessageHandler(
this IHttpClientBuilder builder,
Action<HttpMessageHandler, IServiceProvider> configureHandler);
public static IHttpClientBuilder ConfigureAdditionalHttpMessageHandlers(
this IHttpClientBuilder builder,
Action<IList<DelegatingHandler>, IServiceProvider> configureAdditionalHandlers);
// Existing API which we should obsolete
[Obsolete(...)] // Should point to ConfigureAdditionalHttpMessageHandlers
public static IHttpClientBuilder ConfigureHttpMessageHandlerBuilder(
this IHttpClientBuilder builder,
Action<HttpMessageHandlerBuilder> configureBuilder);
} |
The AddHttpClientDefaults method supports adding configuration to all created HttpClients. The method: - Creates a builder with a null name. Microsoft.Extensions.Configuration automatically applies configuration with a null name to all named configuration. - Ensures that default configuration is added before named configuration in the IServiceCollection. This is to make it so the order of AddHttpClientDefaults and AddHttpClient doesn't matter. Default config is always applied first, then named config is applied after. This is done by wrapping the IServiceCollection in an implementation that modifies the order that IConfigureOptions<HttpClientFactoryOptions> values are added. Fixes #87914 --------- Co-authored-by: Natalia Kondratyeva <knatalia@microsoft.com>
Original issue by @JamesNK
Background and motivation
HttpClientFactory
in Microsoft.Extensions.Http allows a developer to centrally configure and instatiate HTTP clients with DI:Two named clients are defined in the example above. Unfortunately, there isn't a way to apply the default settings for all clients. Both need
AddHttpMessageHandler<MyAuthHandler>()
.This issue proposes an
AddHttpClientDefaults
method that can be used to specify configuration that is applied to all clients.API Proposal
namespace Microsoft.Extensions.DependencyInjection; public static class HttpClientFactoryServiceCollectionExtensions { + public static IHttpClientBuilder AddHttpClientDefaults(this IServiceCollection services); }
API Usage
AddHttpClientDefaults
returns anIHttpClientBuilder
. This is the same type returned byservices.AddHttpClient(...)
. That means all the extension methods forIHttpClientBuilder
automatically work withAddHttpClientDefaults
.The default configuration is run on a client before client-specific configuration:
Default configuration can be added to multiple times, and its order compared to
AddHttpClient
doesn't matter:Alternative Designs
We want
AddHttpClientDefaults
that returns anIHttpClientBuilder
so existing extension methods automatically work with it.We want to avoid the situation that we are seeing where people are adding two extension methods: one to add configuration to a builder, and another to apply configuration to all builders.
Example of what we don't want:
Risks
No response
Background and motivation
HttpClientFactory
in Microsoft.Extensions.Http allows a developer to configure and create HttpClient instances with DI.Consider an example where two named clients are defined, and they all need MyAuthHandler in their message handlers chain.
Unfortunately, there isn't a way to apply the default settings for all clients. All need
AddHttpMessageHandler<MyAuthHandler>()
.This issue proposes an
ConfigureHttpClientDefaults
method that can be used to specify configuration that is applied to all clients.API Proposal
API Usage
1. Basic usage and extension methods
ConfigureHttpClientDefaults
accepts an action overIHttpClientBuilder
. This is the same type returned byservices.AddHttpClient(...)
. That means all the extension methods forIHttpClientBuilder
automatically work withAddHttpClientDefaults
.2. Order of applying
The default configuration is run on a client before client-specific configuration:
Default configuration can be added to multiple times, between each other it will be applied one-by-one in order of registration; and its place in the registration compared to
AddHttpClient
doesn't matter:3. Subsequent per-name reconfiguration
Setting additional properties in a primary handler:
Inserting in a specific position or removing from an additional handlers list:
4. Subsequent per-name reconfiguration -- order of applying
Between each other, the order of
ConfigurePrimaryHandler
andConfigureAdditionalHttpMessageHandlers
and other per-name configuration methods is the same as if it was implemented viaConfigureHttpMessageHandlerBuilder
-- in order of registration, and if a subsequent write overwrites e.g. a primary handler instance, the last one "wins".The text was updated successfully, but these errors were encountered: