search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.27k stars 4.73k forks source link

Dependency Injection IServiceProvider Decoration #45497

Open NinoFloris opened 3 years ago

NinoFloris commented 3 years ago

Proposed API

I want to propose one api addition to MS DI which would be useful for use-cases like service decoration, debug/logging service requests, cross-wiring, child containers and scope tracking. 

public class ServiceProviderOptions
{
          ...
+        /// <summary>
+        /// Enables any service requests or IServiceProvider arguments to be delegated to the returned IServiceProvider.
+        /// </summary>
+        public Func<IServiceProvider, IServiceProvider>? ServiceProviderFactory { get; set; }
}

Naming is of course up for discussion, ServiceProviderFactory is an overloaded term but I didn't directly find any precedent for calling anything a Decorator in dotnet/runtime so I left it like this for now.

Adding this specific api at this specific spot I believe is the smallest change, has the best chance of being generally useful, and as an added bonus does not directly add to the complexity of the shared specification for all container implementations (though a cursory look through other container's implementations of IServiceProvider looks to be a regular service that can be decorated readily).

Background and Motivation

Proper child containers is the main reason for us as a company to propose this api.

For the issues around implementing child containers and more generally cross-wiring I would point to some links by @dazinator (Dotnettency author), and benfoster (Saaskit author), there is plenty more if you search google and github.

Needless to say building this right is a delicate job with any container, however MS DI with its immutable design could, with some tweaks, be the most intuitively understandable and stable implementation out there for building child containers. Having a mutable service graph adds a lot more cognitive complexity.

Moving to scope tracking, this is important for orderly disposal of the container as these may not live for the duration of the app. Our software has tenancy support just like Orchard which we like to run on MS DI, the integration with the framework, stability, immutable service graph and performance are all very good. Specifically the support of soft reload allows us to reload tenant configuration without rebooting the entire app, we keep serving requests with the old container until the soft reload is done, at which point we swap the containers.

Today after we do that swap with the old container we are forced to choose between:

The issue is we have no way of tracking active scopes (and I would find proposing an api for it questionable).

Having the ability to decorate IServiceProvider we can replace any requests for IServiceProviderFactory with a decorated version counting (increment) all CreateScope() calls, giving out decorated IServiceScope instances counting (decrement) all scope disposals. Now when our old container should be disposed we can have the last scope to dispose do so for us!

A prototype implementation of this can be found in the tests: https://github.com/NinoFloris/dotnet-runtime/blob/76fd089360e2e4fa76f05cbfa8a7c08345fd1a40/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderFactoryTests.cs#L64-L137

PR

I created a 'prototype' branch at https://github.com/NinoFloris/dotnet-runtime/tree/feature/service-provider-decorator which is complete, tested, and already handles all the fun re-entrant and concurrent cases.

I'm not ready to open a PR just yet as I want to flesh out a reliable child container implementation on top of this new api first, which will be at https://github.com/NinoFloris/dotnet-runtime/tree/example/child-container The most straightforward way to do that — until we have the ServiceDescriptor api changes — would be to cross-wire parent and child at the GetService level, this has the small downside of having a split service graph between two containers, meaning you cannot add or override a parent service dependency in a child container, but it will correctly handle any open generics and (singleton) disposal behavior.

Regardless of this proposal gaining any approval I will round off our internal child container implementation based on this work but I would definitely like to see these changes 'upstreamed' :)

ghost commented 3 years ago

Tagging subscribers to this area: @eerhardt, @maryamariyan See info in area-owners.md if you want to be subscribed.

Issue Details
## Proposed API I want to propose one api addition to MS DI which would be useful for use-cases like service decoration, debug/logging service requests, cross-wiring, child containers and scope tracking. ```diff public class ServiceProviderOptions { ... + /// + /// Enables any service requests or IServiceProvider arguments to be delegated to the returned IServiceProvider. + /// + public Func? ServiceProviderFactory { get; set; } } ``` Naming is of course up for discussion, `ServiceProviderFactory` is an overloaded term but I didn't directly find any precedent for calling anything a `Decorator` in dotnet/runtime so I left it like this for now. Adding this specific api at this specific spot I believe is the smallest change, has the best chance of being generally useful, and as an added bonus does not directly add to the complexity of the shared specification for all container implementations (though a cursory look through other container's implementations of IServiceProvider looks to be a regular service that can be decorated readily). ## Background and Motivation Proper child containers is the main reason for us as a company to propose this api. For the issues around implementing child containers and more generally cross-wiring I would point to some links by @dazinator (Dotnettency author), and benfoster (Saaskit author), there is plenty more if you search google and github. - https://github.com/saaskit/saaskit/issues/67 - https://github.com/saaskit/saaskit/issues/68 - https://github.com/dotnet/runtime/issues/41050#issuecomment-698241929 - https://github.com/dazinator/Dazinator.Extensions.DependencyInjection/tree/develop/src/DependencyInjection.ChildContainers Needless to say building this right is a delicate job with any container, however MS DI with its immutable design could, with some tweaks, be the most intuitively understandable and stable implementation out there for building child containers. Having a mutable service graph adds **a lot** more cognitive complexity. Moving to scope tracking, this is important for orderly disposal of the container as these may not live for the duration of the app. Our software has tenancy support just like Orchard which we like to run on MS DI, the integration with the framework, stability, immutable service graph and performance are all very good. Specifically the support of soft reload allows us to reload tenant configuration without rebooting the entire app, we keep serving requests with the old container until the soft reload is done, at which point we swap the containers. Today after we do that swap with the old container we are forced to choose between: - Interruption of any requests running with the old container that may still have scopes active, disposing `ServiceProvider` (note, the actual type) breaks all scopes which is unacceptable to us, it would defeat the soft part in 'soft reload'. - No disposal of the old `ServiceProvider`, meaning singletons may never be fully cleaned up, we're choosing to do this today. The issue is we have no way of tracking active scopes (and I would find proposing an api for it questionable). Having the ability to decorate `IServiceProvider` we can replace any requests for `IServiceProviderFactory` with a decorated version counting (increment) all `CreateScope()` calls, giving out decorated `IServiceScope` instances counting (decrement) all scope disposals. Now when our old container should be disposed we can have the last scope to dispose do so for us! A prototype implementation of this can be found in the tests: https://github.com/NinoFloris/dotnet-runtime/blob/76fd089360e2e4fa76f05cbfa8a7c08345fd1a40/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderFactoryTests.cs#L64-L137 ## PR I created a 'prototype' branch at https://github.com/NinoFloris/dotnet-runtime/tree/feature/service-provider-decorator which is complete, tested, and already handles all the fun re-entrant and concurrent cases, this serves a. I'm not ready to open a PR just yet as I want to flesh out a *reliable* child container implementation on top of this new api first, which will be at https://github.com/NinoFloris/dotnet-runtime/tree/example/child-container The most straightforward way to do that — until we have the [ServiceDescriptor api changes](https://github.com/dotnet/runtime/issues/41050) — would be to cross-wire parent and child at the `GetService` level, this has the small downside of having a split service graph between two containers, meaning you cannot add or override a parent service dependency in a child container, but it will correctly handle any open generics and (singleton) disposal behavior. Regardless of this proposal gaining any approval I will round off our internal child container implementation based on this work but I would definitely like to see these changes 'upstreamed' :)
Author: NinoFloris
Assignees: -
Labels: `api-suggestion`, `area-Extensions-DependencyInjection`, `untriaged`
Milestone: -
davidfowl commented 3 years ago

I think avoiding service descriptor API changes is best here so this has that going for it. The other thing I would say is that it needs to be free when this setting is null.

I also don't quite understand how this extensibility solves the things described in the issue so I'd need to see some more concrete ways this solves those problems.

dazinator commented 3 years ago

@NinoFloris I went down the same path as you - in terms of a scope tracking IServiceProvider, capable of being asynchronously disposed - that safely waits for any active scopes to become freed. I had discussions somewhere with @davidfowl at the time about this general concept, will see if I can dig it out - I couldn't sell it very well!

You can find my (working) implementation here: https://github.com/dazinator/serviceprovider-experiment/tree/master/src

I was planning on migrating that accross to Dazinator.Extensions.DependencyInjection and releasing it on nuget.

Related issue I had with the framework - that I had to workaround in my experiment above - is this whole issue of "decorating" not really being possible https://github.com/dotnet/runtime/issues/38240

Final note, if you just want to reload middleware pipeline based on a change token signalling (like options monitor or config) you might find this useful https://github.com/dazinator/Dazinator.AspNetCore.Builder.ReloadablePipeline

NinoFloris commented 3 years ago

Thanks @davidfowl that's an encouraging reply!

I'll go through it piece by piece.

I think avoiding service descriptor API changes is best here so this has that going for it.

I hope you didn't mind the length of the proposal too much ;) it was precisely because I know how hard it is (something something constrained open generics) to get any changes into this area, really glad to hear this!

The other thing I would say is that it needs to be free when this setting is null.

Understood and I wouldn't want it any other way, if you take a look at ILEmit as an example:

- // [return] ProviderScope
+ // [return] ProviderScope.ServiceProvider
  argument.Generator.Emit(OpCodes.Ldarg_1);
+ argument.Generator.Emit(OpCodes.Callvirt, ServiceProviderGetter);

https://github.com/NinoFloris/dotnet-runtime/commit/76fd089360e2e4fa76f05cbfa8a7c08345fd1a40#diff-fbaf59e20d1ca7916fb25c4e1b7a4f272024bf38fcd18d407d91dfacd14cf6a8R196-R198 https://github.com/NinoFloris/dotnet-runtime/commit/76fd089360e2e4fa76f05cbfa8a7c08345fd1a40#diff-fbaf59e20d1ca7916fb25c4e1b7a4f272024bf38fcd18d407d91dfacd14cf6a8R261

This is the case for all the others as well, there are no extra allocs and the additional logic to assign the _serviceProvider field once per scope amounts to a branch _serviceProvider = factory is null ? this : ... ; plus a null check per access of ServiceProvider (the latter would be avoidable but it needs some extra restructuring, creating a bigger diff)

I also don't quite understand how this extensibility solves the things described in the issue so I'd need to see some more concrete ways this solves those problems.

I will absolutely get to that as I agree it's critical for this proposal to go anywhere.

I'm not ready to open a PR just yet as I want to flesh out a reliable child container implementation on top of this new api first, which will be at https://github.com/NinoFloris/dotnet-runtime/tree/example/child-container

I will have an implementation you can play with and check out. I already know it would require ServiceDescriptor changes for a fully unified service graph but if you look past that it really is possible to create something good out of this. Slower due to the runtime overhead of managing two containers but it will be reliable and predictable.

NinoFloris commented 3 years ago

@dazinator thanks for the comment :) I know you battled with many of the same issues, at Crowded we started using .net core 1.0 alpha, back then we used StructureMap like Saaskit and like them we had an endless list of issues, around 2.0 we switched fully to MS DI where we are today. Our tenancy support is an implementation in F# and one day when all of this works without hacks I'd love to open source it, it's 'just plumbing' but it's really good.

We're all bumping into the same limitations, for instance your experiment doesn't (and can't of course) wrap the IServiceProvider passed into ServiceDescriptor implementation factories, meaning any of those could ask for the original IServicScopeFactory and have no trouble obtaining it. Another way would be to register a type with an IServiceProvider or IServiceScopeFactory constructor argument which would again sidestep your wrapper.

All these edge cases is exactly why I opened this proposal, it's written in such a way you can hermetically decorate anything. Just take a look at some of these tests

Final note, if you just want to reload middleware pipeline based on a change token signalling (like options monitor or config) you might find this useful

I wish life was that simple ;)

dazinator commented 3 years ago

@NinoFloris

We're all bumping into the same limitations, for instance your experiment doesn't (and can't of course) wrap the IServiceProvider passed into ServiceDescriptor implementation factories, meaning any of those could ask for the original IServicScopeFactory and have no trouble obtaining it.

Yep thats the exact hurdle I hit. I got as far as exploring the native service provider code - but props on actually working through a solution. I'd love to see this feature land. Decorating the service provider seems the only way to implement scope tracking hermetically. (P.S thank you for also introducing me to the word hermitically!)

dazinator commented 3 years ago

@NinoFloris - given the lack of movement on this, what are you thoughts on forking and publishing the modified package?

dazinator commented 3 years ago

@NinoFloris I've published this as a seperate nuget package so I could continue to use it and try it out in some other projects. Hope that's ok? It's here: https://www.nuget.org/packages/Dazinator.Extensions.DependencyInjection.Microsoft/1.1.0-PullRequest0022.76

maryamariyan commented 3 years ago

Related to https://github.com/dotnet/runtime/issues/36021