OnActivating()/OnActivated() not playing nicely with RegisterDecorator()

[VonOgre]

First of all, thank you for the decorator support added in 4.9.1 and its subsequent refinements! It's a far cry better than the clunky approaches we'd tried implementing on our own or event the older RegisterDecorator() support from earlier versions.

This seems to be at least somewhat related to #860, in terms of being in a similar or same area of code.

Using Autofac 5.1.2, I noticed that the lifetime events OnActivating() and OnActivated() don't quite work the way that might make the most sense when also using RegisterDecorator().

Here are some unit tests with basic comments on success/failure, and I'll describe more below them:

using Autofac;
using NUnit.Framework;

namespace NextIT.ActiveAgent.Ioc.Test
{
    [TestFixture]
    public class TempTest
    {
        private interface ISample
        {
            string Injectable { get; set; }
        }

        private class Sample : ISample
        {
            public string Injectable { get; set; }
        }

        private class SampleDecorator : ISample
        {
            public readonly ISample Decoratee;

            public string Injectable
            {
                get => Decoratee.Injectable;
                set => Decoratee.Injectable = value;
            }

            public SampleDecorator(ISample decoratee)
            {
                Decoratee = decoratee;
            }
        }

        [Test]
        public void RegisterDecorator_with_OnActivating_decorates_all_relevant_registered_types()
        {
            var builder = new ContainerBuilder();
            builder.RegisterType<Sample>()
                   .As<ISample>()
                   .OnActivating(args => args.Instance.Injectable = "String time!");
            builder.RegisterDecorator<SampleDecorator, ISample>();
            var container = builder.Build();

            //Throws cast exception: Unable to cast object of type 'SampleDecorator' to type 'Sample'
            Assert.That(() => container.Resolve<ISample>(), Throws.Nothing);
            var result = container.Resolve<ISample>();
            Assert.That(result, Is.TypeOf<SampleDecorator>());
        }

        [Test]
        public void RegisterDecorator_plus_OnActivating_interface_workaround_decorates_all_relevant_registered_types()
        {
            var builder = new ContainerBuilder();
            builder.RegisterType<Sample>().AsSelf();
            builder.Register<ISample>(c => c.Resolve<Sample>())
                   .OnActivating(args => args.Instance.Injectable = "String time!");
            builder.RegisterDecorator<SampleDecorator, ISample>();
            var container = builder.Build();


            Assert.That(() => container.Resolve<ISample>(), Throws.Nothing);
            var result = container.Resolve<ISample>();
            Assert.That(result, Is.TypeOf<SampleDecorator>());
            //Success!
        }

        [Test]
        public void RegisterDecorator_with_OnActivated_receives_decorated_instance()
        {
            object activatedInstance = null;
            var builder = new ContainerBuilder();
            builder.RegisterType<Sample>()
                   .As<ISample>()
                   .OnActivated(args => activatedInstance = args.Instance);
            builder.RegisterDecorator<SampleDecorator, ISample>();
            var container = builder.Build();

            Assert.That(() => container.Resolve<ISample>(), Throws.Nothing);
            var result = container.Resolve<ISample>();
            Assert.That(result, Is.TypeOf<SampleDecorator>());
            //Assertion Exception: activatedInstance type = Sample, result type = SampleDecorator
            Assert.That(result, Is.SameAs(activatedInstance));
        }

        [Test]
        public void RegisterDecorator_plus_OnActivated_interface_workaround_receives_decorated_instance()
        {
            object activatedInstance = null;
            var builder = new ContainerBuilder();
            builder.RegisterType<Sample>().AsSelf();
            builder.Register<ISample>(c => c.Resolve<Sample>())
                   .OnActivated(args => activatedInstance = args.Instance);
            builder.RegisterDecorator<SampleDecorator, ISample>();
            var container = builder.Build();


            Assert.That(() => container.Resolve<ISample>(), Throws.Nothing);
            var result = container.Resolve<ISample>();
            Assert.That(result, Is.TypeOf<SampleDecorator>());
            //Assertion Exception: activatedInstance type = Sample, result type = SampleDecorator
            Assert.That(result, Is.SameAs(activatedInstance));
        }
    }
}

With regards to OnActivated(), I would have thought that at least the workaround using Register() would have invoked the handler with the final decorator, as that is the completed instance that has gone through all of the machinations. I can understand how the OnActivated() against Register() could end up being invoked as soon as the concrete instance is constructed, since the relative context is based around that type, though would create an inconsistency compared to the perspective of ISample. I don't know that any of this behavior is a bug, but may shed light on an area of enhancement to allow for performing "post-processing" on an instance after it's been decorated, such as resolving observers to register their event handlers for events exposed by the class/interface being resolved.

The scenario with OnActivating(), on the other hand, only functions when using the Register(c => c.Resolve()) workaround that had been identified a number of years ago and added to the wiki (by a former coworker of mine, no less!). It does look like the OnActivating() handler gets called twice: once with an instance of Sample, and again with an instance of SampleDecorator, which seems potentially undesirable and unexpected. I can see why it would behave that way though, since the container sees SampleDecorator as another ISample that is "activating" though it's part of the same resolution chain/graph...

In the case of Register().As().OnActivating(...), if you register a decorator for ISample, resolving ISample results in this exception:

Autofac.Core.DependencyResolutionException: An exception was thrown while executing a resolve operation. See the InnerException for details. ---> System.InvalidCastException: Unable to cast object of type 'SampleDecorator' to type 'Sample'.
   at Autofac.Builder.RegistrationBuilder`3.<>c__DisplayClass32_0.<OnActivating>b__0(Object s, ActivatingEventArgs`1 e)
   at Autofac.Core.Registration.ComponentRegistration.RaiseActivating(IComponentContext context, IEnumerable`1 parameters, Object& instance)
   at Autofac.Core.Resolving.InstanceLookup.Execute()
   at Autofac.Core.Resolving.ResolveOperation.GetOrCreateInstance(ISharingLifetimeScope currentOperationScope, ResolveRequest request)
   at Autofac.Core.Resolving.ResolveOperation.Execute(ResolveRequest request)
   --- End of inner exception stack trace ---
   at Autofac.Core.Resolving.ResolveOperation.Execute(ResolveRequest request)
   at Autofac.ResolutionExtensions.TryResolveService(IComponentContext context, Service service, IEnumerable`1 parameters, Object& instance)
   at Autofac.ResolutionExtensions.ResolveService(IComponentContext context, Service service, IEnumerable`1 parameters)
   at Autofac.ResolutionExtensions.Resolve[TService](IComponentContext context, IEnumerable`1 parameters)
   at TempTest.<>c__DisplayClass3_0.<RegisterDecorator_with_OnActivating_decorates_all_relevant_registered_types>b__1() in TempTest.cs:line 49

In that unit test, the OnActivating() handler is called with an instance of Sample, which succeeds, but the same behavior of the handler being invoked with the decorator instance next fails with the invalid cast exception.

I'll do some more investigation and see if I can scare up any more details, but wanted to get this info out for the sake of gathering opinions/feedback/corrections. My primary concern is in the fact that I work on a product that uses Autofac to facilitate a plugin architecture, where plugins might only have knowledge about some common interfaces in the product itself, and their registrations could indirectly impact each other in unexpected ways and create some difficult debugging adventures. We can close off some doors, as needed to protect the consumers, but it's nice to be able to provide a robust set of capabilities.

• Assembly version information

<ItemGroup>
    <PackageReference Include="Autofac" Version="5.1.2" />
    <PackageReference Include="NUnit" Version="3.12.0" />
</ItemGroup>

[tillig]

Wow, this is... interesting. I definitely think you've found a bug, or at the very least, something super confusing. Nice find! And thanks for the good descriptive unit test repros. It helps make it clear.

Literally right now as I sit here typing I can't think of anything you're doing wrong or any workaround other than what you've already figured out. Keep us posted if you figure it out, and obviously we'll take PRs to fix it if you're so inclined.

/cc @alexmg in case he sees something obvious

[VonOgre]

@tillig - Thanks for the reply! I suppose it's a bit of a mixed bag to find that I'm not doing something backwards, when it means that there's also not a silver bullet fix ;)

I've made a fork and am currently tinkering around, mostly focused on writing tests combining the lifetime events and RegisterDecorator() to explore the behavior and piecing things together from there.

I think the gremlin with OnActivating() lies in InstanceLookup.cs ln105-106, which will raise OnActivating with the newly decorated instance, if decoration was performed. This is after the original instance was already broadcast in its own OnActivating on ln144 when it gets created. This second broadcast is inconsistent with OnPreparing and OnActivated, which are only broadcast with the original registration and undecorated instance, respectively. My gut says that ln105-106 just need to be punted, if for no other reason than to align for consistency. It would also fix the exception test case from my report. If this removal makes sense, I'll move forward with it and have a PR by the end of the weekend, at the latest.

TBH, having all of the lifetime events be raised and operate within the context of the registration the handlers were attached to and the subsequent direct result of that registration (before decoration occurs) does make sense, but there may be some order of operations changes that need to happen, since OnActivated is raised with the undecorated instance AFTER decoration has occurred. That sounds like a much more complicated discussion and set of changes that is worth separating from the above mentioned fix.

[tillig]

Stuff like this makes me think the pipeline concept we're sort of starting to discuss in #1096 is more and more interesting. Keeping track of the correct order of events and ensuring consistency is pretty hard at the moment.

[VonOgre]

I read through the various snippets of discussion and it definitely sounds like such an approach that might be more robust and declarative in the long run, and possibly open the door to more easily introduce support for different design patterns like you noted in #970, as well.