Adding decorator moves OnActivated event invocation

[srogovtsev]

Given the following service/container setup:

public class OuterService
{
    public OuterService(IMiddleService middle) => Console.WriteLine("Outer");
}

public interface IMiddleService {}

public class MiddleService : IMiddleService
{
    public MiddleService(InnerService inner) => Console.WriteLine("Middle");
}

public class InnerService
{
    public InnerService() => Console.WriteLine("Inner");
}

builder.RegisterType<OuterService>().SingleInstance();
builder.RegisterType<MiddleService>().SingleInstance().As<IMiddleService>();
builder
    .RegisterType<InnerService>()
    .SingleInstance()
    .OnActivated(_ => Console.WriteLine("Inner activated"))
    ;

I observe the following output on resolving OuterService:

Inner
Middle
Outer
Inner activated

Note that activation event fires last (which allows us, for example, resolve circular dependency, were Inner to depend on Outer).

Now we add a decorator (everything else stays the same):

public class Decorator : IMiddleService
{
    public Decorator(IMiddleService inner) => Console.WriteLine("Decorator");
}

builder.RegisterDecorator<Decorator, IMiddleService>();

The output is:

Inner
Middle
Decorator
Inner activated
Outer

Now the activation event fires after decorator, not the last in the pipeline (which in our case brings back circular dependency).

Observed on Autofac 6.2 and 6.5.

[tillig]

Well that's... weird. Admittedly, it's been a while since I've looked at the events and ordering, I'd have guessed that you'd get

Inner
Inner activated
Middle
Decorator
Outer

Like, OnActivated would occur closer to the point at which the component itself was activated, not at the point where the entire resolution chain was completed.

I'll agree that changing the order seems like a bug. I haven't had a chance to run the code myself, but the repro looks straightforward and I trust you. ;)

I don't have any ideas on easy fixes off the top of my head. It'll be something to dig into. If you have the time and inclination, we'd take a PR with a fix. Probably need to add some tests to make sure it doesn't get regressed later.

[srogovtsev]

I'd have guessed that you'd get

Inner
Inner activated
Middle
Decorator
Outer

To be completely honest, I originally expected Activated to always fire after Inner, no matter the decorator, but then it occurred to me that we wouldn't be able to jump around the circular dependency this way.

I don't have any ideas on easy fixes off the top of my head.

To be frank, I couldn't even easily find the place where it gets fired (I originally debugged this on our own application, which is kinda complex, the diagnostic trace for this particular failing resolve operation is 2.5 Mb in a text file). We're currently working around this on our side by breaking the circular dependency (which is, honestly, a better option anyway, and I'm glad it got surfaced), but I thought I should put this out for somebody else who might hit the same wall.

I try giving it a go later, though.

[alistairjevans]

The reason for OnActivated firing at the end of everything (without a decorator) is actually to preserve legacy behaviour (pre 5.0 I think) where all Activated events fired when all services in a dependency chain had been resolved, just before the resolve completes; this permits resolves that happen inside the OnActivated callback to make their own resolves without triggering circular reference errors.

Autofac/src/Autofac/Builder/RegistrationBuilder{TLimit,TActivatorData,TRegistrationStyle}.cs

Lines 525 to 533 in 0c79d7b

	// In order to behave in the same manner as the original activation handler,
	// we need to attach to the RequestCompleting event so these run at the end after everything else.
	ctxt.RequestCompleting += (sender, evArgs) =>
	{
	var ctxt = evArgs.RequestContext;
	var args = new ActivatedEventArgs<TLimit>(ctxt, ctxt.Service, ctxt.Registration, ctxt.Parameters, newInstance);
	handler(args);
	};

The reason this changes when the decorator gets involved is because the decorator middleware creates a new ResolveRequest for the Inner resolve, so the decorator itself can have it's own circular dependency set.

Autofac/src/Autofac/Features/Decorators/DecoratorMiddleware.cs

Lines 109 to 120 in 0c79d7b

	var resolveRequest = new ResolveRequest(
	_decoratorService,
	new ServiceRegistration(ServicePipelines.DefaultServicePipeline, _decoratorRegistration),
	resolveParameters,
	context.Registration);
	using (dependencyTrackingResolveOperation.EnterNewDependencyDetectionBlock())
	{
	var decoratedInstance = context.ResolveComponent(resolveRequest);
	context.Instance = decoratedInstance;
	}

It does look weird though, I grant you, but it's because of the decorator supporting circular dependencies inside itself.

We could change OnActivated to always have the behaviour of firing all those events at the end, by attaching to ctxt.Operation.CurrentOperationEnding in OnActivated instead of ctxt.RequestCompleting, but I'm not sure that's the desired behaviour either.

[tillig]

Ah, right, some of that is coming back to me now. Why it's happening makes sense in this context, but I'll also agree that it's potentially unexpected. Hmmm. Not sure how to rectify those two things. Perhaps it's a matter of documentation more than code fix? I'm not sure we've actually got documentation on how decorators affect the lifetime event chain and I don't see any tests that enforce event ordering, just that they happened. Maybe this is the opportunity to designate what we believe the correct ordering to be in a complex resolve chain and enforce that? (Even if the answer is "it's functioning correctly right now?")

[srogovtsev]

this permits resolves that happen inside the OnActivated callback to make their own resolves without triggering circular reference errors.

Yes, this is exactly what we do now: we resolve something in OnActivated, and then assign it to instance property.

We could change OnActivated to always have the behaviour of firing all those events at the end, by attaching to ctxt.Operation.CurrentOperationEnding in OnActivated instead of ctxt.RequestCompleting, but I'm not sure that's the desired behaviour either.

I think that'd be the desired behavior (if by "at the end" you mean "at the end of the whole stack, after outer service), but we should check what it breaks. It definitely doesn't break the outer service (more specifically, in our case it would fix it), but is there any decorator-related scenario where it will break the decorator? I cannot come up with one right now.

[alistairjevans]

I think you may be right about it being the desired behaviour generally, I'm trying to think whether someone could do things in OnActivated that could impact the decorator, but I don't believe so...

I don't have time right now to try the change out @srogovtsev, do you want to try that OnActivated change and see if any tests fail?

[srogovtsev]

I'll try to, ahem, try this out later this week.

[srogovtsev]

by attaching to ctxt.Operation.CurrentOperationEnding in OnActivated instead of ctxt.RequestCompleting

In short, it doesn't work this way: we don't have IComponentContext in CurrentOperationEnding, which we need to pass to OnActivated, and capturing one in-place of course doesn't help because it gets closed before we reach CurrentOperationEnding.

The simple workaround would be to just resolve IComponentContext from ctxt in-place (as it's recommended inside lambda factories and handlers), but that would be a behaviour change from what we have now.

I'll keep digging.

[alistairjevans]

You could create a new request context from the operation for each activated handler?

[srogovtsev]

Like this?

ctxt.Operation.CurrentOperationEnding += (sender, evArgs) =>
        {
            var surrogateContext = new DefaultResolveRequestContext(evArgs.ResolveOperation,
                evArgs.ResolveOperation.InitiatingRequest, evArgs.ResolveOperation.CurrentScope,
                evArgs.ResolveOperation.DiagnosticSource);
            var args = new ActivatedEventArgs<TLimit>(surrogateContext, ctxt.Service, ctxt.Registration, ctxt.Parameters, newInstance);

            handler(args);
        };

Still doesn't help, same error ("This resolve operation has already ended"). This kinda makes sense, because in

Autofac/src/Autofac/Core/Resolving/ResolveOperation.cs

Lines 265 to 275 in 0c79d7b

	private void End(Exception? exception = null)
	{
	if (_ended)
	{
	return;
	}
	_ended = true;
	var handler = CurrentOperationEnding;
	handler?.Invoke(this, new ResolveOperationEndingEventArgs(this, exception));
	}

we first "end" the operation, and then fire the Ending event.

It works, though, if I switch the assignment and invocation (i.e., first invoke the events, and then "end" the operation), but I need to think on the consequences. Also, all the remaining tests pass but one:

Autofac/test/Autofac.Specification.Test/Lifetime/LifetimeEventTests.cs

Lines 75 to 92 in 0c79d7b

	[Fact]
	public void ChainedOnActivatingEventsAreInvokedWithinASingleResolveOperation()
	{
	var builder = new ContainerBuilder();
	var secondEventRaised = false;
	builder.RegisterType<object>()
	.Named<object>("second")
	.OnActivating(e => secondEventRaised = true);
	builder.RegisterType<object>()
	.OnActivating(e => e.Context.ResolveNamed<object>("second"));
	var container = builder.Build();
	container.Resolve<object>();
	Assert.True(secondEventRaised);
	}

Will keep digging.

[srogovtsev]

In fact, there's a much more visible bug arising from the same logic (we just don't use PropertiesAutowired):

private class OuterService
{
    public OuterService(IInnerService service)
    {
    }
}

private interface IInnerService
{
}

private class InnerService : IInnerService
{
    public InnerService(InnermostService service)
    {
    }
}

private class InnerServiceDecorator : IInnerService
{
    public InnerServiceDecorator(IInnerService toDecorate)
    {
    }
}

private class InnermostService
{
    public OuterService Outer { get; set; }
}

var builder = new ContainerBuilder();

builder.RegisterType<OuterService>()
    .SingleInstance();
builder.RegisterType<InnerService>()
    .As<IInnerService>();
builder.RegisterType<InnermostService>()
    .SingleInstance()
    .PropertiesAutowired(PropertyWiringOptions.AllowCircularDependencies);

builder.RegisterDecorator<InnerServiceDecorator, IInnerService>();

using var container = builder.Build();
container.Resolve<OuterService>();

Fails with

The constructor of type 'Autofac.Specification.Test.Features.CircularDependencyTests+OuterService' attempted to create another instance of itself. This is not permitted because the service is configured to only allowed a single instance per lifetime scope.

which is exactly how we stumbled upon this in the first place (we just don't use PropertiesAutowired a lot, preferring calling initialization methods in OnActivated).

Considering that PropertiesAutowired and OnActivated are the only two places (in the main repository) that rely on RequestCompleting, and both seem to break in the same way, shouldn't we try changing the RequestCompleting behaviour to one that seems to be expected, i.e., run them all on the completion of the outer-most request?