Proposal: Add IServiceProviderFactory

[davidfowl]

This will allow 3rd party containers to plug into the default pipeline when the application doesn't:

public interface IServiceProviderFactory<TContainerBuilder>
{
    TContainerBuilder CreateContainerBuilder(IServiceCollection services);
    IServiceProvider CreateServiceProvider(TContainerBuilder containerBuilder);
}

public class ExampleAutofacServiceProviderFactory : IServiceProviderFactory<ContainerBuilder>
{
    public override ContainerBuilder CreateContainerBuilder(IServiceCollection services)
    {
         var containerBuilder = new ContainerBuilder();
         containerBuilder.Populate(services);
         return containerBuilder;
     }

    public override IServiceProvider CreateServiceProvider(ContainerBuilder containerBuilder)
    {
        return new AutofacServiceProvider(containerBuilder.Build());
    }
}

See aspnet/Hosting#829 for consumption

/cc @tillig @khellang

[khellang]

I like it! Reminds me a bit of some of the things I did with the Nancy bootstrapper prototype. This would cut down on the amount of code someone would have to write when you just want to use a third party container, but don't care about its registration APIs. You could question the need for a third party container in this case, though... But it looks like a nice addition.

[tillig]

I like it, particularly in combination with the ConfigureServices overload. Getting a ContainerBuilder (or whatever your container takes) right into the method would save a lot of boilerplate (stuff I actually have in a few projects already).

[davidfowl]

You could question the need for a third party container in this case, though... But it looks like a nice addition.

Yea, that's why paired with the ConfigureContainer(TContainer container) approach, it might be pretty nice.

It would be great to get some more feedback here to see if this is a viable approach. We plan to use this ourselves to test out our system with different DI containers.

[davidfowl]

I wonder if this should just be a hosting specific interface. To hang extension methods off the IWebHost you'd need to reference Hosting.Abstractions anyways. It'll matter more if other systems decide to use this interface for wire up.

[khellang]

I wonder if this should just be a hosting specific interface. To hang extension methods off the IWebHost you'd need to reference Hosting.Abstractions anyways.

Makes sense to split out a separate package, then. Something like StructureMap.AspNetCore.DependencyInjection (in the StructureMap case), to contrast StructureMap.Microsoft.DependencyInjection 😄

Yea, that's why paired with the ConfigureContainer(TContainer container) approach, it might be pretty nice.

Do I smell an extension to IStartup and family as well?

[davidfowl]

/cc @ipjohnson @dadhi @seesharper

[dadhi]

Does it mean, that implementation of provider is supposed to be based on "normal" adapter with it Populate method?

[ipjohnson]

I like it a lot as well, especially being able to provide container specific extension that allows for configuration.

@davidfowl what do you see as the time frame for us to create providers? Should we start planning to create packages soon or a couple months down the road (I see 1.1.0 but not sure how far off that is)?

[davidfowl]

Does it mean, that implementation of provider is supposed to be based on "normal" adapter with it Populate method?

It's the container after the populate call.

@davidfowl what do you see as the time frame for us to create providers? Should we start planning to create packages soon or a couple months down the road (I see 1.1.0 but not sure how far off that is)?

@DamianEdwards roadmap for 1.1?

[davidfowl]

I've commited the DI side of things. Will push the Hosting changes tomorrow.

[davidfowl]

cc814c6

[davidfowl]

It occurs to me this new interface can also be used to dispose the container and solve issues like this #379 (comment).

Any thoughts on this? The new interface would look like this:

public interface IServiceProviderFactory<TContainerBuilder>
{
    TContainerBuilder CreateContainerBuilder(IServiceCollection services);
    IServiceProvider CreateServiceProvider(TContainerBuilder containerBuilder);
    void Dispose(IServiceProvider serviceProvider);
}

This assumes you can get the disposable thing from the IServiceProvider.

Alternatively (I'm not a fan of this one).

public interface IServiceProviderFactory<TContainerBuilder, TContainer>
{
    TContainerBuilder CreateContainerBuilder(IServiceCollection services);
    TContainer BuildContainer(TContainerBuilder containerBuilder);
    IServiceProvider CreateServiceProvider(TContainer containerBuilder);
    void Dispose(TContainer container);
}

[ipjohnson]

With option 1 would the idea be that in the Dispose method we cast the IServiceProvider to something that exposes the TContainer that we then dispose or something a little more meta where we call the service provider to get the TContainer to then dispose?

Either way I think I like option 1 more, it's a little less code and a little less specific on how the Factory is implemented, option 2 requires you to expose TContainerBuilder & TContainer as well as having a BuildContainer method (who's to say every factory needs to implement a BuildContainer method)

my $.02

[tillig]

I see the draw to the first one since it's a little simpler and more generic, but it occurs to me you'll end up with boilerplate every time:

public void Dispose(IServiceProvider serviceProvider)
{
  var castProvider = serviceProvider as MyContainerServiceProvider;
  if(castProvider == null)
  {
    return;
  }

  castProvider.Container.Dispose();
}

Seems like that could be avoided with the second interface.

[khellang]

If you're forcing an implementation of the void Dispose(IServiceProvider serviceProvider) method anyway, couldn't you just as well do something like

public interface IServiceProviderFactory<TContainerBuilder, TProvider>
    where TContainer : IServiceProvider, IDisposable
{
    TContainerBuilder CreateContainerBuilder(IServiceCollection services);
    TContainer CreateServiceProvider(TContainerBuilder containerBuilder);
}

This way, the hosting layer could just call the Dispose method directly on the container. It simplifies the interface a bit, IMO.

Example:

public class ExampleAutofacServiceProviderFactory : IServiceProviderFactory<ContainerBuilder, AutofacServiceProvider>
{
    public ContainerBuilder CreateContainerBuilder(IServiceCollection services)
    {
         var builder = new ContainerBuilder();
         builder.Populate(services);
         return builder;
     }

    public AutofacServiceProvider CreateServiceProvider(ContainerBuilder containerBuilder)
    {
        return new AutofacServiceProvider(containerBuilder.Build());
    }

    private class AutofacServiceProvider : IServiceProvider, ISupportRequiredService, IDisposable
    {
        public AutofacServiceProvider(ILifetimeScope lifetime)
        {
            Lifetime = lifetime;
        }

        private ILifetimeScope Lifetime { get; }

        public object GetRequiredService(Type serviceType) => Lifetime.Resolve(serviceType);

        public object GetService(Type serviceType) => Lifetime.ResolveOptional(serviceType);

        public void Dispose() => Lifetime.Dispose();
    }
}

Now, getting to the provider factory is much harder with two generic arguments though 😢

Also, why can't the hosting layer just do what @tillig showed? The classic (provider as IDisposable)?.Dispose() pattern.

[davidfowl]

@ipjohnson

With option 1 would the idea be that in the Dispose method we cast the IServiceProvider to something that exposes the TContainer that we then dispose or something a little more meta where we call the service provider to get the TContainer to then dispose?

Yep, that's the only kicker. You need to be able to get it back somehow.

@khellang

If you're forcing an implementation of the void Dispose(IServiceProvider serviceProvider) method anyway, couldn't you just as well do something like

Funny, today I was talking to @halter73 about using generic constraints.

Also, why can't the hosting layer just do what @tillig showed? The classic (provider as IDisposable)?.Dispose() pattern.

It does that today but there was some push back on the container implementing

Now, getting to the provider factory is much harder with two generic arguments though 😢

Yep, that's another downside.

Should we just keep down casting the IServiceProvider to IDisposable and calling dispose on shutdown? That's the path of least resistance but you need to return something IDisposable

[ipjohnson]

Maybe I'm old school but I kinda like the explicitness of Dispose being called on IServiceProviderFactory. The factory creates the contianer, the factory disposes the container (there is a synergy to it).

@davidfowl casting to a concrete implementation in the dispose of the factory seems reasonable to me.

[davidfowl]

@ipjohnson Sounds good. It makes sense for the factory to dispose the thing it created. As long as the provider can round trip the right information I think it's a better model as well.

Here's a sample of the current bits you can all play with:

https://github.com/davidfowl/ServiceProviderFactorySample

[ipjohnson]

@davidfowl very cool, what would you think of containers offering overloads for the UseXXXX(configuration) method.

Example:

public static IServiceCollection AddAutofac(this IServiceCollection services,  Action<ContainerBuilder> builderAction = null)
{
     return service.AddSingleton<IServiceProviderFactory<ContainerBuilder>,AutofacServiceProviderFactory>(provider => new AutofacServiceProviderFactory(builderAction));
}

public static IWebHostBuilder UseAutofac(this IWebHostBuilder host, Action<ContainerBuilder> builderAction = null)
{
   return host.ConfigureServices(services => services.AddAutofac(builderAction)); 
}

internal class AutofacServiceProviderFactory : IServiceProviderFactory<ContainerBuilder>
{
  private readonly Action<ContainerBuilder> _builderAction;
  public AutofacServiceProviderFactory(Action<ContainerBuilder> builderAction)
  {
     _builderAction = builderAction;
  }

  public ContainerBuilder CreateBuilder(IServiceCollection services)
  {
       var containerBuilder = new ContainerBuilder();
       containerBuilder.Populate(services);
       builderAction?.Invoke(containerBuilder);
       return containerBuilder;
  }

  public IServiceProvider CreateServiceProvider(ContainerBuilder containerBuilder)
  {
      return new AutofacServiceProvider(containerBuilder.Build());
  }
}

Would this allow you to swap the service provider and do service configuration without doing a startup?

[davidfowl]

I'd prefer to keep the configuration of services in the application itself. The application really starts in the Startup class. The WebHostBuilder code should be considered the "host"

[ipjohnson]

fair enough, what about configuration that might be passed in the container constructor? Or for that should we skip the factory and construct the container in the startup like it's done now?

[davidfowl]

You could pass those to the factory ctor or inject them as a service. Do you have an example of what configuring the container would look like?

We want to deprecate the pattern where the application returns the IServiceProvider at some point.

[ipjohnson]

For example in Grace you have the option of passing in a IKernelConfiguration object. In DryIoc you can pass in a Rule object. Consider them options that need to be provided before the container is constructed. For the most part it's going to be application specific. But I could see passing in a configuration that's host specific (us IL generation for .Net 4.6.2 but use Linq expressions on other platforms).

[davidfowl]

@ipjohnson it might be worth forking the samples and adding support for different IOC adapters so we can flesh out any problems.

[ipjohnson]

@davidfowl I'll give it a whirl this weekend. I'll pose this question to the group, in the example shown for autofac how would allow the developer to set the optional ContainerBuildOptions parameter for IContainer.Builder

[dadhi]

@davidfowl @ipjohnson The interesting bit in DryIoc that adapter actually returns new container. So I am bit struggling how to configure services with DryIoc with less possible boilerplate. For now I may provide something like this:

public static class DIAdapter 
{
    public static IServiceProvider AddDryIoc<TCompositionRoot>(
        this IServiceCollection services, IContainer container = null) 
    {
         var adaptedContainer = (container ?? new Container()).WithDependencyInjectionAdapter(services);
         adaptedContainer.Register<TCompositionRoot>(Reuse.Singleton); 
         adaptedContainer.Resolve<TCompositionRoot>(); // does app registrations
         return adaptedContainer.Resolve<IServiceProvider>();
    }
}

public class AppCompositionRoot 
{
    public AppCompositionRoot(IRegistrator registrator /*, may inject other pre-registered services */ ) 
    {
       // app registrations go here:
    }
}

public class Startup
{
    public IServiceProvider ConfigureServices(IServiceCollection services)
    {
        services.AddMvc();
        return services.AddDryIoc<AppCompositionRoot>(); // optionally can pass pre-configured container with specific Rules and Registrations
    }
// the rest
}

[khellang]

@ipjohnson This is how I did it with StructureMap: davidfowl/ServiceProviderFactorySample@46fba0f. Just pass the options to the factory, which again passes it to the builder/container when it's created.

[davidfowl]

@dadhi not a fan. It's actually not using any of the new interface. What's the actual problem? Again, we're trying to move away from returning the IServiceProvider from the Configure method.

[dadhi]

@davidfowl Ok, then using the new IServiceProviderFactory it would be something like this:

public class DryIocServiceProviderFactory : IServiceProviderFactory<IContainer>
{
    public override IContainer CreateContainerBuilder(IServiceCollection services)
    {
         return new Container().WithDependencyInjectionAdapter(services)
    }

    public override IServiceProvider CreateServiceProvider(IContainer container)
    {
        return container.Resolve<IServiceProvider>();
    }
}

Looks fine.

[ipjohnson]

@khellang I like that, I think I must have been tired last night because I missed david's comment that passing into the factory constructor was a valid option.

I'm sold.

[slaneyrw]

How is this used in the current Asp.NetCore MVC stack ? I cannot find any reference to it anywhere

[khellang]

@slaneyrw It's used below MVC in the stack. Check out aspnet/Hosting 😄

[slaneyrw]

Ok, got this run using a Unity builder but having problems because of missing type mappings in the default container setup.

Looks like the default behaviour of DI system is to inject null when an interface is not mapped. Most containers will fail to inject on missing. Some will build a default implementation - which is worse.

I found 3 issues like this

IConfigureOptions<MvcOptions> -> MvcDataAnnotationsMvcOptionsSetup

• IStringLocalizerFactory constructor parameter optional

IConfigureOptions<MvcViewOptions> -> MvcViewOptionsSetup

• largest constructor fails if IStringLocalizerFactory is null. Isn't the this one called by the default DI ?

IRouter -> MvcRouteHandler

• IActionContextAccessor constructor parameter optional

I used the ConfigureContainer override method of the StartupBase<T> to manipulate the type mappings to allow a vanilla Mvc setup to run correctly.

Using optional constructor arguments is not a great DI practise, IMHO.

[khellang]

We had to make asjustments in ctor selection for StructureMap as well. The adapter expects the container to pick the greediest ctor it can satisfy, not just the greediest.

[halter73]

@slaneyrw Our DI system does not inject null when an interface is not mapped. I think you're running into the issue mentioned by @khellang where our DI system "expects the container to pick the greediest ctor it can satisfy, not just the greediest." This is certainly the case for MvcDataAnnotationsMvcOptionsSetup and IStringLocalizerFactory.

[slaneyrw]

@halter73 Thanks for the confirmation. This can lead to some worrying behaviour and is an interesting design choice

[pksorensen]

I added this to my servicecollection:

 services.AddSingleton<IServiceProviderFactory<IServiceCollection>>(new UnityServiceProviderFactory());

in a hostbuilder.ConfigureServices() - but when my middleware runs and access the context.RequestServices it is still the default aspnetnet core service provider and not my custom one that UnityServiceProviderFactory should create.

At what place should I register my factory implementation for it being picked up?

[halter73]

@pksorensen Is IApplicationBuilder.ApplicationServices the expected custom provider?

If so, you likely need to implement a custom IServiceScopeFactory that gets returned from IApplicationBuilder.ApplicationServices.GetService(typeof(IServiceScopeFactory)). The IServiceScope.ServiceProvider returned from IServiceScopeFactory.CreateScope() should also be your custom provider. You'll also need to ensure that IServiceScope.Dispose disposes the created custom provider.

[pksorensen]

I came to the conclussion that since I am setting up the application directly on WebHostBuilder without a startup class, that IServiceProviderFactory is not used in 1.1.1. When i used latest dev CI nugets i got things working.

[slaneyrw]

@pksorensen Make sure you change your startup class to inherit from StartupBase<TContainerBuilder> and supply the instance of the UnityServiceProviderFactory to the base constructor.

I'm using ASP.NET Core 1.1.1 and it definitely uses the provider factory