autofac / Autofac

An addictive .NET IoC container
https://autofac.org
MIT License
4.47k stars 836 forks source link

Discussion: ContainerBuilder.Update Marked Obsolete #811

Closed tillig closed 4 years ago

tillig commented 7 years ago

In https://github.com/autofac/Autofac/commit/8a89e94ad2ffed0c10ac613b7015c11a56275c99 the ContainerBuilder.Update methods have been marked obsolete.

The plan is to leave them obsolete for a year or more, weaning developers off the use of Update, and in a future major release remove the methods altogether.

This issue is to learn from developers why they believe they need Update and see if there are better ways to handle the situations. If it turns out there is a totally unavoidable situation where Update is literally the only way to handle it, we need to determine how best to fix Update.

If You Want to Keep ContainerBuilder.Update...

If you believe you need ContainerBuilder.Update please provide the following information with your comment - "me too" or "I use it" isn't enough.

We actually need real information to understand the use cases and see if there are different ways your code could be doing things to work around needing Update. If it turns out we're missing a feature, hopefully we can figure out what that is and get you what you need while at the same time removing the need to update the container post-facto.

Why ContainerBuilder.Update Was Marked Obsolete

Here are some of the reasons why the use of ContainerBuilder.Update is generally bad practice and why we're looking at this.

Container Contents Become Inconsistent

Once you resolve something out of a built container, a lot of things get put in motion.

If you change the contents of the container, there's a chance that the change will actually affect these things, rendering cached or tracked items as inconsistent with the current contents of the container.

In unit test form (with a little pseudocode)...

[Fact]
public void InconsistentContainer()
{
  var builder = new ContainerBuilder();

  // In this example, the "HandlerManager" takes a list of message handlers
  // and does some work with them.
  builder.RegisterType<HandlerManager>()
         .As<IHandlerManager>()
         .SingleInstance();

  // Register a couple of handlers that the manager will use.
  builder.RegisterType<FirstHandler>().As<IHandler>();
  builder.RegisterType<SecondHandler>().As<IHandler>();

  using (var container = builder.Build())
  {
    // The manager is resolved, which resolves all of the currently registered
    // handlers, and is cached. This manager instance will have two handlers
    // in it.
    var manager = container.Resolve<IHandlerManager>();

    // Update the container with a new handler.
    var updater = new ContainerBuilder();
    updater.RegisterType<ThirdHandler>().As<IHandler>();
    updater.Update(container);

    // The manager still only has two handlers... which is inconsistent with
    // the set of handlers actually registered in the container.
    manager = container.Resolve<IHandlerManager>();
  }
}

Update Isn't Consistent With Build

When you Build a container, a couple of things happen:

When you Update a container, these things don't happen. We intentionally don't want the base registration sources duplicated; and unless you manually specify it we don't run startable components that may have been added during an update. Even if you specify it, your existing startable components aren't re-run; only the newly added ones would be run.

Why not fix it? There's not a clear "right way" to do that due to the container contents being inconsistent (see above). Most startable components are singletons where the point of starting them is to initialize a cache or execute some other startup logic proactively instead of lazily. If we don't re-run startables, maybe they don't pick up the things that they need. If we do re-run startables, maybe that invalidates even more things... or maybe it doesn't have any effect (in the case of singletons).

Child lifetime scopes spawned from the container get a sort of "copy" of the set of registrations in the base container. Updating the container after a child lifetime scope is spawned doesn't automatically propagate the new registrations into the child scope (Issue #608).

Basically, Update really isn't the same as Build and using it may not be doing 100% of the things you think it's doing.

Diagnostics and Optimizations Difficult to Implement

We see a lot of StackOverflow questions, issues, tweets, etc. about some of the challenges folks have around diagnosing missing dependencies. We'd love to be able to provide some better diagnostics, but one of the challenges in that is with Update: If folks assume they can change the container contents later, we conversely can't assume we can do any sort of proactive analysis or diagnostics when a container is built.

Further, we could potentially implement optimizations that, for example, proactively cache component activation data based on the registered set of components... but doing that and making sure it's all flushed/regenerated on each update doesn't always turn out to be the easiest thing to do.

Why Not Just "Fix" Update?

The question that logically follows is... why not just "fix Update so it behaves correctly?"

Easier Said Than Done

If you think about what would actually have to happen to make Update work "correctly" it includes...

...and so on. Basically, rebuild the whole container. This can really mess with your app if have something that's holding onto a resolved item that gets disposed or becomes invalid.

Locking a Container Isn't Unprecedented

Looking at other containers out there, locking a container after it's built and ready to resolve isn't uncommon. Simple Injector, LightInject, and the Microsoft.Extensions.DependencyInjection containers all disallow updating the container post-facto.

For those that do - StructureMap, Ninject, and Windsor to name a few - it appears they actually do all the work mentioned in "easier said than done" - flushing caches, rebuilding the whole container. And, as mentioned, this can cause inconsistent behavior in the application if it isn't managed very, very carefully.

Possible Workarounds for Update

Instead of using ContainerBuilder.Update, you may be able to...

Pass Around ContainerBuilder Instead of IContainer

Instead of passing the container around and conditionally updating it, change your logic to pass around the ContainerBuilder and conditionally register things correctly the first time. With the new ContainerBuilder.Properties dictionary available, you can add some context and perform business logic during the initial building of the container if you need to rather than wait until afterwards.

Add Registrations to Child Scopes

Occasionally what you need is something available during a child lifetime scope for a specific task, unit of work, or request. You can add registrations to just that child scope using a lambda:

using (var scope = container.BeginLifetimeScope(b =>
  {
    b.RegisterType<NewRegistration>();
  })
{
  // The new registration is available in this scope.
}

You may even want to cache that child lifetime scope and reuse it - like a smaller sub-container with a special purpose. That's how the multitenant integration works - cached lifetime scopes per tenant.

Use Lambdas

If you're trying to change something that's registered based on an environment parameter or some other runtime value, register using a lambda rather than reflection:

builder.Register(ctx =>
{
  if (Environment.GetEnvironmentVariable("env") == "dev")
  {
    return new DevelopmentService();
  }
  else
  {
    return new ProductionService();
  }
}).As<IMyService>();

Use Configuration

Just like with web.config transforms, you may choose to switch deployed Autofac configuration files based on an environment. For example, you may have a development configuration and a production configuration.

Use Modules

If you have a lot of registrations that need to change based on runtime, you can encapsulate that in a module.

public class MyModule : Module
{
  private readonly bool _isProduction;

  public MyModule(bool isProduction)
  {
    this._isProduction = isProduction;
  }

  protected override void Load(ContainerBuilder builder)
  {
    if (this._isProduction)
    {
      builder.RegisterType<FirstProductionService>().As<IMyService>();
      builder.RegisterType<SecondProductionService>().As<IOtherService>();
    }
    else
    {
      builder.RegisterType<FirstDevelopmentService>().As<IMyService>();
      builder.RegisterType<SecondDevelopmentService>().As<IOtherService>();
    }
  }
}

Use Conditional Registrations

Autofac 4.4.0 introduced OnlyIf() and IfNotRegistered extensions. These allow you to execute a specific registration only if some other condition is true. Here's the documentation. Quick example:

// Only ServiceA will be registered.
// Note the IfNotRegistered takes the SERVICE TYPE to
// check for (the As<T>), NOT the COMPONENT TYPE
// (the RegisterType<T>).
builder.RegisterType<ServiceA>()
       .As<IService>();
builder.RegisterType<ServiceB>()
       .As<IService>()
       .IfNotRegistered(typeof(IService));

Handling Application Startup / Bootstrap Items

A common scenario for wanting to update the container is when an app tries to use a container to register plugins or perform app startup actions that generate additional registrations. Ideas for handling that include:

Consider Two Containers

If you are using DI during app startup and then also using it during the execution of the app, it may be that you need two containers: one for each stage in the app lifecycle.

The first is a container that has services used to index plugins (the "assembly scanning" mechanism, logging, that sort of thing); the second is a container into which runtime requirements are registered like the set of plugin assemblies, required common dependencies, and so on.

Don't Over-DI Bootstrap Items

It's good to use DI, but you can easily get into a chicken/egg situation where you try to resolve bootstrap items (like your application configuration system, logging that will run during application startup, and so on) out of the container... that you're trying to set up during app startup.

Don't do that.

If you look at many of the newer ASP.NET Core examples, you'll see a good pattern where app configuration, base logging, and other "bootstrap" elements are actually just directly instantiated or built. Those instances/factories can then later be registered with Autofac for use during runtime, but the initial construction proper isn't done out of Autofac.

Share Instances Across Containers

If you go with the two-container startup, you can always register the same instance of a thing (e.g., application configuration, logging factory, etc.) into two different containers. At that point it's effectively a singleton.

(You can also use Autofac modules to share registrations if you have bunches of them that need to be duplicated, though you'll get different instances of things so be aware.)

Lambdas, Lambdas, Lambdas

Many, many container updates could be worked around using a lambda registration. "I need to register XYZ based on the result of resolving ABC!" - do that with a lambda registration.

Nancy Framework Users

If you use Nancy, it internally uses Update(). There is already an issue filed for Nancy to be updated - you can follow that issue or chime in over there if you're interested in how that is progressing.

Prism (WPF) Framework Users

Prism only supports modules in mutable containers. This is an architectural choice of the Prism project owners. An issue was filed here to alert Prism of the changes around Update() and as part of a major IoC integration refactor the decision was made to only support modules for mutable containers. While it may be possible to enable modules for Autofac via one of the above strategies or something like registration sources, Prism is expecting the community to submit and support that code. Head over there if you'd like to follow up with them; there is no current plan to create an Autofac-project-supported Prism integration library.


Short Term Fix

If your code uses Update and you want to keep using it for the time being, you can disable the warning just around that call.

#pragma warning disable 612, 618
builder.Update(container);
#pragma warning restore 612, 618
tillig commented 7 years ago

I believe you could get around the if(!registered) by using PreserveExistingDefaults:

builder.RegisterType<SomeService>().As<IService>();
// then later
builder.RegisterType<SomeOtherService>().As<IService>().PreserveExistingDefaults();

What that does is add the registration but make sure that previous registrations are used first, so if you have a singleton, you'd still get a previous registration first. The only time that may affect you is if you're resolving IEnumerable<T> - you'd get both services, not just one.

A new way you may be able to work around it is to use the Microsoft.Extensions.DependencyInjection abstraction - less full featured but allows for that sort of pre-build manipulation.

But otherwise, what I'm hearing is that you may be able to get away from Update if you had an IfNotExists registration extension as outlined in #469. Does that sound about right?

Note that part of what we're trying to do is not only provide a good container but push people towards better practices and help them avoid pitfalls. Bringing over practices from one container to another isn't something we're really interested in addressing since each container has its own quirks. On the other hand, it is interesting to provide a good feature set; get people to use good IoC practices that avoid container inconsistency; and be able to bring the container forward by adding new features.

twirpx commented 7 years ago

There is scenario in which I have to use Update(IContainer):

  1. At application start container is building from configuration classes, data access classes, services, logging staff etc.
  2. Then instance of www-host service class resolved though container.
  3. This class creates dotnetcore IWebHostBuilder, IWebHost and starts listening through Kestrel
  4. At this point I have to pass startup-class that contains something like this:

    public IServiceProvider ConfigureServices(IServiceCollection services) {
    IContainer container = Program.Container;
    
    // configuring MVC services here
    
    // I should create new container, populate MVC services into it
    // and then update main container to be able to use it instead of
    // built-in aspnetcore DI container
    ContainerBuilder builder = new ContainerBuilder();
    builder.Populate(services);
    builder.Update(container);
    
    return container.Resolve<IServiceProvider>();
    }

    How can I rewrite this scenario?

RaphyFischer commented 7 years ago

Hey @tillig ,

I am quite new to AutoFac and want to replace Unity in my application with AutoFac. When I tried this I ran into some issues.Most of them were quite easy to solve. My main problem on the other hand "seemed" to be easy to solved, by using theContainerBuilder.Update() function until I ran into this posting #811 here on GitHub. But first let's provide some more information about what I use exactly and why I use it.

Which overload do you use? Update(IContainer), Update(IContainer, ContainerBuildOptions), or Update(IComponentRegistry)?

The overload I use is Update(IContainer).

When you use Update, have you already resolved something from the container before calling Update?

Yes I do resolve some services, before all registrations are registered. Why this is the case I will explain within the use case below.

What is the scenario (use case) in which you are unable to pass the ContainerBuilder prior to container building?

The application I want to integrate AutoFac into is a ASP.NET WebAPI 2 based Web Service with OWIN integration. What I need to do at the startup of the application is registering services (which hold some configuration stuff so it is easily accessable with DI) and do registrations based on the config informations which are held by the mentioned services.

To make this more clear here is some pseudo sample code:

public void Startup()
{
        var builder = new ContainerBuilder();
        builder.RegisterType<ServiceWithConfigInformation>();

       //... do some more registrations and stuff

      var container = builder.Build();
      configService = container.Resolve<ServiceWithConfiguration>();

      if(configService.SomeThingIsSet)
     {
            builder = new ContainerBuilder();
            builder.RegisterType<SomeService>();
            builder.Update(container);
     }

     //... going on with the startup of the application.
}

I know this code is not correct in some ways, but I just wanted to display my situation. Also the resolving sometimes does happen in another class so it would not be possible for me to hold an instance locally and use this. This is not the only case I need this and not the only service I need that for.

The application also uses extensions (MEF and Microsoft Addin Framework) to extend it's funcionality. So there is an use case where additional registrations have to be made when the application is already running. The application also supports updating and installing these extensions on runtime, and changing configurations on runtime. So in this case I may have to change some registrations, remove or update them. I totally understand, that updating an existing container is not the best practice but in my case I didn't see any possiblity to come around this.

The next thing I evaluated were your suggestions using modules or lambdas to extend a container/scope with registrations.

Use Modules

Use Lambdas

In principal I would be ok with these solutions because I could solve most of my problems with it. But I wondered how these functions handle additional registrations for a container/scope. So I went through the sources and discovered, that these functions also use the ContainerBuilder.Update() function. Now I am confused... will the modules and lambdas work in future releases or will this be kicked out as well, or do you have any other solution to this? I saw, that these use the overload : ContainerBuilder.Update(IComponentRegistry) will this stay supported?

So in conclusion: Is there any way to implement my use case with AutoFac in an "correct" way and if not, will modules or lambdas still be supported in future releases?

Thank you!

alexmg commented 7 years ago

@twirpx Is your issue with WebHostBuilder the one described below?

https://github.com/aspnet/Hosting/issues/829

If so an IServiceProviderFactory implementation might help. That interface is now part of Microsoft.Extensions.DependencyInjection.Abstractions and is something we could provide an implementation of in Autofac.Extensions.DependencyInjection.

https://github.com/aspnet/DependencyInjection/blob/dev/src/Microsoft.Extensions.DependencyInjection.Abstractions/IServiceProviderFactory.cs

twirpx commented 7 years ago

@alexmg It seems that it is not a solution. It is just another way to pass IServiceProvider to dotnet dependency injection mechanism.

The problem is that the container building happens before WebHostBuilder initialization. And this two steps cannot be swapped in order because code creating WebHostBuilder depends on externals that got through IoC.

alexmg commented 7 years ago

Hi @Raphy1302.

The first thing that comes to mind looking at the example code is what happens when something asks for the service that wasn't actually registered based on configuration? The same conditional logic would end up having to be used at the point of consuming the service, and most likely without that service taking a direct dependency, which implies the need for some kind of service location anti-pattern.

The Null Object pattern with a lambda based registration that checks the configuration state could remove the need to update the container in a case such as this.

var builder = new ContainerBuilder();
builder.RegisterType<ServiceWithConfiguration>();

builder.Register<ISomeService>(c =>
{
    var config = c.Resolve<ServiceWithConfiguration>();
    if (config.SomeThingIsSet)
        return new SomeService();
    else
        return new NullSomeService();
});

Other services could still take a dependency on ISomeService without having to first check if the configuration flag was set or not. It would instead perform the appropriate NOOP behaviour.

Would something like that get you out of trouble if the Update method wasn't available?

RaphyFischer commented 7 years ago

Hey @alexmg

Thanks for your quick response! This will partially solve my problems. What you pointed out is correct and not the best way, but in this application this case will be not a big problem, because it is controlled by the developer who creates a extension. Also this will be changed and the extension (which consumes the service on runtime) has to enable the registration in a config file. My bigger problem here is, that the application will load such extensions on runtime. Therefor the services for this extension will be loaded when the extension is installed. Also the application should be able to change configurations on runtime.

What is about my last point I mentioned about the modules and lambda expressions. I still wonder how this is gonna work when the update function is not available anymore?

Thanks!

huan086 commented 7 years ago

My use case is to inject the IContainer itself into the constructor, so that I could use it in using (ILifetimeScope scope = this.dependencyContainer.BeginLifetimeScope())

See http://stackoverflow.com/a/22260100/263003

var builder = new ContainerBuilder();

// register stuff here

var container = builder.Build();

// register the container
var builder2 = new ContainerBuilder();
builder2.RegisterInstance<IContainer>(container);
builder2.Update(container);
public class WindowService : IWindowService
{
    private readonly IContainer container;

    public WindowService(IContainer container)
    {
        this.container = container;
    }
}
osoykan commented 7 years ago

hi @huan086,

i've solved your problem using a different way which is also available in nuget package and tested. Project: Autofac.Extras.IocManager I hope this would be helpful.

tillig commented 7 years ago

@huan086 If you need the container, it implies you're doing some service location. I'd recommend using the solution from @osoykan or another service locator like CommonServiceLocator rather than forcing the container to be able to resolve itself.

huan086 commented 7 years ago

@osoykan @tillig Don't think I'm using service locator. All I really need is to BeginLifetimeScope, which happens to be a method on IContainer.

I realize that I don't need the Update method after all. I shouldn't be injecting IContainer. I should inject ILifetimeScope instead. Autofac already register that automatically, and I can use ILifetimeScope.BeginLifetimeScope.

See http://stackoverflow.com/questions/4609360/resolve-icontainer

arieradle commented 7 years ago

Which overload do you use? Update(IContainer), Update(IContainer, ContainerBuildOptions), or Update(IComponentRegistry)?

The overload I use is Update(IContainer).

When you use Update, have you already resolved something from the container before calling Update?

Yes, consider the following example:

builder.Register ... 
...
var container = builder.Build();

var extenders = container.Resolve<IEnumerable<ISomeExtender>>()
extenders.ForEach(e => e.ConfigureODataStuffHereForEachModule(HttpConfiguration)); 
// has to happen here, while I'm still configuring WebApi and OData

var updater = new Autofac.ContainerBuilder();
updater.RegisterWebApiFilterProvider(HttpConfiguration);

updater.Update(container);
...

The removal of the "Update" method will force me to manage two different containers: one for extensibility and other for general logic - one big mess.

tillig commented 7 years ago

@arieradle What stops you from registering the filter provider when building the container the first time? No requests would be running through the system at the time so it shouldn't matter.

arieradle commented 7 years ago

@tillig RegisterWebApiFilterProvider requires HttpConfiguration object to be ready I presume.

Will RegisterWebApiFilterProvider work if I still need to register some OData points and attribute routing for WebApi after it runs?

tillig commented 7 years ago

@arieradle It needs the config object so it can add itself as the filter provider, but it won't resolve or really do anything until a request comes in. Give it a shot. I think you'll be fine, and it would mean no call to Update.

arieradle commented 7 years ago

@tillig Could be, I'll try.

Have another issue:

var signalrResolver = new Autofac.Integration.SignalR.AutofacDependencyResolver(container);
GlobalHost.DependencyResolver = signalrResolver;

var updater = new ContainerBuilder();

updater.RegisterInstance(signalrResolver.Resolve<Microsoft.AspNet.SignalR.Infrastructure.IConnectionManager>());

updater.RegisterType<SignalRAssemblyLocator>().As<Microsoft.AspNet.SignalR.Hubs.IAssemblyLocator>().SingleInstance();

updater.Update(container);
tillig commented 7 years ago

@arieradle Well, there are two things there. Let's tackle each in turn.

The second line is the easiest:

updater.RegisterType<SignalRAssemblyLocator>().As<Microsoft.AspNet.SignalR.Hubs.IAssemblyLocator>().SingleInstance();

There's no reason you couldn't register that earlier. It's not being resolved or activated, it's just a registration.

The first line is a tiny bit trickier and I'm not sure why you need it. The way I'm reading it...

I'm not sure why that's required. The thing you're requesting is already in the container, so you're just adding a second, static copy of it. My initial thought is that you probably don't need that line at all.

However, let's say you do need it or want it for some reason. You can do delayed resolution of an item by registering things as a lambda. I think that's something which may of the Update scenarios I've seen out there could use to work around the need for updating.

You could take this line...

updater.RegisterInstance(signalrResolver.Resolve<Microsoft.AspNet.SignalR.Infrastructure.IConnectionManager>());

...and change it to a lambda that uses the GlobalHost.DependencyResolver:

builder
  .Register(ctx => GlobalHost.DependencyResolver.GetService<IConnectionManager>())
  .As<IConnectionManager>()
  .SingleInstance();

The lambda doesn't execute until you actually try to resolve it, so by the time you need to get the object you'll have already set the static GlobalHost.DependencyResolver field and things will work out.

Again, though, verify that circular registration is necessary. If you can resolve it from the container already, you shouldn't need to re-add it.

dornstein commented 7 years ago

I think I might need to use Update() but I'm not sure yet. If there's an alternative, I'd love to know but if not then consider this another case for keeping it.

I'm building a platform (user ASP.NET) that supports pluggable "apps."  I'm using Autofac extensively currently for global and per-request scoped services.  Now I need to allow my plugged in apps to register components/services but I'm not sure how to do that. Note that the app lifetimes pretty much match the entire global lifetime of the web app.

Here's the challenge:

One of the current services is the singleton AppManager service which is responsible for loading each app and getting it started up.  The challenge is this: the AppManager of course has various dependencies that it needs injected in so the app manager can't be instantiated in order to load apps until Build() has already been run on the IContainer.  Only then can each app be given a chance to register its services.   But by then the container has been built.

I've seen that there is an Update() method for the container but there are strong reasons listed why using that should be avoided.  Because I have confidence that the app-based registration will happen relatively early in the startup of the whole platform maybe these cautions can be ignored (?), but I'm wondering if there is some better way.

How would you approach this in a way most consistent with best practices for Autofac?

tillig commented 7 years ago

There are a few ways I've seen things in these situations handled. These aren't the only ways, and it's not One Size Fits All, but mix and match as appropriate.

Keeping in mind the original workarounds and fix tips I already posted...

Consider Two Containers

If you are using DI during app startup and then also using it during the execution of the app, it may be that you need two containers: one for each stage in the app lifecycle.

I have this myself in several of my larger apps - a container that has services used to index plugins (the "assembly scanning" mechanism, logging, that sort of thing); and a second container into which runtime requirements are registered like the set of plugin assemblies, required common dependencies, and so on.

Don't Over-DI Bootstrap Items

It's good to use DI, but you can easily get into a chicken/egg situation where you try to resolve bootstrap items (like your application configuration system, logging that will run during application startup, and so on) out of the container... that you're trying to set up during app startup.

Don't do that.

If you look at many of the newer ASP.NET Core examples, you'll see a good pattern where app configuration, base logging, and other "bootstrap" elements are actually just directly instantiated or built. Those instances/factories can then later be registered with Autofac for use during runtime, but the initial construction proper isn't done out of Autofac.

Share Instances Across Containers

If you go with the two-container startup, you can always register the same instance of a thing (e.g., application configuration, logging factory, etc.) into two different containers. At that point it's effectively a singleton.

(You can also use Autofac modules to share registrations if you have bunches of them that need to be duplicated, though you'll get different instances of things so be aware.)

Lambdas, Lambdas, Lambdas

Truly, almost every container update I see in the wild could be worked around using a lambda registration. "I need to register XYZ based on the result of resolving ABC!" - do that with a lambda registration (as shown in the workarounds at the top).


Given there's not a One Size Fits All solution, I can't say "here's the step-by-step way you, in your very specific app, would handle all of these cases." But the above should help. The two-container approach is the first thing I thought of when you mentioned your issue.

Note: I copied these ideas to the top so they're not buried in the middle of this long chain.

dornstein commented 7 years ago

I might actually lean towards the Don’t Over-DI recommendation. I could probably get my AppManager and a few other key objects (like logging and configuration) done manually – and then add those to the container before it’s built.

If I went with two containers, would I then end up needing to pass around both containers (as my apps, for example need services form both). Or is there some kind of composite resolver that could be used? Passing around two seems onerous – and the app then needs to know which one has which services…

tillig commented 7 years ago

Instead of passing around two containers, consider the "share instances across containers" idea...

var configuration = CreateTheConfigurationInstance();
builderForContainer1.RegisterInstance(configuration);
builderForContainer2.RegisterInstance(configuration);

You could also do something like...

builderForContainer2.Register(ctx => container1.Resolve<Something>());

That is, use a lambda to resolve something from one container using another container.

That latter snippet has a bad smell, in my opinion. I think sharing bootstrap object instances across containers is fine. Those instances may even be factories.

var loggingFactory = new LoggingFactory(configuration);
builderForContainer1.Register(ctx => loggingFactory.CreateLogger()).As<ILogger>();
builderForContainer2.Register(ctx => loggingFactory.CreateLogger()).As<ILogger>();

If you have stuff in your app startup container that you also want to use during runtime, then you really need them to be singletons (instances). The problem is that if they aren't then the dependencies they are using from the first container aren't consistent with the dependencies registered in the second container. That's the HandlerManager example I posted in the comment at the top. If you're registering an object in one container that's been resolved from another container... that has a bad smell to it.

Let me use an analogy: think about a circular build dependency. (I'm going off topic a bit here, but stick with me.)

Say you have two libraries (Library 1 and Library 2) and some custom build tasks. The custom build tasks are used to build both libraries... but they also depend on Library 1.

You could address that by building Library 1 without the tasks, saving a copy of that, building the build tasks, then re-building Library 1 and Library 2 using the built version of the tasks... but your build tasks are then using a stale/inconsistent version of Library 1. That's not cool.

The right way to handle that is to either stop building Library 1 with the same set of build tasks that depend on it... or split the stuff out of Library 1 that the build tasks need into, say, Library 0, and build Library 0 without the tasks and let Library 1 and 2 build with the tasks.

Bringing it back on topic:

It may seem like there's no problem with that initial circular dependency... and that's the "I don't care if my app's dependencies are inconsistent, I need to use Update()!" argument. (Or "...I'll just register one object that came from one container into another container!") But there is something wrong with the circular dependency - it needs to be unwound so things are consistent and expected - just like trying to over-DI or create complex containers-calling-containers really should be unwound.

It also may seem like there's no way to unwind the circular dependency... and that's the "It's too hard to change my design so I'll do the least work possible and make it work despite the complexity!" argument. But there is a way to unwind it by changing the problem definition. In the circular dependency analogy, splitting Library 1 up allows you to change the way things work; in the app startup vs. plugins scenario, consider what really needs to be shared. Is it actual object instances or is it registrations? Can you use Autofac modules to help you? Can you use lambdas or RegisterInstance to help? What about doing a few things manually outside the container?

How, exactly, to unwind that is going to be up to you. I can't really provide much beyond what I already have when faced with fairly sweeping generalities. I'd just say... if you see the solution getting really complicated ("this container resolves from that container which reads from some factory that...") then you may not have the right solution.

An example of a widely-used two-container setup with some delegation around it is ASP.NET Web API 2. In there you'll see that the application configuration object has a static Services collection (basically a container) against which application level services can be registered (like the action invoker). When the application needs something, it doesn't query the dependency resolver, it queries the services collection. If the services collection doesn't have the service, it falls back internally to the dependency resolver.

That comes with its own challenges since, at least in Web API 2, that doesn't support per-request services. In some cases things get incorrectly cached, too, so something that's registered "instance-per-dependency" will be held for the app lifetime and become a singleton.

A lot of things changed in ASP.NET Core, where most things do actually come from the container... but when you look at how things like middleware Invoke methods are executed, the parameters for Invoke are a much more manual resolution method. Same with parameters that get pushed into your Startup class methods.

I can't tell you if you need your own version of a Services collection like Web API 2... I can't tell you if you need to do a few things more manually to unwind that multi-container conundrum you describe... but hopefully there are a few ideas here you can use to get you out of the jam.

nblumhardt commented 7 years ago

Loving this discussion, apologies for jumping in a bit late. I find myself leaning heavily towards making the registrations-in-child-container pattern a first-class concept, but then found myself wondering, will supporting registrations in child scopes interfere with any of the simplification/optimisation opportunities we're trying to open up?

(Also, FWIW, System.Composition is another immutable container implementation.)

dornstein commented 7 years ago

I'm starting with the idea of getting the minimal set of app-loading items up and running without DI. That seems simplest. I think that will probably work but if it doesn't I may try the multiple containers.

I might end up trying to anyway because I'm trying to decide if one app's registrations should be visible for another app -- there are pros and cons -- but if they need to be per-app then one global container plus one container per app is probably the only choice.

If I do end up with multiple containers, I would share instances for the singletons, but I'm really not sure how I'd deal with per-request scoped objects (yet).

[And thanks for being so responsive. This sort of plugin situation seems like it would be at least semi-common...]

tillig commented 7 years ago

Child lifetime scopes (another item mentioned in the top post) could also be a solution.

Create the container with the application startup stuff. Each plugin then gets its own child lifetime scope and treat those lifetime scopes "like containers" for the plugins. Or if it's a central container with multiple "apps" or whatever your terminology is... instead of passing around IContainer, use ILifetimeScope. You can add registrations to the child scope when it's created.

When building the child app scopes, instead of passing around a ContainerBuilder you could pass a List<Action<ContainerBuilder>> and register things that way...

var regActions = new List<Action<ContainerBuilder>>();
regActions.Add(b => b.RegisterInstance(something));
regActions.Add(b => b.RegisterType<SomeType>());

...and later just run those.

var appScope = container.BeginLifetimeScope(b => {
  foreach(var a in regActions)
  {
    a(b);
  }
});

This "treat a child lifetime scope as a container" thing is how the multitenant integration works.

bc3tech commented 7 years ago

Update gets used by bots built on the  Microsoft Bot Framework as the BotBuilder SDK uses Autofac internally. If users wish to register additional types in to the container already present in the BotBuilder for use in other areas of their bot, ContainerBuilder + Update() was the way to do it.

Said a different way, the BotBuilder SDK exposes an IContainer to which users can, and often do, add additional registrations in to at startup (before any resolutions happen).

tillig commented 7 years ago

@bc3tech Interesting. We'll have to file an issue over there to see if we can get them to change their architecture. Thanks!

brandonh-msft commented 7 years ago

@tillig I've started an internal thread on this

tillig commented 7 years ago

@brandonh-msft would it be better if I didn't file the issue?

brandonh-msft commented 7 years ago

it may be that we create some (better) guidance on how to register components. Standby.

samueldjack commented 7 years ago

I've been using Update() in the context of customising a container for integration tests. Specifically, I sometimes want to override some registrations to supply a mock instead of the real service.

Current Set-up

My project is built on Asp.Net Core, and I'm using its TestServerimplementation in my tests. I'm initializing the TestServerwith new WebHostBuilder().UseStartup<Startup>() where Startup is the startup class for my Asp.Net Core project.

In order to enable overriding services in the container from my tests, my Startup.ConfigureServicesmethod first builds the container, then it registers the IContaineritself in the container. Like so:

        var builder = new ContainerBuilder();

        // populate the container with the asp.net services collection first
        // so that any registered by the modules override the defaults
        builder.Populate(services);
        builder.RegisterAssemblyModules(typeof(AuthorizationModule).GetTypeInfo().Assembly);

        var container = builder.Build();

        // In integration tests, we need to be able to replace services with mocks on occassions,
        // so we need to provide the container so that it can be updated
        if (_hostingEnvironment.IsIntegrationTesting())
        {
            builder = new ContainerBuilder();
            builder.RegisterInstance(container).SingleInstance();
            builder.Update(container);
        }

        return new AutofacServiceProvider(container);

In my integration tests, the TestServerprovides access to the container through its Host.Services property. So if I want to replace a service I can resolve IContainerout of the container and use ContainerBuilder.Update to supply a new registration.

How to do this without Update()?

I've tried to think through the various options to rework this to avoid using Update(), but so far I've not come up with a satisfactory solution.

WebHostBuilder.ConfigureServices()

My first thought was to try the ConfigureServicesmethod on WebHostBuilder. This allows services to be registered before the Startup.ConfigureServices method is called.

The problem here is that any subsequent registrations made by the ContainerBuilderwould override anything I registered specifically for the tests. To avoid this, I'd need to register everything conditionally, which sounds like quite a pain.

Subclassing Startup for specific tests

Another potential solution that came to mind was adding a virtual method to my Startup class which would allow customisations to the ContainerBuilderbefore it was built. In my tests, I could then sub-class Startup, and provide the derived class to WebHostBuilder. A point to note is that WebHostBuilderrequires the type of a startup class, rather than an instance, and it constructs instances from the type.

This approach doesn't appeal to me either, since every test which wanted to customise registrations would need its own sub-class of Startup, and that would get very cumbersome. It would also be difficult to share data between the tests and the mocks.

My ideal solution

The ideal solution would let me do something like this:

    public void Test()
    {
        var builder = new WebHostBuilder()
            .UseEnvironment("Development")
            .UseStartup<Startup>()
            .OverrideService<IAuthorizationService>(() => new Mock<IAuthorizationService>().Object);

        _server = new TestServer(builder);

        // do test
    }

Any ideas how this can be achieved without Update()?

tillig commented 7 years ago

@samueldjack In your "current set-up" snippet, simply move the container build operation to after the if statement.

        var builder = new ContainerBuilder();

        // populate the container with the asp.net services collection first
        // so that any registered by the modules override the defaults
        builder.Populate(services);
        builder.RegisterAssemblyModules(typeof(AuthorizationModule).GetTypeInfo().Assembly);

        // In integration tests, we need to be able to replace services with mocks on occassions,
        // so we need to provide the container so that it can be updated
        if (_hostingEnvironment.IsIntegrationTesting())
        {
            builder.RegisterInstance(container).SingleInstance();
        }

        // Build the container after modifying the contents instead of before. 
        var container = builder.Build();

        return new AutofacServiceProvider(container);

You could also make use of the Autofac.Configuration package to optionally include overrides if you're doing it in your Startup class.

// Do all your normal stuff.
builder.RegisterType<Whatever>().As<IWhatever>();

// Time for optional overrides! Use Autofac.Configuration
// and Microsoft.Extensions.Configuration. If the file doesn't exist,
// this effectively registers nothing. If it does, great - you get the overrides.
var config = new ConfigurationBuilder();

// The "true" flag there means it's optional - it won't blow up
// or complain if the file doesn't exist.
config.AddJsonFile("autofac.json", true);

var module = new ConfigurationModule(config.Build());
builder.RegisterModule(module);

// Last but not least, build the container.
var container = builder.Build();

There are potentially ways to handle this through the ASP.NET Core / .NET Core Microsoft.Extensions.DependencyInjection stuff as it pertains to starting up your app, but it might be better to get recommendations from the Microsoft.Extensions.DependencyInjection repo since that's not Autofac-specific at all.

tillig commented 7 years ago

It was mentioned in a comment on the code that Nancy.Boostrappers.Autofac uses Update - we're aware of that as are the maintainers of that project. The Nancy folks are looking at a new way to handle bootstrappers since Autofac isn't the only container that treats container contents as immutable.

WilliamBZA commented 7 years ago

But otherwise, what I'm hearing is that you may be able to get away from Update if you had an IfNotExists registration extension as outlined in #469. Does that sound about right?

@tillig In my case: this is exactly what I need. PreserveExistingDefaults() isn't quite enough unfortunately as the API I am required to conform to requires that the client can ask me if a type has been registered or not before the building step.

I have worked around this for now by storing a local HashSet of all the services I register, but it's a pain and feels like I'm lugging this list of items around for no real reason.

famschopman commented 7 years ago

I stumbled upon this thread because I was looking for ways to register tenants runtime and just happened to create a question for this on stackoverflow. So this appears to be a case that is impacted by this change.

What I basically want to do; on startup register all tenants, and after startup add new registrations when new tenants are created so we can avoid an application restart.

http://stackoverflow.com/questions/42068224/autofac-best-practices-on-runtime-adding-tenants

tillig commented 7 years ago

@famschopman You wouldn't use Update() to register tenants. Your question is unrelated to this topic.

tillig commented 7 years ago

@WilliamBZA If you have ideas on how IfNotExists should work, consider commenting on #469 - there are some open questions and it's not necessarily as straightforward as all that.

tillig commented 7 years ago

I just pushed version 4.4.0-394 to the MyGet feed with implementations of OnlyIf and IfNotRegistered predicates. You can see some example usages in the unit test fixture. At a high level, you have two options:

// Provide a Predicate<IComponentRegistry>
builder
  .RegisterType<Something>()
  .OnlyIf(reg => { /* Do something with IComponentRegistry */ });

// or provide a service type
builder
  .RegisterType<First>()
  .As<IService>();
builder
  .RegisterType<Second>()
  .As<IService>()
  .IfNotRegistered(typeof(IService));

Give it a shot, see what you think.

vincentparrett commented 7 years ago

I spent the day refactoring my way around this today.. the only ones I wasn't able to resolve are where we register things in child scopes.

tillig commented 7 years ago

@vincentparrett Did you try registering the child scope items during child scope creation as part of BeginLifetimeScope()?

vincentparrett commented 7 years ago

@tillig Yes, and I was able to do that in some places, however we're using Nancy and the child scope is already created before we add to it in the bootstrapper.

tillig commented 7 years ago

@vincentparrett Ah, Nancy. There is already work in motion on Nancy to fix it on their end. We can't fix their design on our end; you may want to chime in over there or submit them a PR.

vincentparrett commented 7 years ago

@tillig will do.

tillig commented 7 years ago

I published Autofac 4.4.0 to NuGet which includes the OnlyIf and IfNotRegistered functions. Documentation here.

tesharp commented 7 years ago

We currently use Update() to update our root container in our own special context container. Not sure how to do that without Update().

Our context container is based on tenant container, but slightly different. Client will send in a context in all calls, SignalR and Web API, and based on context container will switch to a specific lifetimescope to resolve all services. Lifetimescopes are hierarchical, so we basically have a hierarchy like this:

Root container
 |- Application container
   |- Module1
     |- SubModule1
     |- SubModule2
   |- Module2
     |- SubModule3
     |- SubModule4

In addition to having a module, each module can also have a client id. So potensially it is quite complex hierarchical structures of lifetimescopes. All this is read out from config (modules) and database (clients) during boot time. So it will initiallize Autofac MEF to find plugins and read config files, then build root container with all the "Core" modules. Using this container it will resolve an IPluginService to read available plugins and clients, at this point it requires access to many of the core components.

After it has resolved IPluginService it will resolve all plugins, create lifetimescopes hierarchically and register module specific registrations in those lifetimescopes. The problem is however that some modules need to register things back into root container, so it is accessible to all modules. E.g. most modules register validators for their own models in its own scope, but some times SubModule3 might need to validate a model registered in SubModule1. To do this we today register the IValidator in root container so it can be resolved in all scopes. That way models that are defined inside plugins (that are not accessible at start) can be verified across modules.

And that is where Update() comes in... We need to create the parent container before creating a module lifetimescope, but need to add registrations up the hierarchy. We are not changing existing registrations, only adding new registrations in parent containers.

tillig commented 7 years ago

@tesharp Generally we have seen things like this solved in one of two ways:

Option 1: Two Containers

In this case, you'd have one container only used for application startup stuff like IPluginService. The things in this container would be used to initialize the app and build up a second container. The second container is what would be used for the application itself. There may need to be some changes around how your modules work (e.g., trying to "register things back into a parent container" isn't really a great thing; instead you'd start using named lifetime scopes and SingleInstance()), but it's possible.

Option 2: Manually Create App Startup Items

Instead of using a container to build up IPluginService or app startup components, manually create them. If that needs to be pluggable somehow, consider using a pattern where a configuration key indicates a type name to bootstrap the service and using Activator.CreateInstance() or something similar. The idea is that you break the cycle between things needed to bootstrap the container and the actual stuff inside the container.


I might, if possible, lose the MEF integration and stick to Autofac registrations, too. The MEF integration is nice, but it can sometimes complicate things when trying to interoperate things registered with MEF and things registered not using MEF.

Obviously I can't provide too much specific guidance on your app since it sounds like there's a bit of complexity there that isn't totally clear without seeing the code and really getting app specific but generally using one of the above patterns can help unwind things enough to see different solutions to the problem.

tillig commented 7 years ago

@tesharp This comment contains some additional explanation and ideas around bootstrap items.

tesharp commented 7 years ago

@tillig Thanks for reply. I looked a bit into alternative solutions and concluded that 2 containers would not work that great how our application is designed now. For MEF we only use it to load plugins and find all Autofac.Module, and initialize them. Only a few core components is registered in MEF, like IConfiguration, and we can then inject that when loading Plugin modules. With how few components registered in MEF we have not had any problems with that.

As for main application I ended up with spliting plugin load into 2 parts. Previously we have a PluginModule.Initialize method that was called once and did all registrations (also into parent lifetimescopes). Now I've split it into PluginModule.InitializeApplication and PluginModule.Initialize so we can first register what we need in application scope, and then start initializing child lifetime scopes with their components. Not finished rewriting all this yet, so still remains to see how it will work at the end, but I think this should solve our problems.

ShadowDancer commented 7 years ago

I have one thing that is on my todo list, but not yet done.

I have quite big ecommerce/cms application, that uses plugin system. Currently all plugins are found and registered at startup, and it works quite well for production.

However there are devops, who reconfigure applications. They need to restart application quite often, because configuration changes registered services. I planned to delay plugin loading until they are required. Simple mock:

// Rebuild controller if plugins were loaded
public class MyControllerActivator : IControllerActivator
{
    public object CreateController(ControllerContext context)
    {

        try
        {
            propertyActivator.Activate(context, controller);
        }
        catch(RebuildControllerException ex)
        {   // ofc this needs more logic, probably some loop etc.
              propertyActivator.Activate(context, controller);
        }
    }
} // Probably would have to rebuild lifetimescope too?

// Load plugins on demand
MyController
{
    MyController(MyFeature feature)
    {
        // in constructor check if feature is loaded, so construction can be aborted and redone
        if(!feature.ArePluginsLoaded){ // simply check boolean value
            // locking etc. is skipped, this is just proof of concept
            var containerBuilder = new PluginLoader().LoadPluginsFor(feature);
            containerBuilder.Update(container);
            feature.ArePluginsLoaded = true;
            throw new RebuildControllerException(); 
        }
    }
}

What about diagnostics? I could load all plugins during integration tests and check diagnostic information there, just like automapper is doing with MapperConfiguration.AssertConfigurationIsValid()

Why not register everything at startup? My app needs to hit multiple databases, and eventually pull plugins from external repository, and people need to stare at their screens instead of doing something productive. Also debug symbols have to be loaded when debugging / assembly scanning, so it slows even more for developers.

Why not use multiple containers? I would like to have plugins seamlessly integrated in extensions points. When using second container I couldn't reuse existing services from first container, or if I rebuilt it would have separate singletons etc.

stevejbrother commented 7 years ago

Hi,

I thought that I might put in my 2 cents here about the Update, and why I might see a potential problem here. I was just faced with an issue using Prism-Forms in my Xamarin app, and I'm using Autofac as the IoC/DI container.

One of the issue I was facing is that I needed to implement ISavePicture on an Android device because I needed to have the photo registered with the Android device Gallery so this way, the user can see the photo he's taken in his own device's gallery. To make it short, I used James Montemagno's plugin for the Media. but it wasn't persisting the photo to the device's gallery. So Instead, I tried using DependencyService.

Didn't work.

I tried using the Prism's implementation of DependencyService with Autofac, and that didn't work!

It wasn't until in I talked to a few people stating that in Autofac, you CANNOT add another RegisterType once the container has been built! That could explain why I kept getting Unhandled Exceptions being thrown. Another words, my app was doomed until someone pointed out that I needed to use Update the container.

So doing a DependencyService with a constructor injection NEVER would have worked as I hoped.

So then, Instead of immediately using a constructor injector to inject the Android implementation's of ISavePicture, I had to use IPlatformInitializer, like this:

public class AndroidInitializer : IPlatformInitializer
{
    public void RegisterTypes(IContainer container)
    {
        var temp = new ContainerBuilder();

        temp.RegisterType<SavePicture_Android>().As<ISavePicture>();
        temp.Update(container);

And it worked like a charm.

So if you take away that functionality, then how are people going to be able to update the container at another future point like I had to do so here in my Xamarin app?

Just my 2 cents!

tillig commented 7 years ago

@ShadowDancer Your setup looks like yet another situation that could be pretty easily remedied by using lambda registrations and maybe a custom registration source.

Consider a design where one or more of these things happen:

Obviously I can't design the whole thing for you, but here's one idea to get the juices flowing. In this example, you have single-instance features (so you don't have to check whether the plugins were loaded) combined with Lazy<T> to actually execute the resolution of things that may take time.

public class MyFeatureModule : Module
{
  protected override void Load(ContainerBuilder builder)
  {
    // The feature is a singleton so it'll only load plugins once.
    builder.RegisterType<MyFeature>()
      .OnActivating(e => e.Instance.LoadPlugins())
      .SingleInstance();

    // Here's an imaginary expensive-to-create plugin
    // that talks to a database. It's specifically associated
    // with the feature via metadata.
    builder.Register(ctx =>
      {
        var db = ctx.Resolve<DbContext>();
        var pluginName = db.GetPlugin("key");
        return PluginFactory.LoadPlugin(pluginName);
      })
      .As<IPlugin>()
      .WithMetadata("feature", "MyFeature")
      .SingleInstance();
  }
}

public class MyFeature
{
  private Meta<Lazy<IPlugin>>[] _plugins;
  public MyFeature(IEnumerable<Meta<Lazy<IPlugin>>> allPlugins)
  {
    // The feature only loads plugins that are associated.
    this._plugins = allPlugins.Where(p => p.Metadata["feature"].Equals("MyFeature")).ToArray();
  }

  public void LoadPlugins()
  {
    foreach(var p in this._plugins)
    {
      // By getting the Lazy<T> out of the metadata object
      // and resolving the value it instantiates the singleton
      // that was registered in the module.
      var resolved = p.Value.Value;
    }
  }
}

By virtue of resolving the MyFeature class it auto-loads only the plugins associated with the feature, accomplishing the lazy-load like you wanted, but also not requiring on-the-fly container updates.

If you want to get fancier, you can look at examples of how IRegistrationSource is implemented (like the AnyConcreteTypeNotAlreadyRegisteredSource) and potentially implement on-the-fly registrations that way - when the container asks "is XYZ registered?" you can dynamically respond as needed. That's a little more involved but is a second idea.

I think with the creative application of some of these concepts it would be possible to get what you want without modifying the container.