Redundantly decorated service when using nested ContainerBuilder

[demdh]

Autofac 4.9.1

When resolving an decorated service from a LifetimeScope with nested ContainerBuilder,
the return service is decorated twice.

Repro:

namespace AutofacRepro
{
  using Autofac;

  class Program
  {
    static void Main(string[] args)
    {
      var builder = new ContainerBuilder();
      builder.RegisterType<Service>().As<IService>();
      builder.RegisterDecorator<ServiceDecorator, IService>();
      var container = builder.Build();

      ILifetimeScope lifetimeScope = container.BeginLifetimeScope(
        containerBuilder =>
        {
          containerBuilder.RegisterType<SomeAdditionalComponent>().AsSelf();
        });

      // The service (Foo) contained in the decorator is surrounded by two decorators.
      var foo = lifetimeScope.Resolve<IService>();

      // When resolved via container or via a LifetimeScope create without nested ContainerBuilder
      // Resolve will returns a single decorated service.
    }
  }

  public interface IService { }

  public class Service : IService { }

  public class ServiceDecorator : IService
  {
    private readonly IService service;

    public ServiceDecorator(IService service)
    {
      this.service = service;
    }
  }

  public class SomeAdditionalComponent { }
}

[tillig]

Verified - I'm able to reproduce this. Unclear at this moment exactly what's causing the issue, but since we can repro it, we can troubleshoot it. If you happen to be also troubleshooting and figure it out, let us know what you find.

[tillig]

Notes as I continue research here.

The difference between starting a scope with and without a lambda registration...

// without lambda
container.BeginLifetimeScope();
// with lambda
container.BeginLifetimeScope(b => { });

... is that without the lambda the lifetime scope uses a CopyOnWriteRegistry as the component registry; but for the one with the lambda it's a ScopeRestrictedRegistry.

Admittedly, this has been a source of struggle for a few different issues.

The primary difference is that when the ScopeRestrictedRegistry is created, the components from the parent are brought forward into the registry using an ExternalRegistrySource and adapter delegate.

If I set a breakpoint at the point where decorators are being applied then I see two different behaviors.

With a no-lambda lifetime scope (CopyOnWriteRegistry) I only see decorators located one time - the component (Service) has a decorator found, but the decorator itself has no additional decorators located.

Autofac.Features.Decorators.InstanceDecorator.TryDecorateRegistration(Autofac.Core.IComponentRegistration registration, object instance, Autofac.IComponentContext context, System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> parameters) Line 56
Autofac.Core.Resolving.InstanceLookup.Activate(System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> parameters, out object decoratorTarget) Line 124
Autofac.Core.Resolving.InstanceLookup.Execute() Line 85
Autofac.Core.Resolving.ResolveOperation.GetOrCreateInstance(Autofac.Core.ISharingLifetimeScope currentOperationScope, Autofac.Core.IComponentRegistration registration, System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> parameters) Line 130
Autofac.Core.Resolving.ResolveOperation.ResolveComponent(Autofac.Core.IComponentRegistration registration, System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> parameters) Line 69
Autofac.Core.Resolving.ResolveOperation.Execute(Autofac.Core.IComponentRegistration registration, System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> parameters) Line 83
Autofac.Core.Lifetime.LifetimeScope.ResolveComponent(Autofac.Core.IComponentRegistration registration, System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> parameters) Line 276

With a lambda configured scope (ScopeRestrictedRegistry) I see the decoration logic get hit twice. The first time, I can see it pass through the delegate activator provided by the ExternalRegistrySource.

Autofac.Features.Decorators.InstanceDecorator.TryDecorateRegistration(Autofac.Core.IComponentRegistration registration, object instance, Autofac.IComponentContext context, System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> parameters) Line 56
Autofac.Core.Resolving.InstanceLookup.Activate(System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> parameters, out object decoratorTarget) Line 124
Autofac.Core.Resolving.InstanceLookup.Execute() Line 85
Autofac.Core.Resolving.ResolveOperation.GetOrCreateInstance(Autofac.Core.ISharingLifetimeScope currentOperationScope, Autofac.Core.IComponentRegistration registration, System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> parameters) Line 130
Autofac.Core.Resolving.InstanceLookup.ResolveComponent(Autofac.Core.IComponentRegistration registration, System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> parameters) Line 188
Autofac.Core.Registration.ExternalRegistrySource.RegistrationsFor.AnonymousMethod__2(Autofac.IComponentContext c, System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> p) Line 83
Autofac.Core.Activators.Delegate.DelegateActivator.ActivateInstance(Autofac.IComponentContext context, System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> parameters) Line 71
Autofac.Core.Resolving.InstanceLookup.Activate(System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> parameters, out object decoratorTarget) Line 122
Autofac.Core.Resolving.InstanceLookup.Execute() Line 85
Autofac.Core.Resolving.ResolveOperation.GetOrCreateInstance(Autofac.Core.ISharingLifetimeScope currentOperationScope, Autofac.Core.IComponentRegistration registration, System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> parameters) Line 130
Autofac.Core.Resolving.ResolveOperation.ResolveComponent(Autofac.Core.IComponentRegistration registration, System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> parameters) Line 69
Autofac.Core.Resolving.ResolveOperation.Execute(Autofac.Core.IComponentRegistration registration, System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> parameters) Line 83
Autofac.Core.Lifetime.LifetimeScope.ResolveComponent(Autofac.Core.IComponentRegistration registration, System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> parameters) Line 276

The second time through, the call stack is identical to the earlier/simpler stack that doesn't go through the ExternalRegistrySource.

Basically, the delegate inside the ExternalRegistrySource is decorating the component... and then once that's resolved, the current scope is also then trying to decorate the component.

I'm not sure as yet the right way to stop that from happening, but at least I'm getting closer to figuring out the root cause.

[tillig]

I added a failing unit test marked as skip for this issue.

I think there are a couple of potential solutions here.

One would be to somehow signal to either the ScopeRestrictedRegistry or the ExternalRegistrySource that it shouldn't include decorators. That would make it so things don't try to decorate the already applied decorators. I'm not sure how that would affect application of decorators that are registered as part of that lambda in BeginLifetimeScope but we have some tests around that.

Another option might be to improve the overall resolve/decoration mechanism to track the set of decorators and ensure the same decorator registration isn't applied twice to the same resolve op. I think this would potentially impact memory usage and performance since there'd be more to pass around on each resolve and the check would happen for each decorator being applied. It could be an expensive add-on to handle a case that's potentially not as frequent.

One challenge with the latter option of tracking the applied decorators is that technically the two decorations are happening separate from each other. We do have a decoration context we use to track the series of applied decorator instances, but one operation is happening inside that ExternalRegistrySource wrapped lambda; and after that the decoration context is gone because it's a whole separate decoration operation happening in the CopyOnWriteRegistry.

I do see there's an exception in the ExternalRegistrySource where it doesn't include components that are adapters for other registrations - that is, Func<T> is an adapter for T so that's not wrapped. I wonder if decorators might fall into that same bucket.

I could definitely use some input from @alexmg on this one. I'm not sure if there's something he's already seen or thought of during the creation of this new decorator mechanism.

[tillig]

I tried updating the RegisterDecorator<T, U>() method temporarily so the internal registration of the decorator service would set a target (Targeting(IComponentRegistration)) and have the ExternalRegistrySource consider it an adapter, thus excluding it from being wrapped. No luck. It not only didn't fix the issue, but broke other things. I see why that happened - it basically means it didn't include it from the parent registry... but also didn't consider it in the child scope registry. Hmmm.

[alexmg]

I expanded some of the other tests to include a type check on the inner service and found the double decoration issue also applied to adapters like Func<T> and Lazy<T>. Those tests were only asserting the directly resolved type and not the full decorator chain. That one extra assertion can sometimes make a big difference to a test.

Because the issue is specific to the decorator feature I made the fix in the InstanceDecorator class, where it now skips decoration if the registration is for an adapter (registration.IsAdapting()) or if the decorator has already been applied (registration.Activator.LimitType != instance.GetType()). I created the InstanceDecorator class to help consolidate as much decorator related knowledge into the one spot as possible so it seems like the best spot for the fix.

[tillig]

Is there a use case where the same decorator type might actually need to be applied twice? For example, a logging decorator that might include additional information for nested lifetime scopes or something.

You might also want to add a test where the container applies two decorators - resolution in a child scope should avoid duplicating both of them.

[alexmg]

Excellent test suggestions @tillig. There is plenty of craziness one might choose to get up to. 😄

The tests below intentionally add a second decorator of the same type, one in the root lifetime scope, and the other only in a child lifetime scope. These and the open generic equivalents seem to be holding up. I was rather relieved to find that to be the case.

[Fact]
public void DecoratorCanBeAppliedTwice()
{
    var builder = new ContainerBuilder();
    builder.RegisterType<ImplementorA>().As<IDecoratedService>();
    builder.RegisterDecorator<DecoratorA, IDecoratedService>();
    builder.RegisterDecorator<DecoratorA, IDecoratedService>();
    var container = builder.Build();

    var service = container.Resolve<IDecoratedService>();

    Assert.IsType<DecoratorA>(service);
    Assert.IsType<DecoratorA>(service.Decorated);
    Assert.IsType<ImplementorA>(service.Decorated.Decorated);
}

[Fact]
public void DecoratorCanBeAppliedTwiceInChildLifetimeScope()
{
    var builder = new ContainerBuilder();
    builder.RegisterType<ImplementorA>().As<IDecoratedService>();
    builder.RegisterDecorator<DecoratorA, IDecoratedService>();
    var container = builder.Build();

    var scope = container.BeginLifetimeScope(b => b.RegisterDecorator<DecoratorA, IDecoratedService>());
    var scopeInstance = scope.Resolve<IDecoratedService>();

    Assert.IsType<DecoratorA>(scopeInstance);
    Assert.IsType<DecoratorA>(scopeInstance.Decorated);
    Assert.IsType<ImplementorA>(scopeInstance.Decorated.Decorated);

    var rootInstance = container.Resolve<IDecoratedService>();
    Assert.IsType<DecoratorA>(rootInstance);
    Assert.IsType<ImplementorA>(rootInstance.Decorated);
}

[alexmg]

I added one more unit test with intentionally duplicated decorators and the empty builder lambda being provided when creating the child lifetime scope.

[alexmg]

This fix is included in 4.9.2 on NuGet.