Autofac: Decorator + IfNotRegistered on the decorated type breaks in 6.0

[abe545]

Describe the Bug

If I have a service registration that is registered with IfNotRegistered, and then try to decorate it with RegisterDecorator, the decorator is not resolved from the container when resolving the interface. This is new in autofac 6.0.

Steps to Reproduce

c# public class ReproTest { [Fact] public void Repro() { var builder = new ContainerBuilder(); builder.RegisterType<X>().As<IX>().IfNotRegistered(typeof(IX)); builder.RegisterDecorator<DX, IX>(); using var container = builder.Build(); var instance = container.Resolve<IX>(); Assert.Equal(instance.GetType(), typeof(DX)); } public interface IX { } public class X : IX { } public class DX : IX { public DX(IX wrapped) { } } }

Expected Behavior

I would expect the decorator to be applied to the resolved service, as it was in previous versions.

Dependency Versions

Autofac: 6.0.0

[tillig]

If it's decorated, the type you're going to get when you resolve IX is DX not X. What is the type you're actually getting?

``c# // X will be decorated by DX, soinstancewill // be aDXwithXas thewrapped` parameter.

// This will fail.
Assert.Equal(instance.GetType(), typeof(X));

// This should pass, and is correct.
Assert.Equal(instance.GetType(), typeof(DX));
```

[abe545]

Whoops, sorry I fat fingered my repro case. I'm actually seeing X. I expect DX. I'll update the original comment to be clear.

[abe545]

Also, this works as expected:

  [Fact]
  public void Repro()
  {
    var builder = new ContainerBuilder();
    builder.RegisterType<X>().As<IX>(); //.IfNotRegistered(typeof(IX));
    builder.RegisterDecorator<DX, IX>();
    using var container = builder.Build();
    var instance = container.Resolve<IX>();
    Assert.Equal(instance.GetType(), typeof(DX));
  }

[tillig]

Looks like we're hitting a registration callback ordering issue. Swapping the order of RegisterType and RegisterDecorator allows the test to pass:

```c#
[Fact]
public void IfNotRegistered_CanBeDecorated()
{
var builder = new ContainerBuilder();

        // Note the order here - RegisterDecorator then RegisterType.
        builder.RegisterDecorator<Decorator, IService>();
        builder.RegisterType<ServiceA>().As<IService>().IfNotRegistered(typeof(IService));
        var container = builder.Build();
        var result = container.Resolve<IService>();

        // This works.
        Assert.IsType<Decorator>(result);
    }

```

This sort of overlaps with the other weird ordering problems we've seen with startup stuff and race conditions.

RegisterDecorator<TDecorator, TService> is added as a callback so it can run after the main registrations are done being added.

However, IfNotRegistered also ends up being a callback because you want to make sure it only runs if the thing in question wasn't added to the registry.

It's almost like we need to have more control over ordering the callbacks but... it's a sticky situation. I don't have an answer off the top of my head.

In the meantime, you can work around the issue by reordering the registrations.

[tillig]

Added a skipped test for this so we can get troubleshooting on it easier.

[nycdotnet]

Thanks

[alistairjevans]

I've taken a little bit of a deeper dive into this; TLDR is that it is indeed a callback ordering problem, but there's some additional complexity to go with it.

When the deferred callback for the IfNotRegistered runs, it initialises the ServiceRegistrationInfo for IService, and constructs the service's resolve pipeline, which will be empty/default.

When the deferred callback to add the IServiceMiddlewareSource for the decorator fires afterwards the pipeline for the IService has already been built (when it was initialised by the IsNotRegistered condition), so the new source will not be considered.

Two options as I see them:

1. Separate out the set of build callbacks so that 'sources' callbacks get run before anything else.
2. Consider all ServiceRegistrationInfo structures initialised before all the build callbacks have finished running to be 'ephemeral', in that they will be re-initialised the first time they are actually used after all callbacks have run is registered.

Not sure which option I prefer right now. It's possible that the 2nd option would generally clear up a lot of registration ordering issues.

[tillig]

I'm trying to wrap my head around the options and I _think_ we may still end up with an ordering problem with the second option.

Let's say we allow build callbacks to run and then go through and re-initialize everything in the registry at the end. What order do the build callbacks run in? Probably the order in which they were registered, right? Which means we'd still have some issues around whether the "if not registered" callback runs before or after an "only if" callback that maybe runs before or after the decorator callback... and in the end there are still traps to fall into.

Perhaps I'm misunderstanding?

I almost think there's a need for a build pipeline just like there is for a resolve pipeline. Or maybe events you can subscribe to so a callback runs at a certain known, ordered point in the registration process, so you could be sure to run the decorator stuff at the end of the build chain, but before IStartable, and after IfNotRegistered. Or something. I feel like there's got to be some sort of explicit ordering. And that ordering is potentially different than the order in which the actual ContainerBuilder.Register calls were executed.

But that's just my gut feel. I haven't traced out the code as yet to figure out all the things in play here.

[alistairjevans]

We generally have a problem with Dynamic Sources and Service Pipelines where one of the build callbacks evaluates the registry. Here's another (admittedly somewhat contrived) example:

[Fact]
public void IfNotRegistered_CanConditionGenericService()
{
    var builder = new ContainerBuilder();
    builder.RegisterGeneric(typeof(SimpleGeneric<>)).IfNotRegistered(typeof(SimpleGeneric<int>));
    var container = builder.Build();
    // Throws ComponentNotRegistered.
    var result = container.Resolve<SimpleGeneric<int>>();
    Assert.IsType<SimpleGeneric<int>>(result);
}

I don't see how an ordering solution is going to fix that particular problem, because it's the same build callback!

Thinking about the 'reset' approach, I think that any lookups of service info during container build are just not retained. That would solve the decorator problem and the generic one, because as soon as the IfNotRegistered predicate returns, the registry is in a state to accept a new generic source.

I'll try to carve out some time to see how this might work.