Open CarnaViire opened 3 years ago
Tagging subscribers to this area: @dotnet/ncl See info in area-owners.md if you want to be subscribed.
Author: | CarnaViire |
---|---|
Assignees: | - |
Labels: | `api-suggestion`, `area-Extensions-HttpClientFactory` |
Milestone: | - |
👀
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.
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?
+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.
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.
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:
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)ConfigureHttpMessageHandlerBuilder
throw when per-request scope is enabled for HttpMessageHandlers
- this doesn't fix IHttpMessageHandlerBuilderFilter
problemHttpMessageHandlers
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 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.
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 ?
@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!
@CarnaViire thanks for the response!
I've the same issue and my project is using blazor server .net 6
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 I did end up swapping to having a dedicated scope capturing outer handler that passes down scope 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).HttpRequestMessage.Options
Needless to say, official support for this would be very much appreciated!
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);
}
}
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
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.
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!
API changes introduced in https://github.com/dotnet/runtime/issues/87914 pretty much cover what was mentioned in https://github.com/dotnet/runtime/issues/47091#issuecomment-789159437 🥳 While the issue still remains in the Future bucket, this does bring it closer.
Given the discussion in https://github.com/dotnet/runtime/issues/35987 I will optimistically put this to 9.0 as well.
Updated
The scope can be provided via the keyed services infra. The API to opt-in into Keyed services registration is proposed in https://github.com/dotnet/runtime/issues/89755
Usage:
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 severalHttpClient
configurations in DI container and then instantiateHttpClient
s according to the respective configuration. A configuration can specify thatHttpClient
should use a specificHttpMessageHandler
or even a chain of such handlers. When creating a client,HttpClientFactory
caches and reusesHttpMessageHandler
s to avoid creating too many connections and exhausting sockets, so handlers will live for a configurable timespanHandlerLifetime
.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
Usage Examples
The only change needed for both named and typed clients is to opt-in via callling
SetPreserveExistingScope(true)
Named client example:
Typed client example:
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 interfaceIScopedHttpClientFactory
is added here.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:
Typed client example:
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 creatingHttpMessageHandler
s. The only thing that will change is that there will be more allocations (every injection ofHttpClientFactory
will create a new instance).New opt-in behavior is only meaningful within a scope, so
HttpClientFactory
should be resolved within a scope forPreserveExistingScope=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 ofPreserveExistingScope=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 modifyingPrimaryHandler
, it will not work as expected, but will produce potential security risk and impact performance by creating redundantPrimaryHandler
s to be thrown away. Addressing the risks of allowingPrimaryHandler
modification will require additional API change.2021-02-11 Edit: Changed main proposal to be about transient
HttpClientFactory
. MovedIScopedHttpClientFactory
to alternatives. Removed option withIServiceProvider
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 withIHttpMessageHandlerFactory
, but actual usage examples where only scoped message handler would be needed but notHttpClient
are not clear. It can be easily added later if there will be any demand for that.