Discussion: Making container and scopes truly immutable after build

[eugbaranov]

I really like that Autofac discourages from modifying built container/scope without creating a new scope but currently it can be easily circumvented (admittedly in a ugly way):

using (var container = builder.Build())
{
    container.ComponentRegistry.Register(RegistrationBuilder.ForType<A>().CreateRegistration());
    var a = container.Resolve<A>();
    using (var scope = container.BeginLifetimeScope())
    {
        scope.ComponentRegistry.Register(RegistrationBuilder.ForType<B>().CreateRegistration());
        var b = container.Resolve<B>();
    }
}

I haven't looked at how much effort it might take but would you consider tightening this in a such way so that ComponentRegistry cannot be modified directly?

Two possible approaches I could think of on top of my head are to either make ComponentRegistry property or its Register* methods internal. Obviously a breaking change but maybe something that can be marked as obsolete as get removed over time?

[tillig]

Sorry, looks like we never responded here. Yeah, this would be an interesting thing to figure out, but I think it'd be a pretty big change internally. It'd be interesting to see what it'd take.

[weelink]

I'm willing to give this feature a try.

There are two options to consider:

1. I would expect the IComponentRegistry to be the read-only part and provide a new way of adding IComponentRegistrations. The advantage is that it will improve the API, but it is going to be a massive breaking change (e.g. IModule.Configure(IComponentRegistry) can no longer be supported).

2. An alternative is to keep IComponentRegistry as-is, and add something new for the immutable parts. This is probably the safer option, as far as API changes are concerned, but it does mean going for a suboptimal solution that will haunt Autofac for years.

I prefer 1 and see if it can be implemented in such a way that there is an acceptable middleground until the deprecated methods/properties are officially removed. Perhaps as a separate NuGet package containing Autofac.RegistrationApi.BackwardsCompatible that will help in a smooth transition.

The power I have as a pre-contributor of this project are too limited to be able to make this decision on my own. Any input is appreciated.

[tillig]

I don't have specific guidance on how to do this because as you've seen it's pretty big. Things I'd be thinking about, in no particular order, and maybe of varying value...

• This would probably be a 5.0 or beyond change. As such, I wouldn't worry so much about breaking API changes. I mean, try to minimize it, but the notion of the immutable container is already pretty breaking along with making ContainerBuilder.Update obsolete (Discussion: ContainerBuilder.Update Marked Obsolete #811) so we'd probably do that at the same time.
• Child lifetime scope registrations and fallback may be challenging. When you start a child lifetime scope, the way additional registrations get handled is sort of interesting and, honestly, has caused a lot of trouble. That is, the whole thing where you pass a builder lambda into BeginLifetimeScope - the scopes get handled pretty differently depending on whether or not you pass in a new set of registrations. It would be super nice to simplify this despite how it would be a big change internally. It's relevant here because whatever ends up happening, there's a need to add stuff to a "child scope registry" and still have fallback support to the parent registry.
• Modules and sources may be challenging. You already noticed that doing something to support configuration modules may be difficult or need to change. Configuration sources - the things that support Func<T>, Lazy<T>, etc. - may also be a pain. Depending on the solution to how registration changes, sources generally get evaluated last, after concrete registrations of specific types or lambdas. If the registration API changes, it's possible that necessitates differences in how resolutions get made, so that's something to keep in mind.
• Registration order/resolution order is important. Due to the Microsoft.Extensions.DependencyInjection conforming container making the assumption that the order things are registered is the same order as they are resolved in an IEnumerable<T> scenario, that will need to continue working. We had to make changes to ensure that conforms.
• Fluent registration is still desirable. The whole way we use the builder to register things in a nice, fluent syntax is valuable. It may or may not still be ContainerBuilder, but being able to retain that sort of syntax is key.
• I personally like the collection mechanism from MEDI. Microsoft.Extensions.DependencyInjection has this IServiceCollection registration mechanism that allows you to poke around in it and modify the registrations after they've been made but before you build the container. Our current builder callback support doesn't have that control. I've actually had to use the MEDI collection mechanism to allow Autofac to hook into some places in ASP.NET Core; it'd be nice to offer something similar to our consumers.
• It still needs to stand alone. I like that IServiceCollection thing, but Autofac needs to stand alone without a dependency on that. If we end up with a collection mechanism, it has to be our interface and our implementation. We can update our Autofac.Extensions.DependencyInjection integration if we need to do conversions or something.
• Look for ways to improve diagnostics and performance. People have asked for ways to see what's registered in the container; to try doing container analysis (though I don't think that's really possible with registration sources and modules in play); for ways to see what is getting resolved and when; for improvements in caching, locking, and so on. If the registration API changes how things are stored and retrieved, having before and after benchmarks will be important. If there are ways to improve perf, all the better. If there are ways to add better diagnostics based on the internal changes, all the better.

That's a lot, and like I said, it may or may not all be useful here, but it'd be stuff I'm keeping in the back of my head as I go. The registration API will likely have a sort of trickle-down effect as it changes the way things get registered... then get stored during resolution... and so on.

[weelink]

This is going to be done in baby steps. I will update this list as the progress continues:

Notes

• The tracking of IComponentRegistration and ServiceRegistrationInfo is being used during registration and resolving, so it needs to be in a separate class in order to reuse it

Tasks

• Split up the API into a immutable and mutable interface, but keep ComponentRegistry intact.
• Extract the tracking of registered services into a dedicated class.
• Improve StartableManager so that it doesn't require modifying IComponentRegistry.Properties.
• Make ComponentRegistry immutable and move all mutable actions in a ComponentRegistryBuilder.
• Administration (i.e. update documentation, add/remove unit tests etc.).

WIP

https://github.com/weelink/Autofac/tree/feature/976_FixDuplicateTagsWhenBeginningLifetimeScope

[eugbaranov]

One of the benefits to have immutable ComponentRegistry would be reduced number of locks requires, e.g. in Registrations property and similar.

It will most likely require to use some sort of immutable list, @tillig are you willing to take System.Collections.Immutable as a dependency? Alternatively we could write a basic immutable list implementation too.

[tillig]

Could we simply ensure that the interfaces returned to public APIs are read-only like IEnumerable<T>, IReadOnlyCollection<T>, or IReadOnlyList<T> instead of IList<T> in places? If not, I suppose using immutable collections are fine. It'd be nice to keep the direct dependencies limited where possible.

[eugbaranov]

Exposing e.g. IEnumerable<T>, the question is whether to use existing implementation behind the scenes or have a basic internal implementation. Probably worth staring with a basic and see it it's enough.

[weelink]

I've tried to turn IComponentRegistry.Properties into an IReadOnlyDictionary. It almost works. The only problem left is the test ContainerBuilderTests.RegistrationsCanUsePropertyBag.

builder.Register(ctx =>
{
    // TOTALLY not thread-safe, but illustrates the point.
    var count = (int)ctx.ComponentRegistry.Properties["count"];
    count++;
    ctx.ComponentRegistry.Properties["count"] = count;
    return "incremented";
}).As<string>();

There is also a mutable dictionary in IComponentRegistration.Metadata which I haven't checked yet.

Some possible options:

1. Leave IComponentContext.Properties a mutable Dictionary for now
2. Add an overload to RegistrationExtensions.Register: builder.Register(Func<IComponentContext, IDictionary<string, object>, T>
3. Separate IComponentContext into two interfaces. One where it can be mutated, and a readonly one to be used after it has been built.
4. Maybe there is a different approach

I would like to have option 3, but the PR for this issue is already going to be big.

[tillig]

Registrations may be immutable, but the properties should be mutable. Unclear if the properties need to be attached to the component registry or could come from somewhere else, but I don't have any suggestions for alternatives.

[tillig]

With the release of 5.0 containers are truly immutable after build. If there's additional work that should be done to this effect, please open a new issue with more specifics. Thanks!