seesharper / LightInject.Microsoft.DependencyInjection

Implements Microsoft.Extensions.DependencyInjection.Abstraction
MIT License
46 stars 13 forks source link

Add possiblity to customize container and lifetimes on CreateServiceProvider #191

Closed feO2x closed 2 years ago

feO2x commented 2 years ago

First of all, thank you so much for LightInject. We use it in many of our products.

We currently work on a Fat Desktop WPF project where we also use LightInject, but want to incorporate IServiceCollection as many libraries provide AddXXX methods to register their types with the DI container. Within the application, our code uses using var ... statements to explicitly scope the lifetime of objects, thus we always use transient or singleton lifetimes for our own types.

However, when calling the CreateServiceProvider extension method, some of our types get a PerRequestLifetime as they implement IDisposable. We want to avoid this, we do not want to work with container scopes in this project.

Do you think its possible to add options to the CreateServiceProvider method so that

I'm also willing to provide this functionality as a PR.

seesharper commented 2 years ago

Hi Kenny and thanks for those kind words 😍

Both the root scope and the convention for registering services implementing IDisposable as PerRequestLifetime is there simply to stay compliant with Microsoft.Extensions.DepenencyInjecton.

We could try to implement some kind of delegate that makes it possible to register "disposable" services without a lifetime.

My only concern with this is that we will diverge from the default behaviour in MS.Ex.DI.

This test shows pretty much how this is meant to work

https://github.com/dotnet/runtime/blob/f5fd8fc62917d0d7a725be492ba656ae9295f389/src/libraries/Microsoft.Extensions.DependencyInjection.Specification.Tests/src/DependencyInjectionSpecificationTests.cs#L446

The problem with implementing some special handling of transients is that your code will suddenly depend on a certain behaviour that is not compliant with MS.EX.DI. So If you later on should decide to only use the default container in MS.EX.DI, there might be a problem.

What do you think?

UPDATE:

Also found this documentation that explains the behaviour of disposables

https://docs.microsoft.com/en-us/dotnet/core/extensions/dependency-injection-guidelines

feO2x commented 2 years ago

Hey Bernhard,

I agree, the way it is implemented right now is the correct way to make LightInject compliant with Ms.Ext.DI. Thus there should be method overloads where you can provide options like these:

public class CreateServiceProviderOptions
{
    public bool CreateRootScope { get; set; } = true;

    public Func<ServiceDescriptor, Scope?, ILifetime>? MapMicrosoftToLightInjectLifetime { get; set; }
}

Additionally, we should provide extensive XML documentation that warns users about the inconsistency with Microsoft DI Guidelines and hints them at the use case where you might want to do this (e.g. to explicitly handle lifetimes in orchestration code without requiring a container scope). An additional paragraph in readme.md might also help.

We usually have the following scenario in the WPF client:

public class SomeViewModel
{
    // Function Factories from LighInject are awesome
    public SomeViewModel(Func<IDatabaseSession> createSession)
    {
        CreateSession = createSession;
    }

    private Func<IDatabaseSession> CreateSession { get; }

    // This command is data-bound to a button in the view and calls into SaveToDatabase
    public ICommand SaveCommand { get; }

    public async void SaveToDatabase()
    {
        using var session = CreateSession(); // The lifetime of the session is handled by the view model
        session.AddContact(SomeContact);
        await session.SaveChangesAsync();
    }
}

As IDatabaseSession also derives from IDisposable, it will always be registered with a PerRequestLifetime. However, we do not use container scopes here (and are not planning to introduce them, thus I would like to use different lifetimes (transient).

The main reason why we are doing this is to make the unit tests for the view model easier. We can check if the view model correctly disposes the created session when the command is executed without setting up a DI container in the tests at all.

TBH, the DI guidelines of Microsoft are quite restrictive. They might work well for ASP.NET Core and other services where a scope is created for each request - but I don't know if this is appropriate for all scenarios. The factory pattern is OK, but why implement a factory for all things that shouldn't be tracked by the DI container? Especially when I have LightInject Function Factories?

seesharper commented 2 years ago

One way to deal with this is to explicitly register the Func<IDatabaseSession> with the container. In fact, MS.Ex.Di does not support injecting Func<T> unless it has been registered in the ServiceCollection.

Here is an example using only Ms.Di.Ex

using Microsoft.Extensions.DependencyInjection;

var serviceCollection = new ServiceCollection();
serviceCollection.AddSingleton<Func<IDatabaseSession>>(sp => () =>
{
    return new DatabaseSession();
});

serviceCollection.AddTransient<SampleViewModel>();

var provider = serviceCollection.BuildServiceProvider();

// Create a scope just to prove that the scope does not dispose the database session.
using (var serviceScope = provider.CreateScope())
{
    var viewModel = serviceScope.ServiceProvider.GetService<SampleViewModel>();
    viewModel.SaveToDatabase();
}

public class SampleViewModel
{
    private readonly Func<IDatabaseSession> _createSession;

    public SampleViewModel(Func<IDatabaseSession> createSession)
        => _createSession = createSession;

    public void SaveToDatabase()
    {
        using (var session = _createSession())
        {

        }
    }
}

public class DatabaseSession : IDatabaseSession
{
    public bool Disposed { get; set; }

    public void Dispose()
    {
        // Dispose is only called from the "using" statement in
        Disposed = true;
    }
}

public interface IDatabaseSession : IDisposable
{
}

Taking this further there is also a possibility to create an extension method to IServiceCollection that takes care of converting it to a function factory.

Something like

public static class ServiceCollectionExtensions
{
    public static IServiceCollection RegisterOwnedTransient<TService, TImplementation>(this IServiceCollection serviceCollection) where TImplementation : TService
    {
        // Very rudimentary implementation. Should probably be an compiled extression tree.

        serviceCollection.AddSingleton<Func<TService>>(CreateFunctionFactory<TService, TImplementation>());
        return serviceCollection;
    }

    private static Func<IServiceProvider, Func<TService>> CreateFunctionFactory<TService, TImplementation>() where TImplementation : TService
    {
        var ctor = typeof(TImplementation).GetConstructors().Single();
        var parameters = ctor.GetParameters();

        return (sp) =>
        {
            return () =>
            {
                object[] args = new object[parameters.Length];
                for (int i = 0; i < parameters.Length; i++)
                {
                    args[i] = sp.GetService(parameters[i].ParameterType);
                }
                var instance = (TService)Activator.CreateInstance(typeof(TImplementation), args);
                return instance;
            };
        };
    }
}

Then the registration will be like this

var serviceCollection = new ServiceCollection();
serviceCollection.RegisterOwnedTransient<IDatabaseSession, DatabaseSession>();

The upside here is that this will work regardless of the underlying DI framework since it is compliant with MS.Ex.DI

Note that this is a very naive and rudimentary implementation, but it should be fairly easy to create an expression tree representing the function factory , compile it, cache it and register it with the IServiceCollection

feO2x commented 2 years ago

Thanks for your proposal. The issue I have is that the implementation of IDatabaseSession needs some dependencies as well (e.g. an EF Core DbContext) which I want to resolve via the container. I would avoid resolving these via a different mechanism just to not instantiate this type via the container.

seesharper commented 2 years ago

Okay, let's try this then 😀

After you have created the IServiceProvider you can simply loop through the registrations and set the LifeTime to null where LifeTime is PerRequestLifeTime

var container = new ServiceContainer(new ContainerOptions().WithMicrosoftSettings());
var provider = container.CreateServiceProvider(serviceCollection);
var perRequestRegistrations = container.AvailableServices.Where(sr => sr.Lifetime is PerRequestLifeTime);
foreach (var registration in perRequestRegistrations)
{
    registration.Lifetime = null;
};
feO2x commented 2 years ago

That's a good workaround for dealing with the lifetime and I think it might work overall, but for purity's sake: a root scope would have been created. I'd like to avoid that, too.

Still, if you think this is a bad idea, I can add this functionality into another NuGet package and publish it separately.

seesharper commented 2 years ago

In LightInject one could say that the container itself represents a "root" scope. In LightInject.Microsoft.DependencyInjection, we have a root IServiceScope, but that is only used to track singletons more or less exactly in the same way LightInject would treat singletons when LightInject.Microsoft.DependencyInjection is not being used. Having a root IServiceScope does not represents any loss in performance. Unless you have other reasons for avoiding the root scope being created ? 😀

The reason I'm reluctant to make "openings" here is that correct scoping is in fact pretty difficult to get right and I fear issues related to scoping. I've had a few over the years to say the least 😀.

feO2x commented 2 years ago

Well, I see your point. I will publish it separately. Thanks for the "enlight(inject)ing" conversation.