Dependency Injection of Open Generics via factory

wvpm commented 4 years ago

Background and Motivation

When using the Microsoft.Extensions.DependencyInjection's ServiceviceDescriptor to register a service, you can register an instance, factory or implementationtype. This is great and generally works fine. However, when registering an open generic type, you can only use an implementationtype. Any other variant will cause runtime errors.

Proposed API

I propose adding a property to the ServiceDescriptor, along with a constructor to set the property like this:

namespace Microsoft.Extensions.DependencyInjection {
    public class ServiceDescriptor
    {
+    public Func<IServiceProvider, Type, object>? TypedImplementationFactory { get; }
+    /// <summary>
+    /// Initializes a new instance of <see cref="ServiceDescriptor"/> with the specified <paramref name="factory"/>.
+    /// </summary>
+    /// <param name="serviceType">The <see cref="Type"/> of the service.</param>
+    /// <param name="factory">A factory used for creating service instances. The type parameter is the requested serviceType.</param>
+    /// <param name="lifetime">The <see cref="ServiceLifetime"/> of the service.</param>
+    public ServiceDescriptor(
+        Type serviceType,
+        Func<IServiceProvider, Type, object> factory,
+        ServiceLifetime lifetime)
+        : this(serviceType, lifetime)
+    {
+        if (serviceType == null)
+        {
+            throw new ArgumentNullException(nameof(serviceType));
+        }
+    
+        if (factory == null)
+        {
+            throw new ArgumentNullException(nameof(factory));
+        }
+    
+        TypedImplementationFactory = factory;
+        ImplementationFactory = provider => factory(provider, serviceType);
+     }

Usage Examples

You would be able to use it when adding DI registrations as follows:

IServiceCollection serviceCollection = new ServiceCollection();
ServiceDescriptor descriptor = new (
     typeof(IGenericService<>),
     (IServiceProvider provider, Type requestedType) =>
          provider.GetRequiredService<IGenericServiceFactory>()
          .Build(requestedType),
     ServiceLifetime.Singleton
);
serviceCollection.Add(descriptor);

Where IGenericService<> and IGenericServiceFactory would be custom interfaces not part of the dotnet runtime code. IGenericServiceFactory.Build(Type) would of course return an instance of that type.

Alternative Designs

I could not think of any other good solution. A bad alternative solution required emitting dynamic wrapper classes.

Risks

I foresee two risks and two concerns regarding extra work due to obsolete code.

Risks:

ServiceProviders would have to be edited to provide support for the TypedImplementationFactory. If a ServiceProvider does not support this, you would only find this out runtime possibly by a nullref, depending on implementation.
The IServiceCollectionExtensions should be expanded with overloads for the TypedImplementationFactory, which might confuse programmers.

Extra work concerns, If the IServiceCollectionExtensions and ServiceDescriptor constructor using the ImplementationFactory were tagged as obsolete:

Programmers using Microsoft.Extensions.DependencyInjection should update their code, resulting in extra work.
The obsolete code should eventually be removed from Microsoft.Extensions.DependencyInjection, resulting in extra work.

ghost commented 4 years ago

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

wvpm commented 4 years ago

~~I lack the rights to push a branch to this repository, so I cannot create a pull request myself.~~ I did work out an implementation in the CallSiteFactory which is used to actually resolve services. I'll create a fork later to show the code changes.

        private ServiceCallSite TryCreateExact(ServiceDescriptor descriptor, Type serviceType, CallSiteChain callSiteChain, int slot)
        {
            if (serviceType == descriptor.ServiceType)
            {
                ServiceCallSite callSite;
                var lifetime = new ResultCache(descriptor.Lifetime, serviceType, slot);
                if (descriptor.ImplementationInstance != null)
                {
                    callSite = new ConstantCallSite(descriptor.ServiceType, descriptor.ImplementationInstance);
                }
+                else if (descriptor.TypedImplementationFactory != null)
+                {
+                    callSite = new FactoryCallSite(lifetime, descriptor.ServiceType, provider => descriptor.TypedImplementationFactory(provider, serviceType));
+                }
                else if (descriptor.ImplementationFactory != null)
                {
                    callSite = new FactoryCallSite(lifetime, descriptor.ServiceType, descriptor.ImplementationFactory);
                }
                else if (descriptor.ImplementationType != null)
                {
                    callSite = CreateConstructorCallSite(lifetime, descriptor.ServiceType, descriptor.ImplementationType, callSiteChain);
                }
                else
                {
                    throw new InvalidOperationException("Invalid service descriptor");
                }

                return callSite;
            }

            return null;
        }

        private ServiceCallSite TryCreateOpenGeneric(ServiceDescriptor descriptor, Type serviceType, CallSiteChain callSiteChain, int slot, bool throwOnConstraintViolation)
        {
            if (serviceType.IsConstructedGenericType &&
                serviceType.GetGenericTypeDefinition() == descriptor.ServiceType)
            {
+                var lifetime = new ResultCache(descriptor.Lifetime, serviceType, slot);
+                if (descriptor.TypedImplementationFactory != null)
+                {
+                    Type closedServiceType;
+                    try
+                    {
+                        closedServiceType = descriptor.ServiceType.MakeGenericType(serviceType.GenericTypeArguments);
+                    }
+                    catch (ArgumentException)
+                    {
+                        if (throwOnConstraintViolation)
+                        {
+                            throw;
+                        }
+
+                        return null;
+                    }
+                    return new FactoryCallSite(lifetime, descriptor.ServiceType, provider =>  descriptor.TypedImplementationFactory(provider, closedServiceType));
+                }
+
                Debug.Assert(descriptor.ImplementationType != null, "descriptor.ImplementationType != null");
-                var lifetime = new ResultCache(descriptor.Lifetime, serviceType, slot);
                Type closedType;
                try
                {
                    closedType = descriptor.ImplementationType.MakeGenericType(serviceType.GenericTypeArguments);
                }
                catch (ArgumentException)
                {
                    if (throwOnConstraintViolation)
                    {
                        throw;
                    }

                    return null;
                }

                return CreateConstructorCallSite(lifetime, serviceType, closedType, callSiteChain);
            }

            return null;
        }

eerhardt commented 4 years ago

I lack the rights to push a branch to this repository, so I cannot create a pull request myself.

You don't need push rights to make a pull request. You can fork the repo yourself, and make a PR from your fork.

However, since this is proposing a new API you should wait until the API is reviewed and approved before creating a PR. See https://github.com/dotnet/runtime/blob/master/docs/project/api-review-process.md for more info.

carusology commented 4 years ago

Unless the stance of Microsoft here, here or here changed, this isn't happening.

A good summary statement from @pakrym (Source):

We haven't broken other service provider implementation since 1.0 and this feature is certainly not worth it. What it's trying to accomplish already exists in other providers

While it's not a breaking change for consumers, they don't want a breaking change for other DI implementers who would need to support the new typed implementation factory API. You correctly identified this problem in your "Risks" section.

For reference, it's still fairly popular question on StackOverflow with no great answer.

wvpm commented 4 years ago

Thanks @carusology for linking those. I couldn't find any related issues within this github and the stackoverflow post reveals the open wound. I could imagine that with .Net 5 Microsoft could consider real API changes. When they made .Net Core they changed a lot also. If they don't want to change the API for this feature specifically, perhaps they can bundle it with other features. It would be sad if the API is stuck in place due to implementers having to add features. That sounds very similar to ECMAScript '99-2015.

If I look at the before and after for implementers that do not support this feature, it would be runtime exception vs runtime exception. Where you just have to know that open generics can only be registered with an open generic implementation. Seems to me there is not that much to lose.

davidfowl commented 4 years ago

.NET 5 is done, we can look at something for .NET 6 but we're not making any breaking changes in this space regardless. On top of that, any new requirement to the DI container abstraction needs to be implemented in all existing supported containers, that is the minimum bar for new features.

That said, I like this feature 😄

wvpm commented 4 years ago

Good to hear .NET 5 is done.

I added a fallback option in the proposed API constructor. The TypedImplementationFactory variant now also sets the ImplementationFactory. This way, providers that don't support the TypedImplementationFactory can continue to use the ImplementationFactory. So if I use the TypedImplementationFactory constructor and my provider does not support it, it will work if the serviceType is not an open generic (as before). It will throw runtime exceptions with open generics (as before). So if a provider does not change anything, it will work the same as before. This allows providers to implement it at their pace.

If for some reason the ServiceDescriptor API may not be altered because not all implementors support it and the implementors depend on a TypedImplementationFactory, then you have a chicken-egg issue. So I hope dotnet takes the initiative and alters the API is a non-breaking way, so that the implementors can use the new API to update their code.

davidfowl commented 4 years ago

That's not how changes to the DI system work. You need to take the initiative and see how the supported containers behave and how difficult this support would be to implement. When the rubber meets the road, we actually tests all known implementations as part of success criteria so it's not something we would commit then figure out.

We can mention the various authors to get some other opinions

dazinator commented 4 years ago

I am creating a library to provide a "child container" feature for the native MS ServiceProvider but I have hit this problem with respect to singleton open generic registrations . I just cannot solve it in a decent way, precisely due to this issue outlined. I've worked around it, by giving the user some options around how these registrations are dealt with when they are encountered, none of which are ideal. For example one option is to just throw when these descriptors are encountered - and there are usually atleast the following added by the Hosting layer:

Dazinator.Extensions.DependencyInjection.Tests.ChildServiceProvider.GenericHostTests.Options_WorksInChildContainers(args: [""])
   Source: GenericHost.cs line 23
   Duration: 269 ms

 > Message: 
    System.NotSupportedException : Open generic types registered as singletons in the parent container are not supported when using child containers: 
    ServiceType: Microsoft.Extensions.Options.IOptions`1
    ServiceType: Microsoft.Extensions.Options.IOptionsMonitor`1
    ServiceType: Microsoft.Extensions.Options.IOptionsMonitorCache`1
    ServiceType: Microsoft.Extensions.Logging.ILogger`1
    ServiceType: Microsoft.Extensions.Logging.Configuration.ILoggerProviderConfiguration`1

The issue (as already described) is that when a singleton open generic descriptor is registered, there is no ability to specify that you want the instance to be provided from "elsewhere". This is a problem for the parent --> child relationship I was aiming to achieve! In my case the child container needs to be able to resolve a singleton open generic from the parent container so it gets the same instance.

This TypedImplementationFactory would only partly solve the problem - enabling me to do as @wvpm suggested:

IServiceProvider parentServiceProvider = GetParentServiceProvider();
IServiceCollection childServices= GetChildServiceCollection();

// for each of the singleton open generic descriptors registered in the parent, add them to the child but re-point the instance to be sourced from the parent container as we want to keep it a true singleton. 
childServices.Add(new ServiceDescriptor(parentDescriptor.ServiceType , (IServiceProvider provider, Type requestedType) => parentServiceProvider.GetRequiredService(requestedType))

I say partly, because it doesn't address the last option that we have with closed type singleton registrations - the ability when the instance is provided to prevent the container from owning / tracking / disposing of that instance. When I ultimately provide a singleton instance that I have created "externally" (in my case via the parent container,) I want to hint to the container that this singleton instance is "external" like I can with closed type singleton registrations when providing an instance. Please - if you do decide to make the changes necessary to solve this issue, please can you also go the whole hog and add this option? @wpvm do you have any thoughts on what that api might look like - i.e to indicate its an "external" instance that shouldn't be tracked for disposal?

dazinator commented 4 years ago

We can mention the various authors to get some other opinions

@tillig @jeremydmiller would be very greatful for any opinions you may like to share on this as well as any other container authors!

tillig commented 4 years ago

This is not something Autofac provides out of the box. I think it'd be possible, but it doesn't exist today.

That said, I haven't seen an issue come through for something like this for Autofac, though I have seen a couple of StackOverflow questions over the years. I have to assume that if it was a popular enough feature that enough people wanted we'd have seen an issue come in by now. I could be wrong.

From a container developer perspective, if a new "registration type" gets added to the system, it's a breaking interface change. The way things work, at least in Autofac, we iterate through the set of registrations in the IServiceCollection and effectively "convert" them to Autofac registrations. If there's a new property added to the registration, that means one of two things happens:

The property isn't looked at and folks wonder why that registration type isn't handled.
The property is looked at and we now have to maintain different code (maybe different packages?) based on whether the Autofac/MS DI adapter package is being used with old registrations or new ones.

My understanding of the Microsoft DI container is that it was intended to be sort of a lowest-common-denominator conforming container, enough to support the use cases for ASP.NET Core; and if you want more "advanced features," that was the point of being able to use other backing containers. Sort of like the difference between System.Text.Json and Newtonsoft.Json - System.Text.Json is there, it does most of what you need, but it isn't as fully-featured as Newtonsoft.Json and isn't intended to be. If you want the extra features, you opt in to use Newtonsoft.Json to get them rather than having every feature implemented in System.Text.Json.

So... I think the feature is interesting, for some folks it is probably valuable, but I don't think it should be added to the MS DI container. I think the MS DI container should stay as basic as possible to allow easier conformance. I'd say if someone wants to use a feature like this, it could be in a more fully-featured backing container.

jeremydmiller commented 4 years ago

This won't help you at all, but the equivalent functionality was buggy in StructureMap and I've held the line of not wanting to have to support this functionality in Lamar. It's just a very complex problem with oodles of edge cases.

dazinator commented 4 years ago

@tillig @jeremydmiller thanks both for responding!

I understand your point @jeremydmiller - child containers are certainly not something for everyone, it's complicated enough for consumers, let alone authors. Saying that, they do solve some problems architecturally in a nice way and so have their uses. I can see why some DI containers would want to support the notion and not others.

.. but actually you should be supporting me in this @jeremydmiller - with a couple of very minor tweaks to ServiceDescriptor, it would be possible to populate two independent containers in a way that results in a parent - child relationship, without the underlying container author having to do anything special - just support population of their container from IEnumerable<ServiceDescriptor>(). The only change needed for this to be a reality is to extend ServiceDescriptor with the two tweaks mentioned in this issue:

For open generics, allow a factory delegate to provide the instance
For open generics, allow a boolean hint as to whether the container should own the instance that is provided by the above.

I appreciate @tillig that this may seem an advanced feature that you should be swapping containers for - and I take your point about not seeing much demand for it - although you do support ExternallyOwned for open generic registrations - so from that perspective there must be some demand for that aspect. However from my perspective, ServiceDescriptor is just a descriptor - and as such, should be able to describe the registration requirement - it's down to the underlying DI container to decide (like it currently does) if it wants to throw a "NotSupportedException". Perhaps (based on your feedback) there should be a better way for DI containers to be able detect unsupported service descriptors if changes are made in future - so like you say, you don't have to contain so many conditional compilations?

There is also another point purely from aesthetical point of view. Closed types can currently be described / registered as singleton "externally" owned instances, I am just seeking Feature parity for open generic registrations. Why is it a valid requirement for one and not the other?

tillig commented 4 years ago

but ServiceDescriptor is just a descriptor - all I am asking is that it should be able to describe the registration requirement ... it's down to the underlying DI container to decide if it wants to throw a "NotSupportedException"

That's not how it works, unfortunately. In order to say you support the MS DI abstraction, you have to support all the things. There's no NotSupportedException option. There are even specification tests the backing container has to pass. If something gets added to the base DI container, all the conforming containers have to support it.

ExternallyOwned has nothing to do with open generics or specifically supporting that. ExternallyOwned is about whether, once you resolve a concrete closed generic, the Autofac lifetime scope tracks and manages disposal. It's an opt-out. It doesn't participate in the actual resolution/construction of the thing.

I'll again reiterate, if something gets added, it's effectively a breaking change. There's not a way to say "this Autofac adapter package targets netstandard2.0 and should use this code if you're referencing the old MS.DI abstraction but this other code if you're referencing the new one." It's an interface, just like ILogger<T> or anything else. It seems like a simple thing to add, but there are a ton of downstream ripple effects.

It's exactly for these reasons that there's a separation between ConfigureServices and ConfigureContainer in a Startup class. If you can stick to the common abstractions, great. If not, ConfigureContainer.

Anyway, I've kinda said my piece here. I think it might be an interesting feature for a backing container, I definitely don't think it belongs in the MS.DI container. There's not much more I can contribute on the subject.

jeremydmiller commented 4 years ago

I'm going to ditto what @tillig said about this being a feature for a specific backing container. The configuration part of this isn't that bad, it's the runtime that's going to get you. The rules about what the client container can override or carry through from the parent container is rough, not disposing objects created from the parent, and every user having a little different idea about exactly how the child container feature should work.

As a somewhat active maintainer of one of the very many IoC containers that conform to the ASP.Net DI standard, I'd strongly prefer that that standard basically never change, or at least very, very rarely.

dazinator commented 4 years ago

Thanks @tillig

I'll again reiterate, if something gets added, it's effectively a breaking change. There's not a way to say "this Autofac adapter package targets netstandard2.0 and should use this code if you're referencing the old MS.DI abstraction but this other code if you're referencing the new one." It's an interface, just like ILogger or anything else. It seems like a simple thing to add, but there are a ton of downstream ripple effects.

Thats a pain isn't it! Do you think this warrants a seperate API proposal (and breaking change) in some attempt at addressing? Or is it not worth it? E.g some mechanism to allow the container to detect servicedescriptors that it can't process safely at runtime?

dazinator commented 4 years ago

@jeremydmiller - ditto, I also hope breaking changes are always very few and very far between! :-)

dazinator commented 4 years ago

I think me mentioning "child containers" in this discussion may have side tracked it slightly - apologies for that @wvpm ! The issue really has a much more limited scope which has already been summarised nicely, and getting into the complexities of child containers is probably not helpful on my part.

dazinator commented 4 years ago

@wvpm

I could not think of any other good solution.

Given there is a lot of pushback on any proposed changes to ServiceDescriptor (and for good reasons) - I wonder if an alternative might be for the consumer to register some sort of provider globally with the microsoft service provider directly (on app startup somewhere), that the service provider will then delegate to (if set) when it needs to create a new instance from an open generic registration. If its not set, it will just do its normal type resolution it does today. In this way there would not be any new property added to ServiceDescriptor, the descriptor would still just have the ordinary ServiceType and ImplementationtType set.

The good thing about that (aside from no changes to service descriptor) would be that this function would be a container feature added to microsoft service provider only, and not expected to be implemented by all other containers.

I haven't looked at the code yet, but if it looks viable, might submit an API proposal for that type of change.

davidfowl commented 4 years ago

@dazinator I agree that child containers shouldn't be discussed on this issue.

But generally for issues like this we can do in 2 directions:

Expose another builder on top of the ServiceCollection that's specific to the built in container. That puts you in the category of being tied to a specific container implementation.
Enhance the abstraction. Getting container authors on board to implement more functionality in the spec. Even when this is a breaking change it is possible to do this in newer versions in practice (especially if those scenarios were failing before).

The first requires a bunch of ground work to design a new set of registration APIs (possibly only new ones?) and the second requires looking at all of the existing tests and container and getting agreement and consensus with the current container authors.

wvpm commented 4 years ago

@dazinator, @tillig & @jeremydmiller Thanks for your input. From what I understand this proposal creates new questions for some implementors in regards to child containers.

To make sure we talk about the same specifics, I added the following code:

//A. Registration of closed type via factory
services.AddSingleton(typeof(IService), (provider) => new implementation());
//B. Registration of open type
services.AddSingleton(typeof(IGenericService<>), typeof(Implementation<>));
//C. Registration of open type via factory
services.AddSingleton(typeof(IGenericService<>), (IServiceProvider provider, Type requestedType) => provider.GetRequiredService<IGenericServiceFactory>().Build(requestedType));

It is my understanding (based on the Microsoft implementation) that a singleton is maintained in the root container and shared with all descendants. I don't see any difference here between the closed type via factory (A), open generic type (B) and open type via factory registrations (C).

Could either one of you explain to me the crucial difference between these registration types?

dazinator commented 4 years ago

@wvpm

Thanks for your input. From what I understand this proposal creates new questions for some implementors in regards to child containers.

As per above, I think we should forget child containers were ever mentioned, the issue isn't really to do with them.

It is my understanding (based on the Microsoft implementation) that a singleton is maintained in the root container and shared with all descendants. I don't see any difference here between the closed type via factory (A), open generic type (B) and open type via factory registrations (C).

You've missed one there! We can also register a singleton (closed type) instance. In which case the instance won't be owned / disposed by the container when the container is disposed:

services.AddSingleton(fooInstance);

So to try and summarise the feature gap for open generic registrations vs closed type registrations:

closed types can have a factory method (container will still own / dispose of them), open types can't (this is what you are trying to address)
An instance of a closed type can be registered as a singleton and the container won't own / dispose of it. No way to do that for instances created from open type registrations. (This API proposal doesn't try to address that)

I think the API proposal would ideally resolve both of the above!

dazinator commented 4 years ago

@davidfowl

Expose another builder on top of the ServiceCollection that's specific to the built in container. That puts you in the category of being tied to a specific container implementation.

I think this would be great. If this could be addressed purely as native ServiceProvider / ServiceCollection feature, i.e some api's to bring open type registrations up to par with what you can currently do with closed type registrations then I'd be down with that. It won't break any other containers this way.

wvpm commented 4 years ago

@dazinator I know the singleton closed type instance registration, but did not include it in my post since it's a standalone case in my opinion. It's quite different compared to the other registrations in that the DI implementation only passes it on to a consumer. It is not managed at all beyond that. My proposal (in the original post) is about using a function to resolve a closed type at runtime that inherits from an open generic type (B). I think this should function very similar to the current implementation of closed type factory registration (A) and the open type generic registration (C). See my previous post for a code sample for A, B and C.

From what I've read there are 3 'issues':

child container behaviour (which we'd best leave out of this thread)
externally owned singletons (which I think are not that relevant to my proposal)
implementors would have to edit their implementation, else MS DI would no longer be the lowest common denominator.

I think issue 3 basically comes down to "Is MS DI set in stone?". If the API may not be edited now or in the future, then the whole proposal can be rejected at once. If however, changes are possible, we could talk about how other implementors could update their implementation to support this API change and if that effort is worth it.

Since there is interest and are stackoverflow post asking for this, I argue that it is worth the effort (atleast for MS DI).

davidfowl commented 4 years ago

I think this would be great. If this could be addressed purely as native ServiceProvider / ServiceCollection feature, i.e some api's to bring open type registrations up to par with what you can currently do with closed type registrations then I'd be down with that. It won't break any other containers this way.

It begs the question as to why you don't just use the other containers that support this then.

dazinator commented 4 years ago

@davidfowl

It begs the question as to why you don't just use the other containers that support this then.

My library is specifically aiming to extend / leverage the native service provider to allow it to be used for child containers in applications. Its ok for me to be coupled with the native service provider- this is by design and is my goal.
Not all other containers currently support these options for open generic registrations as per discussion above - I havent looked at which ones do and don't - but purely from a native service provider perspective, I think making these additional registration options available for open generics would fill a gap (from whats available for closed types) that would then just so happen to be very useful for my scenarios.

To pre-empt a possible next question - why do I want to create such a project? Why am I not just switching to a new container already!

I like the native service provider and think it does a reasonable job perf wise. If I can avoid switching in my applications I will do. Child containers is the only feature that has ever caused me to switch away. I was hoping to do something about that.

MithrilMan commented 4 years ago

mmm ok I was trying to achieve this kind of child service provider because I've the need to run different Hosts that share the same DI resolutions. I tried to have a child serviceprovider implementation basically solving the child using the parent sp but of course I can't proceed because of this open generic problem. My only chance now would be to be able to reuse the same serviceprovider in different Hosts but even this doesn't seems possible, am I missing something? (e.g. being able to set host.Services property passing the main serviceProvider instance)

davidfowl commented 4 years ago

You can plug in a service provider into the host via an ISericeProviderFactory https://docs.microsoft.com/en-us/dotnet/api/microsoft.extensions.hosting.hostbuilder.useserviceproviderfactory?view=dotnet-plat-ext-5.0

MithrilMan commented 4 years ago

Hey @davidfowl I tried that approach, my problem is that I'm trying to run multiple Hosts using the same serviceprovider and actually the main serviceprovider is built before other hosts are configured so I can't add services to it. I tried to implement a kind of aggregator to simulate a child service provider but failed (a Iserviceprovider implementation that first tries to solve with the parent, then with the child serviceprovider, but I had too many problems and at the end it couldn't work if you try to resolve a service that have constructor parameters that has to be solved some on the parent container and some on the child container) I opened a discussion https://github.com/dotnet/aspnetcore/discussions/28228 to see what's the best way to achieve what I'm trying to achieve

I may be able to alter the design and delay the building of the main host in order to register everything in the same HostBuilder, exposing different APIs on different endpoints with swagger, eventually adding blazor server, etc... but I doubt it's feasible, if it is I'd like to know how

dazinator commented 4 years ago

Hey @davidfowl I tried that approach, my problem is that I'm trying to run multiple Hosts using the same serviceprovider and actually the main serviceprovider is built before other hosts are configured so I can't add services to it. I tried to implement a kind of aggregator to simulate a child service provider but failed (a Iserviceprovider implementation that first tries to solve with the parent, then with the child serviceprovider, but I had too many problems and at the end it couldn't work if you try to resolve a service that have constructor parameters that has to be solved some on the parent container and some on the child container) I opened a discussion dotnet/aspnetcore#28228 to see what's the best way to achieve what I'm trying to achieve

I may be able to alter the design and delay the building of the main host in order to register everything in the same HostBuilder, exposing different APIs on different endpoints with swagger, eventually adding blazor server, etc... but I doubt it's feasible, if it is I'd like to know how

Have you looked at the Child containers provided by https://github.com/dazinator/Dazinator.Extensions.DependencyInjection - it has some tests - open generic registrations for singletons can result in duplicate singleton in the child container though (there are options for how you address this).

MithrilMan commented 4 years ago

I saw your project but didnt't tried it because it actually has same limitation of my implementation e.g. in my hacking attempt , in ConfigureServices of my 2nd host (built after the main was already running) I passed the main serviceprovider and its servicecollection and did this

// copies all the services registered in the forge, maintaining eventual singleton instances
// also copies over singleton instances already defined
foreach (ServiceDescriptor service in _registeredServices)
{
   if (service.ServiceType == typeof(IHostedService))
   {
      //prevent to start again an hosted service that's already running
      continue;
   }
   // open types failure, adding as service wull resolve to a new instance
   else if (service.ServiceType.IsGenericType)
   {
      services.Add(service);
   }
   else if (service.Lifetime == ServiceLifetime.Singleton)
   {
      services.AddSingleton(service.ServiceType, sp => _serviceProvider.GetServices(service.ServiceType).First(s => service.ImplementationType == null || s.GetType() == service.ImplementationType)); //resolve singletons from the main provider
   }
   else
   {
      services.Add(service);
   }
}

that gave me the false illusion it was working but (I was serving singleton by using the main _serviceProvider as wrapper), as you may guess, it generates duplicate IOptions for instance or other glitches if GetService on an open generic was accepting a factory it would work tho :/

dazinator commented 4 years ago

I saw your project but didnt't tried it because it actually has same limitation of my implementation e.g. in my hacking attempt , in ConfigureServices of my 2nd host (built after the main was already running) I passed the main serviceprovider and its servicecollection and did this
// copies all the services registered in the forge, maintaining eventual singleton instances
// also copies over singleton instances already defined
foreach (ServiceDescriptor service in _registeredServices)
{
   if (service.ServiceType == typeof(IHostedService))
   {
      //prevent to start again an hosted service that's already running
      continue;
   }
   // open types failure, adding as service wull resolve to a new instance
   else if (service.ServiceType.IsGenericType)
   {
      services.Add(service);
   }
   else if (service.Lifetime == ServiceLifetime.Singleton)
   {
      services.AddSingleton(service.ServiceType, sp => _serviceProvider.GetServices(service.ServiceType).First(s => service.ImplementationType == null || s.GetType() == service.ImplementationType)); //resolve singletons from the main provider
   }
   else
   {
      services.Add(service);
   }
}
that gave me the false illusion it was working but (I was serving singleton by using the main _serviceProvider as wrapper), as you may guess, it generates duplicate IOptions for instance or other glitches if GetService on an open generic was accepting a factory it would work tho :/

Ah right. Yep you've hit the same limitation as I did :-). I dont think a resolution will be possible unless something like what @davidfowl suggested is added specifically for the native service provider to allow the construction of open generic type instances to be delegated to a custom factory set by the user:

Expose another builder on top of the ServiceCollection that's specific to the built in container. That puts you in the category of being tied to a specific container implementation.

This small additional feature being added to the native provider, and arguably useful in its own right, would then unlock a larger additional feature being provided for the native service provider, through community packages (or app specific implementations) namely - child container behaviour - giving some users less of a need to switch away from the native service provider - when they have been generally happy with its performance, with the caveat that usage of such "advanced" features may tie you more strongly to the native provider and make it more difficult for you to switch (if you wanted too) in future.

NinoFloris commented 4 years ago

As for the ServiceDescriptor api changes, I would strongly recommend while doing these changes to also add the concept of ownership with a flag like ExternalOwnership (somewhat mirrorring ExternallyOwned() in AutoFac), this would make cross wiring at the descriptor level (for a unified and overridable service graph) much more feasible.

If it would require another api proposal to do so we should.

dazinator commented 3 years ago

@MithrilMan I've addressed the "singleton open generic" type registration issue now, so that when resolving from the child service provider, if the type being resolved matches an open generic registration that is registered at parent (and not child) level - the request is forwarded to the parent service provider for resolution - so you get the same singleton instance. It's over here if you wanted to try it out. It does add a tiny bit of overhead when resolving services from a child container, but I've tried to keep that as small as possible, and it's the best I can come up with given current status of api's involved.

MithrilMan commented 3 years ago

@dazinator thanks, I've solved for my use case but I'll take a look.

AKA285066112 commented 3 years ago

This is the cause of .net core self-death

davidfowl commented 3 years ago

This one bug Doubtful... There are also ways to do this on .NET Core with other DI containers.

dazinator commented 3 years ago

The only workaround I could find was to:-

Examine the ServiceDescriptors before the service provider is built, and harvest Open Type Generic descriptors.
Use the information gleaned in step 1, to construct a decorator for the original IServiceProvider, that intercepts GetService requests and identifies if the service being requested relates to an open generic type descriptor. The result of this can be lazily cached by type for a perf boost. to avoid checking each time. If it is, have the decorator construct or return the instance from wherever you need it to.
Swap out the original IServiceProvider usage in your app for the decorated one. This is not as easy as it should be and led me to other issues. YMMV

I use this approach here with tests here

retterbot commented 2 years ago

@tillig this is something that Autofac does out of the box, the RegisterGeneric factory delegate has a types parameter. But just knowing the requested type would be enough.

https://autofac.readthedocs.io/en/latest/register/registration.html#open-generic-components

dazinator commented 2 years ago

@tillig just a minor point of clarification:

ExternallyOwned has nothing to do with open generics or specifically supporting that. ExternallyOwned is about whether, once you resolve a concrete closed generic, the Autofac lifetime scope tracks and manages disposal. It's an opt-out.

I do understand the usage of ExternallyOwned in Autofac, I mentioned it because singleton instances (created outside of the container) then registered with MS DI are ExternallyOwned. Disposing of the ServiceProvider does not dispose them. Therefore when registering singletons you can create an instance in advance and register that instance to take control of its lifetime. However for Open Generic registrations this is not possible because both features are lacking: 1) ability to take control of creating the instance, 2) ability to specify that the created instance is ExternallyOwned.

The following is not aimed at anyone, but just my general thought on the topic still after all this time. As a consumer, its great that containers conform to a standard, but its not great that the standard limits some kinds of registrations over others making them inherently less useful or desirable. For example registering a closed singleton generic type I can take control of instance creation and lifetime. Registering a singleton open generic type I can't. I lack all control over instance creation and whether the container owns those instances. Why the bias - are we saying that in one scenario its useful and the other it never is?

tillig commented 2 years ago

@brian-tygotech You're correct, Autofac does support it now. At the time of my comment we were working on getting the 6.0 release out the door and a lot was going on. That feature was added in the 6.0 release a couple of weeks before my comment and in the shuffle I'd forgotten. Sorry. My brain was still in 5.x land at the time.

Regardless, I'm still pretty much in the same position I was when I made that original comment:

I think the feature is interesting, for some folks it is probably valuable, but I don't think it should be added to the MS DI container. I think the MS DI container should stay as basic as possible to allow easier conformance. I'd say if someone wants to use a feature like this, it could be in a more fully-featured backing container.

retterbot commented 2 years ago

@tillig yes, I now understand what you are saying in regard to the MS DI container being the DI container specification for the dotnet framework and so adding a new feature causes issues for every conforming container.

However what I would say is that now the framework has a pretty good DI container it's starting to become embedded into lots of libraries and so although it's original aims may have been to be a simple container, developers seem to have other aspirations for it.

I do appreciate that it's possible to populate an Autofac container with an IServiceCollection and use both kinds of registration, but having tried it and being a diehard Autofac fan for almost a decade it just felt messy to me. These days we don't have any choice but to register services through an IServiceCollection because libraries are now rapidly adopting it, so you are left with this nasty chimera container, or that's how it feels to me.

I also get that this is a somewhat esoteric feature, but not having it causes two specific issues:

It is very hard to register generic components generated at runtime such as proxies, the most likely reason one might want to do this is to register fakes in test code.
The workaround is to create a non-generic factory that knows how to create the generic that one wants to create and inject that instead, but this is a code change and to me feels similarly bad to injecting the container itself.

All I'm asking is that rather than making this a hard no, you leave the door open and consider putting it on the roadmap. Thanks.

tillig commented 2 years ago

If the argument is that libraries are looking for a way to register a factory delegate for an open generic... I'm thinking the library may be doing a bit too much. I'll stand by the notion that the MS container should be lowest common denominator. Libraries should focus on providing the inversion of control required to support DI and focus less on the specific registrations. And, yeah, I get the desire to have the glue "convenience methods" of registration, but I think those are separate adapter libraries for specific DI containers, not core functions in your main library.

There's a lot of history about arguing the confirming container point that goes back before it even came out. It's not worth rehashing it all here, but if you search a little, you'll find a lot of threads just like this where some folks want the MS DI container to have a lot of functionality and others (like me) think it should stay as minimal as possible.

retterbot commented 2 years ago

I think my argument was that people are looking to converge on this as the dotnet DI container. However, I appreciate that there is a lot more history to this that I'm aware of.

On Thu, 30 Dec 2021 at 20:22, Travis Illig @.***> wrote:

If the argument is that libraries are looking for a way to register a factory delegate for an open generic... I'm thinking the library may be doing a bit too much. I'll stand by the notion that the MS container should be lowest common denominator. Libraries should focus on providing the inversion of control required to support DI and focus less on the specific registrations. And, yeah, I get the desire to have the glue "convenience methods" of registration, but I think those are separate adapter libraries for specific DI containers, not core functions in your main library.

There's a lot of history about arguing the confirming container point that goes back before it even came out. It's not worth rehashing it all here, but if you search a little, you'll find a lot of threads just like this where some folks want the MS DI container to have a lot of functionality and others (like me) think it should stay as minimal as possible.

— Reply to this email directly, view it on GitHub https://github.com/dotnet/runtime/issues/41050#issuecomment-1003170071, or unsubscribe https://github.com/notifications/unsubscribe-auth/AVOUDMWI2RWQFWHXRXSYO33UTS5QPANCNFSM4QFHQ7KA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

EvilVir commented 1 year ago

I'll support argument that .NET's implementation should NOT be minimal. It's everywhere: examples, tutorials, documentation and as base of other .NET Core's libraries. So in general this solution is default now in .NET world, yet crippled. At this point in time, OTHER libraries should release compatibility patches if they want to be used as .NET's DI replacements IMHO.

Anyway, going back to the original problem, what about some kind of fallback factory for ALL unresolved types?

AddLastChanceResolver(this IServiceCollection services, Func<ServiceProvider, Type, object?> factoryDelegate); // If null then current sad path is taken (eg. exception is thrown) if not null then this instance is used.

KalleOlaviNiemitalo commented 1 year ago

If the application calls AddLastChanceResolver, how would the DI container implementation of IServiceProviderIsService (https://github.com/dotnet/runtime/pull/54047) know which types are available from the last-chance resolver? Would the container have to not provide IServiceProviderIsService if AddLastChanceResolver has been called?

EvilVir commented 1 year ago

Well it can be another delegate, that returns bool or instead method can take type of a class that would implement some ILastChanceResolver that would have both methods on it. Even better as then such type could be registered as yet another service and take full advantage of DI.

Having methods like IsSupported(Type) is common pattern when it comes to factories (eg. JsonConverterFactory etc.).

peabnuts123 commented 1 year ago

Unless the stance of Microsoft here, here or here changed, this isn't happening.

A good summary statement from @pakrym (Source):

We haven't broken other service provider implementation since 1.0 and this feature is certainly not worth it. What it's trying to accomplish already exists in other providers

While it's not a breaking change for consumers, they don't want a breaking change for other DI implementers who would need to support the new typed implementation factory API. You correctly identified this problem in your "Risks" section.

For reference, it's still fairly popular question on StackOverflow with no great answer.

A lot of history here so I'll reply to just this comment which as far as I can tell is the overarching reason that this (and other issues) can't get any traction. Does this not create a bit of a "death by 1000 cuts" scenario? If the service provider system can ONLY tolerate huge, necessary changes, how can it ever improve? Even if there are 100 smaller changes that would collectively make up a huge necessary change, they can't be made. This seems like an ecosystem problem if nobody is capable of making any changes to keep their implementation up-to-date. I understand that implementers need to match a spec for consumers benefit, but surely this cannot be used to keep the service implementation the same forever? What about accepting API changes, and then targeting them all for a future release (such as .NET 9 or 10)? Could implementers not implement these API changes with sufficient warning, much like any other API change?

KalleOlaviNiemitalo commented 1 year ago

In .NET 8, Microsoft.Extensions.DependencyInjection now supports keyed service registrations (https://github.com/dotnet/runtime/issues/64427), which shows it is not impossible to add new features to ServiceDescriptor even if they then need to be supported in each IServiceProviderFactory\<TContainerBuilder> implementation.

tillig commented 1 year ago

Counterpoint: it may be that not all IServiceProviderFactory<TContainerBuilder> implementations can/will support keyed services in the way they've been added (the AnyKey behavior is... complex) so by adding more features to the conforming container you slowly cut down your DI options until only the MS DI container is left.

dotnet / runtime