Adding a HttpMessageHandlerType that is registred as Transint in IServiceCollection creates strange results

[petterek]

If you add a HttpMessageHandler to the list of handlers you want,

httpClientBuilder.AddHttpMessageHandler<CustomHeadersAuth>();
then you need to register the type in the ServiceContainer.

serviceCollection.AddTransient<CustomeHeadersAuth>();
serviceCollection.AddTransient<SomeContexValue>((sp)=> new SomeContextValue());

 public class CustomHeadersAuth: DelegatingHandler
    {
        private readonly SomeContexValue Value;

        public DefaultSimployerAuth(SomeContexValue value)
        {
            this.Value = value;
        }

        protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
        {
            //Add headers from values in clientContext
            request.Headers.Add("PrRequestKey", Value.Key);          
            return base.SendAsync(request, cancellationToken);
        }
    }

They are now all registered as transient, but..
if I then do a loop creating and using the HttpClientFactory.CreateClient("somename")

the value of the first created SomeContextValue is used for all instances of the Client.

I have looked at the code and this is probably by design , because the messageHandlers are cached, but this is kind of hard to figure out, and also frustating..

I do not know what a good solution for this whould be, I solved it in my case by using a factory and getting the value in the SendAsync method in stead.

Should there be some inspection of how the handlers is registerd?

[rynowak]

Hi @petterek - the factory creates and caches a 'chain' of handlers. Since they are all linked together they need to all have the same lifetime, which is why we end up caching your class for the same lifetime as the underlying http client handler.

It sounds like what you want is for SomeContextValue to be created for each outgoing http request you send, is that right? You could solve this by accepting the IServiceProvider in the constructor to CustomerHeadersAuth and then resolving SomeContextValue for each outgoing request.

[petterek]

That is correct. I solved it by creating and accepting an ISomeContextValueGetter and that solved the problem for me.

The issue for me was that no documentaion says anything about "we will take your transient handler and cache it, so it is not transient any more".
Creating the chain of handlers should not be very costly, maybe just caching the last handler would be a solution to this "issue".
Maybe configuring the HttpClientFactory not to cach the whole chain, just the last one?

[rynowak]

@petterek - can I ask what the problem is that you're trying to solve by doing this? Are you trying to tunnel information from the ambient DI scope into the client?

[petterek]

I was trying to inject a "per call" value into the header, of the request. I solved it in my testcase by injecting a factory that is singleton, and then altering the value in the singleton, in front of every call.
I'm using the handler in a non IServiceProvider context as well, so I cannot depend on that.

My point is that the Handler that is registered as transient, is cached by the factory, and suddenly is it not transient any more, but kind of "transient every 10 sec", so the you are "changing" the lifetime of the handler from what is registerd in the DI container.

[cortex93]

@petterek I have a similar problems where I want to flow input request headers like correlation ID to the outbound request.
I manage to do this with :

services.AddHttpClient(clientName)
                .ConfigureHttpClient((provider, client) =>
                {
                    var service = provider.GetRequiredService<IFoo>();
                    client.DefaultRequestHeaders.Add(..., service.GetValue() );
                });

@rynowak This works well when the configuration doesn't depends of the request/response.
I have other needs where I want to inject a bearer token resolved from a token cache stored in a session cookie when the response is 401 and replay the request.

To do this, I need a scoped middleware. And before moving to HttpClientFactory, I managed to only cache the HttpClientHandler and resolve the handler chain each time.

If you re not considering scoping the lifetime of the handler, I think AddHttpMessageHandler should be clear either in its name or in its documentation as to which scope will be applied. For now, there is a remarks which is very confusing : "The configureHandler delegate should return a new instance of the message handler each time it is invoked."
It sounds like the lifetime is transient.

[petterek]

Any more response to this from the team? @rynowak

[rynowak]

My point is that the Handler that is registered as transient, is cached by the factory, and suddenly is it not transient any more, but kind of "transient every 10 sec", so the you are "changing" the lifetime of the handler from what is registerd in the DI container.

Yes, this is what the factory does. We can clear up the docs about this, but the answer is that the factory manages the lifetime of the handlers.

We also discovered that the 2.1 behavior with handlers that come from DI is broken in a significant way other than what's being discussed on this thread. Namely, that a transient handler can outlive the DI scope that it was resolved in, which means that it will be disposed while it's still in use.

---

We're making a change to how the factory uses DI in 2.2 - each time the factory instantiates a handler chain, we will create a scope associated with that handler chain, and keep the scope open until the handler is ready to be disposed. This allows handlers to use other services of any lifetime which is currently broken.

---

So it sounds like what you're asking is to pass data into a handler - there are 2 major ways I can think of to do that:

1. async-local
2. HttpRequestMessage.Properties

Is your data per-request or per client?

[petterek]

My data is pr request, and I have solved my issue, by injecting a factory to handle the data..

And I belive the miss behavior you describe, is part of the same issue that @cortex93 and I is describing.
"Changing the lifetime of a component".

I might not see all the use cases here, but caching the last handler only, whitch is really the one you know and care about, should solve the problem? Letting the DI cache everything else..

[rynowak]

I might not see all the use cases here, but caching the last handler only, whitch is really the one you know and care about, should solve the problem? Letting the DI cache everything else..

No this is not possible sadly. DelegatingHandler.InnerHandler cannot be reassigned once you've sent a request. https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/src/System/Net/Http/DelegatingHandler.cs#L31

[cortex93]

It's per-request. But as you mentioned there is way to circumvent that and I use ihttpcontextaccessor.
But if we really need to share per-request dependency it becomes really cumbersome. For example, if you want the handler to use the same EF DbContext as the controller and save changes as a single Unit of Work.

With the current implementation, I think it's best to think the whole chain as a singleton even if it's recreated from time to time. The fact is that your handler must depend only on singleton dependencies.
But the whole chain doesn't need to be cached. What cost most is to build the root HttpClientHandler (more precisly not reusing one).
If we really need performance, it should be a dev concern and the api may provide AddCachedHttpMessageHandler. On top, we can add classic transient httpMessageHandler. The builder can forbid adding cached handler after transient one, either fluently (eg. returning a ITransientOnlyHttpClientBuilder) or by coded assertions but we really should avoid surprise at runtime.

About disposed handlers issues, shouldn't be the responsability of the developpers to create the appropriate scope to resolve from ?
I think the issue can arise when resolving a HttpClient in a request and use this instance in a new background task. The request scope will be disposed as all resolved service and HttpClient in the long running thread may be broken. It's a common issue and there is not much to do other than doc. It's the same thing for DbContext for eg.

[rynowak]

It's not a workable solution for the handlers resolved by the factory to share DI scope with the current request. You should use IHttpContextAccessor if you want get access to services in the request inside a handler.

If you have business logic concerns to handle, I would suggest writing a typed client, and putting the functionality there. That is designed to share DI scope with the current request (if you're in ASP.NET COre).

[rynowak]

You create something like what you're asking for by using the new IHttpMessageHandlerFactory if you need to create handlers that live in the same DI scope as you request. That would look something like:

// configure a client pipeline with the name "MyTypedClient"
...

// then
services.AddTransient<MyTypedClient>((s) =>
{
    var factory = s.GetRequiredService<IHttpMessageHandlerFactory>();
    var handler = factory.CreateHandler(nameof(MyTypedClient));
    
    var otherHandler = s.GetRequiredService<MyOtherHandler>();
    otherHandler.InnerHandler = handler;
    return new MyTypedClient(new HttpClient(otherHandler, disposeHandler: false));
});

You lose the ability to use the builder to configure HttpClient, but I can't think of anything else you'd miss out on.

[cortex93]

Sounds ok for me.

[rynowak]

Closing this since we provided a solution. We've already made changes to have handlers interact with DI in 2.2 to fix several other blocking issues. If you want handlers to share DI scope with the currrent request, you will need to do something like this - you can also use IHttpContextAccessor if all you want is access to the current request.