Enable build callbacks to run when registered in child lifetime scopes

[Nava2]

When registering a Build Callback (i.e. RegisterBuildCallback) within a lifetime scope secondary registration do not get called.

I apologize if this is incorrect usage.

packages.config:

<?xml version="1.0" encoding="utf-8"?>
<packages>
  <package id="Autofac" version="4.9.2" targetFramework="net471" />
  <package id="xunit" version="2.4.1" targetFramework="net471" />
  <package id="xunit.abstractions" version="2.0.3" targetFramework="net471" />
  <package id="xunit.analyzers" version="0.10.0" targetFramework="net471" />
  <package id="xunit.assert" version="2.4.1" targetFramework="net471" />
  <package id="xunit.core" version="2.4.1" targetFramework="net471" />
  <package id="xunit.extensibility.core" version="2.4.1" targetFramework="net471" />
  <package id="xunit.extensibility.execution" version="2.4.1" targetFramework="net471" />
</packages>

using System;
using Xunit;

namespace Autofac.Tests
{
    public class Tests
    {
        [Fact] // Passes successfully
        public void RootCallbackThrows_ExpectThrows()
        {
            var builder = new ContainerBuilder();

            var expected = new Exception();
            builder.RegisterBuildCallback(c => throw expected);

            var ex = Assert.Throws<Exception>(() => builder.Build());
            Assert.Same(expected, ex);
        }

        [Fact] // Failure - no exception thrown
        public void NestedCallbackFromDelegateThrows_ExpectThrows()
        {
            var builder = new ContainerBuilder();

            using (var outer = builder.Build())
            {
                var expected = new Exception();

                var ex = Assert.Throws<Exception>(
                    () => outer.BeginLifetimeScope(b => { b.RegisterBuildCallback(c => throw expected); }));
                Assert.Same(expected, ex);
            }
        }

        [Fact] // Failure - no exception thrown
        public void NestedCallbackFromModuleThrows_ExpectThrows()
        {
            var builder = new ContainerBuilder();

            using (var outer = builder.Build())
            {
                var expected = new Exception();

                var ex = Assert.Throws<Exception>(
                    () => outer.BeginLifetimeScope(b => { b.RegisterModule(new CallbackModule(expected)); }));
                Assert.Same(expected, ex);
            }
        }

        private sealed class CallbackModule : Module
        {
            private readonly Exception _expectedException;

            public CallbackModule(Exception expectedException)
            {
                _expectedException = expectedException;
            }

            protected override void Load(ContainerBuilder builder)
            {
                base.Load(builder);

                builder.RegisterBuildCallback(c => throw _expectedException);
            }
        }
    }
}

[tillig]

I kind of feel like this is "functions as designed" - the callbacks are for container build time (hence the name) but a nested lifetime scope isn't building a container. I can see how it's potentially confusing since the builder is how things are registered in nested scopes, but it's building on top of an existing container.

That said, it's a possible future enhancement we may be able to add, so I'll leave the issue open and if a PR comes in for it, we can check it out.

[Nava2]

I can see how it's potentially confusing since the builder is how things are registered in nested scopes, but it's building on top of an existing container.

We have a scenario where we are trying to avoid registering module repeatedly due to time. In previous container frameworks (e.g. Guice), this was the use case for the "life-time scope"-like pattern. Is there a pattern that I'm missing from the documentation that covers this? I can move this to StackOverflow should that be more appropriate.

[tillig]

Yes, I think SO will get you help faster since you can include some context there around why you're trying to about registering the module and why things like "instance per lifetime scope" won't work.

[weelink]

Since IContainer is a specialized ILifetimeScope it would make sense if we see the same hierarchy in the builders. There could be a LifetimeScopeBuilder that has most of the current features of a ContainerBuilder and have ContainerBuilder extend LifetimeScopeBuilder to include build callbacks.

I created a PR for this solution to see what it would take and what it would look like, if this is useful #1049 .

The only question remaining is what to do when a module is registered. It could be enough to throw an exception if RegisterBuildCallback(Action<IContainer>) is called from a module, but that always feels a bit dirty to me.

I've added a commit to my PR where the hierarchy is also present in the modules. A ContainerModule that can only be used for building a container and a LifetimeScopeModule that can be used for starting a lifetime scope and building a contianer.
It means that developers have to think about in which situations it should be allowed to use their modules.

As you can see in the PR it could be too much. I'm not entiry sure what to think of this solution.

[tillig]

Let's talk about ways to solve this since it'd be nice to have agreement on that before we head into code land.

Things I can think of:

Throw an exception for RegisterBuildCallback on a lifetime scope

In this case, RegisterBuildCallback at the base/container level would work but if you're adding to the callbacks in a lifetime scope startup then it'd throw an exception.

This would ensure that new callbacks aren't added for a lifetime scope, but throwing for an API call like this seems odd. It may be better if it wasn't there.

Remove RegisterBuildCallback from the API when starting a lifetime scope

Create a similar but not identical API for registrations that can be added to a ContainerBuilder for lifetime scopes. The key difference is RegisterBuildCallback wouldn't be available for a lifetime scope. (This appears to be what PR #1049 went for.)

This would make the API clearer, but could result in some pretty breaking changes and quite a bit of longer-term maintenance due to the complexity of omitting this one call. For example, if there are different builders for lifetime scopes and root containers, that's breaking; and it also means we have an interesting potential divergence long term of what those things mean and how to maintain/test them. Maybe that's OK.

Enable build callbacks in lifetime scopes

The term 'build callback' wouldn't be... really super accurate because lifetime scopes aren't built, but the idea would be to:

• Omit the build callbacks from the properties that get propagated to child scopes when building so they don't get repeated
• Allow a new set of callbacks to be registered during the creation of a lifetime scope
• Execute the callbacks for that scope just before the lifetime scope itself is returned to the client for consumption

I could probably get past the somewhat-inaccurate naming. We might need to update RegisterBuildCallback to take an Action<ILifetimeScope> or add an overload that passes the IContainer version to the ILifetimeScope version if we care about back-compat.

The properties dictionary may need to be updated to allow for a "blacklist" of properties that don't get passed up/inherited. I'd prefer a blacklist over a hard-coded one-off.

The lifetime scope creation tests will get a little more complicated since we'll need to test for those callbacks running, but that's not insurmountable.

I think there's a possibility this is a foot-gun, since someone putting this sort of thing into a per-request lifetime scope isn't going to realize that their poorly-performing "build callback" is eating resources on every single inbound request, but we can document that.

I'd also guess there will be a subsequent desire to have the ability to register a callback that runs for any created scope (like, "I want to register a callback when the container is built that runs when any lifetime scope is created"). It makes me wonder if using the lifetime scope creation events in this solution may somehow be interesting. Unclear. If this was implemented in a way that could be extended to support something like that, it may be valuable.

What else?

Are there other solutions I'm missing?

Let's figure out what the best solution is and we can move forward from there.

[alistairjevans]

I'm not sure if I can think of any additional options.

I don't really have a problem with the proposed approach of changing ContainerBuilder to derive from a new LifetimeScopeBuilder, if we could figure out a way to not have to change the Module class layout. Perhaps we could add another override to the Module class that takes the LifetimeScopeBuilder and we call that method instead? Doesn't really solve the ambiguity issue though.

The problem with allowing a build callback for lifetime scopes is mostly a semantic one I think, but also that mean's there's yet another thing to do when we initialise a lifetime scope. The purpose of build callbacks (from my perspective) was always to register the container into some other entity for integration purposes, whereas you aren't likely to need that for lifetime scopes, which are usually thread-local and available directly when you create it.

[tillig]

Build callbacks definitely get used in... unforeseen ways. I don't think adding a foreach to run the build callbacks on a scope is that big of a deal, but yeah, it's one more thing.

I do sort of cringe at seeing the difference between a "container builder" and a "lifetime scope builder" on this level, just to stop one thing from happening.

Do we know how any of this changes when we factor in the immutable container work? Does the builder construct change enough there that it maybe makes more sense to take this container/scope builder separation?

[alistairjevans]

Taking a look at the changes for immutable container, it looks like there will be some impact at least. But nothing that appears to change the container/scope relationship sufficiently.

Something that has occurred to me though...since startable components are started for objects registered in a nested lifetime scope (only when configuring the new scope), in the same way as a container, could we consider 'build callbacks' to be an internal startable component that we register the first time you call RegisterBuildCallback inside ContainerBuilder?

When this new BuildCallbackService is 'started', we invoke the build callbacks. That way the behaviour will be consistent between scopes and containers.

The only question is what the type passed to the build callback would be...

[tillig]

Making it startable is an interesting thread which may be valuable to pull on a little. Something to consider is you can build a container and skip startable components which would foil this attempt. You can't skip build callbacks.

I think switching a build callback to be on an Action<ILifetimeScope> and away from Action<IContainer> isn't that big of a deal. It may solve a lot of this. There's a value in being able to detect the "root" in various use cases, though, so while it seems there's no difference between a container and a lifetime scope, "being the root" is interesting, and you could ask whether the provided ILifetimeScope is IContainer if it really mattered in the callback.

[alistairjevans]

Perhaps not precisely an 'IStartable', but if we take a variant of StartableManager as a basis (i.e. flagging registrations to prevent re-execution), the same pattern might work for managing the set of callbacks. That still means there would be extra work to do in the lifetime scope setup, but only in the scenario where you reconfigure the scope.

[alistairjevans]

If we add a ''RegisterContainerBuildCallback'' method to the ''ContainerBuilder'', we could take an explicitly typed IContainer callback that is only called for container builds, whereas the regular build callback is invoked regardless?

[tillig]

No, I think RegisterContainerBuildCallback might be the same problem we have now - it's a ContainerBuilder in a lifetime scope and I called RegisterContainerBuildCallback - why wasn't it called?

Easier to explain based on the name but still... no.

I like the startable-ish pattern you're talking about, though.

[alistairjevans]

Fair point. I'll try and find time to put a PR together with the startable approach.