Creating a scoped service with an HttpClientFactory

[twsouthwick]

I really like the idea of HttpClientFactory, but have found issues that the convenience methods assume you have singleton instances of the services required. For instance, the following fails:

public void ConfigureServices(IServiceCollection services)
{
    services.AddScoped<Test2>(); // Works if I switch this to AddSingleton
    services.AddHttpClient<Test1>();
    services.AddMvc().SetCompatibilityVersion(CompatibilityVersion.Version_2_1);
}

public class Test1
{
   public Test1(HttpClient client, Test2 test2)
   {
   }
}

public class Test2 { }

This results in the following:

InvalidOperationException: Cannot resolve scoped service 'WebApplication1.Startup+Test2' from root provider.

Can there be a way to set the lifetime of the scope? In this example, I want to set Test1 as a singleton.

[twsouthwick]

I found that I can do the following at it starts working:

var type = typeof(ITypedHttpClientFactory<>).Assembly.DefinedTypes.Single(t => t.Name.Contains("DefaultTypedHttpClientFactory"));
services.AddScoped(typeof(ITypedHttpClientFactory<>), type);

Is this ok to do?

[martincostello]

At least for message handlers, transient is required for things to work properly. Have you tried that instead?

[twsouthwick]

So this?

var type = typeof(ITypedHttpClientFactory<>).Assembly.DefinedTypes.Single(t => t.Name.Contains("DefaultTypedHttpClientFactory"));
services.AddTransient(typeof(ITypedHttpClientFactory<>), type);

[martincostello]

What I meant was to register Test2 as a transient, rather than scoped, dependency. Alternatively it may be related to being a nested type. I’m not at a computer at the moment so I don’t have an IDE to hand to go digging.

[twsouthwick]

That allows it to be resolved, but changes the semantics of the lifetime of Test2. I need it to be scoped so that everything within that request will have the same instance.

[theezak]

Injecting handlers is however subject to a memory leak (see other issue opened on that topic). The dependency container will keep a reference to the handler because the handler implements IDisposable. That prevents the handler from being released for garbage collection. A way around it is using a container that allows you to disable IDisposable tracking (for example ExternallyOwned () on autofac).

In general, you always need to keep on mind how IDisposable is handled by the container. The .net core container will keep references, but that was not actually immediately clear from the documentation.

The cache defines its own lifetime independent of the dependency scopes. If you want the scopes to control lifetime of the handler, you can just register your own (not using httpclientfactory).

[rynowak]

Assigning myself to take a look at this.

[rynowak]

So after some discussion here, I think the right thing to do is for us to create a scope for each handler chain created by the factory. The reason is that each handler chain is its own lifetime which is independent of other lifetimes in the application (per request in a web app).

This will allow things like transients to work as expected when they outlive the request (since they are resolved from the scope). This will also avoid piling up a bunch of transient disposables in the root scope.

---

The change here will be that you can no longer use scoped services to share state outside of the handler chain - since we're creating a new scope to build the handler chain. I don't have major concerns about this because as folks have pointed out, there are lots of problems right now with mixing delegating handlers + DI right now.

Another change that isn't as easy to reason about is that when using typed clients, the typed client will be resolved from a different DI scope than the handler chain. So if you're using scoped services as a way to pass data from a typed client to a delegating handler, that would no longer be supported. I don't have a big concern about this because HttpRequestMessage.Properties is provided for that purpose.

---

If you're familiar with this area please read and digest the above changes and let me know if you have any concerns.

/cc @theezak @twsouthwick @martincostello @glennc @davidfowl

[rynowak]

@twsouthwick - reading over your post again - what does the consuming code look like? Where are you resolving Test1?

InvalidOperationException: Cannot resolve scoped service 'WebApplication1.Startup+Test2' from root provider.

This implies that you're resolving from the root provider - ie not from a scope. I'm curious about if you found some clever way to break the world.

[martincostello]

From a quick digest of the suggested change, the one thing I'd wonder about is for some monitoring code we have in some internal applications. We use a singleton class to pool sockets to send metrics to StatsD over UDP as we found this gave us a lot better throughput on a metrics and overall CPU load on the application. We wrap this singleton into a delegating handler to send metrics for out HTTP traffic to and from the applications' dependencies.

If I've understood the proposal correctly and if the scoping is changing so that these singletons are only scoped to the handler chain's lifetime (with a 2 minute default lifetime?), then we'd be losing some of the gains we've made here as we'll have a pool per handler chain instead, with them being regularly cycled. While not a world-ending change, it would be a trade-off compared to some features we're enjoying today with 2.1.

[twsouthwick]

@rynowak I don't have the original scenario (it was some customer's code) but I don't remember doing anything weird. I think it was just some scoped service being resolved within a controller. I'll see if I can repro it again.

[theezak]

The proposed solution reflects perfectly the fact that these handlers have their own lifetime. I think there is no impact on singletons. For transients and scoped you have to acknowledge that they are part of the handler pipeline and thus have a different lifetime already. If there is a breaking change here for you, then the current code is already broken.

[rynowak]

@martincostello

If I've understood the proposal correctly and if the scoping is changing so that these singletons are only scoped to the handler chain's lifetime (with a 2 minute default lifetime?), then we'd be losing some of the gains we've made here as we'll have a pool per handler chain instead, with them being regularly cycled. While not a world-ending change, it would be a trade-off compared to some features we're enjoying today with 2.1.

Singleton means singleton - key word being 'single' - scopes inherit singletons from the root. This change should have no negative impact on this scenario. I'm going to be adding tests for this kind of thing so that I make sure I'm not missing anything.

https://github.com/aspnet/DependencyInjection/blob/94b9cc9ace032f838e068702cc70ce57cc883bc7/src/DI/ServiceLookup/CallSiteRuntimeResolver.cs#L41

Lots of stuff to grok here, but resolving a singleton always resolves it in the root scope.

---

@twsouthwick - I'm going to include this sort of thing in my integration tests so that we know we understand the behavior. I'll update you if I figure something out, but as you described it, I would expect that to work.

---

@theezak

If there is a breaking change here for you, then the current code is already broken.

Thanks, that's the same conclusion I came to as well.

[brockallen]

I have a similar issue -- I have a message handler registered with AddHttpMessageHandler and it depends on something in a DbContext. I had to change my implementation of the message handler to resolve the DbContext from an IHttpContectAccessor's RequestServices due to this scoping issue.

Point is that if you have all this new http factory stuff in DI and ancillary services also in DI, then people will naturally want to depend on other services in DI (scoped or otherwise). So either the lower level bits in the http factory could be more forgiving with respect to the DI system, or everyone else's services need to be aware that they're in essence singletons and need to manually resolve stuff from DI if they need scoped stuff.

[rynowak]

@twsouthwick the issue you reported was the same as the one reported here: #134 (comment)

The ITypedClientFactory is a singleton, but it closes over the service provider (which means the application-level one). I've redesigned how this works so that it can be registered as a transient.

[rynowak]

@brockallen - if you just want to use a DbContext from inside a message handler, this fix will help you. If you need to share the DbContext's internal state with the current request, this change will not help since the scope that we're creating is not shared with the request.

Let me know if you have suggestions or need more details

[martincostello]

@rynowak I’ve not really used the scoping stuff on the service provider before, so I thought that it would create a “parallel world”, rather than inherit any singletons already created in the root. In that case, given I’ve just misunderstood the container scoping, the proposed solution sounds fine.

[IrickNcqa]

Ran into this problem with AutoMapper https://github.com/AutoMapper/AutoMapper.Extensions.Microsoft.DependencyInjection

Specifically, IMapper is registered as a scoped instance, so...

public RestServiceWithMapping(HttpClient httpClient, IMapper mapper) { }

fails as noted in the OP

[rynowak]

Fixed for 2.2.0-preview2

[marfenij]

@rynowak any hot fixes for 2.1 will be released?

[rynowak]

No, this is not currently planned to be patched for 2.1.X. This is a much bigger change than the sort of thing we usually include in a patch.

[marfenij]

@rynowak sad, but I understand that.
Any ETA for 2.2 release ? )))

[rynowak]

Soon? 😁 preview2 is out now

[marfenij]

preview it's great, but need more stable version

[andyalm]

@rynowak Do you mind explaining why the delegating handlers in the IHttpClientFactory pipeline need to have their own lifecycle? We currently utilize a few DelegatingHandler's whose job is to transparently forward some state from our main http request scope (e.g. a correlation id). Up until now, we have always built our HttpClientpipeline on every request, which allowed us to use DI to inject these cross-cutting DelegatingHandler's without every component that uses HttpClient needing to worry about them. The obvious downside to this approach is that we weren't getting the benefits of connection pooling. I was hoping that we would be able to just plug in IHttpClientFactory to achieve a performance boost, but was disappointed to see that we could no longer attach request scoped data to our DelegatingHandler's.

I understand why the primary HttpMessageHandler needs to have its own lifecycle (that's how you achieve the performance benefit), but I'm confused on why the DelegatingHandler's also need to have their own lifecycle. Can you enlighten me?

[rynowak]

I understand why the primary HttpMessageHandler needs to have its own lifecycle (that's how you achieve the performance benefit), but I'm confused on why the DelegatingHandler's also need to have their own lifecycle. Can you enlighten me?

The real answer, because this is how we intended for it to work and how we coded it up. We didn't intend for message handlers to be stateful or share state with the 'current request'. It works when used with a typed client, and doesn't work if you call the factory directly.

You could imagine a more complicated design where some handlers were managed by the factory and some not, and then you run into some really practical problems:

1. DelegatingHandler always disposes Inner https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/src/System/Net/Http/DelegatingHandler.cs#L64
2. DelegatingHandler does not allow you to set Inner to another value onces its been used for a request

So you run into some limitations right away.

• We have to put a firewall between everything 'user' and the primary handler to prevent it from being disposed. That's not a huge problem, we could solve it with code.
• All of your handlers have to be transient because they will have to be reused. That's not a big problem, you would see an error if you registered a handler as non-transient.

These aren't big problems that cannot be solved. We'd have to introduce yet another interface/abstraction, because the IHttpClientFactory doesn't accept a service provider as a parameter, and it would have to. We would end up continuing the quirk that typed clients work differently than calling the factory directly.

---

This isn't necessarily the final word on this. The changes we're doing in 2.2 solve a set of real problems that are totally blocking. We could consider a more involved design that creates handlers from the current scope in the future if it's really really important - but I want to understand the problems that people have right now.

I'm really more interested at this point in knowing more about what your handlers are, and what problems you use to solve them. We think that there's a common set of features that everyone wants, and we're interested in providing them.

Anytime someone asks me a question like "why can't I use this thing in this way", what I want to know is "what problem are you trying to solve in the first place".

[rynowak]

There was a similar discussion here: #166 with a suggestion for a workaround.

[andyalm]

Thanks for responding @rynowak. In terms of the problem we are trying to solve, we have a suite of internal http api's that all take a standard set of headers for cross cutting concerns like authentication, logging/monitoring, etc. Our preference is that when we are coding our clients, these cross cutting concerns are taken care of transparently without needing to worry about them in every place that we make an http call. Some of the values of these headers are based on data specific to the current request. A simple example is that we have a X-Correlation-Id header that we want to forward to all of our service calls. Its value comes from the current request (and is logged with the current request). We have always used a factory for our HttpClient which would add DelegatingHandler's scoped to the current request to the pipeline. We have a CorrelationIdHandler, for example, which takes the CorrelationId for the current request into its constructor and adds the X-Correlation-Id to all outgoing requests. This correlation id allows us to trace requests through the entire stack in our logs. When I tried to update our home grown HttpClient factories to make use of (or be replaced by) IHttpClientFactory, I was disappointed to find out there was no way for me to inject the CorrelationIdHandler and actually have it work anymore. The ugly workarounds I can figure out are the use IHttpContextAccessor inside my DelegatingHandler, or resort to private reflection to pull out the inner _handler from the HttpClient and wrap the DelegatingHandler's around that, before then creating my own HttpClient around it all. I'm not thrilled about either of those options.

You have suggested a couple options that could address my concerns (like providing a CreateClient that takes an IServiceProvider. I thought I would throw out one additional possibility for your consideration:

• Provide a way for getting access directly to the pooled HttpMessageHandler rather than only a fully built out HttpClient pipeline. DefaultHttpClientFactory's primary value/complexity IMHO is in managing the lifecycle of the pooled handlers, yet that handler is not available via any public or even protected API right now. Providing a way to access that without the opinionated handler management around it would provide a lot of additional flexibility. The decorator pattern can be powerful, and one of the things that makes it powerful is its dynamism. Forcing the entire decorator pipeline to be built/declared "up front" and in its own lifecycle takes a way a lot of its power IMHO.

[martincostello]

Getting access to the underlying HttpMessageHandler should be enabled by the changes coming in 2.2.0 from #118.

[andyalm]

Thanks @martincostello. I was just looking through master and saw IHttpMessageHandlerFactory. That is great and will give the flexibility I need! Thanks!

[glennc]

@andyalm HttpClient has a property that controls timeout of the underlying connection. If the only value you see in the factory is the ability to periodically recycle a connection then you can set that and call life good. But I would've thought that there's more value than that given that you are presumably going to write a bunch of code similar to the factory once you start using that, to stitch together your handlers and maybe do something like a typed client. Do you see value in typed clients?

[andyalm]

@glennc What property of HttpClient are you referring to? While I have certainly written some code similar to the factory, its not all that complex or interesting (just newing up some objects in a chain), so its not very burdensome and it solves my problem of needing request scoped DelegatingHandler's. On the other hand, duplicating/replicating the relatively complex lifecycle management of the primary handler is not something I would want to take on myself if I can avoid it (and reading through that part of DefaultHttpClientFactory code confirms that).

As far as typed clients go, yes, I absolutely see value in typed clients and think they were a great addition. While the typed clients themselves can be request scoped, which is great, the DelegatingHandlers in their pipelines cannot be request scoped, so they have the same limitations. As I mentioned above, we have a number of internal services which all have the same cross-cutting headers that we want passed on every request, so I want to be able to ensure that behavior is added without needing to put boilerplate code in every typed client we build.

Having the IHttpMessageHandlerFactory exposed in the next version addresses my primary concern here. It allows us to get access to the pooled handler instances while still having the flexibility to layer in DelegatingHandlers with whatever lifecycle we see fit.