RegisterDecorator corrupts resolution when a target registration has greater lifetime than per-dependency and IEnumerable<> is resolved

[VonOgre]

Reproduced in Autofac trunk DecoratorTests, but also noted in Autofac 5.1.2.

If one or more services (e.g. IService) are registered with a specific lifetime (InstancePerLifetimeScope, SingleInstance), resolving IEnumerable will return instances of all of the types as normal, but adding a decorator into the mix breaks the IEnumerable resolution, replacing all instances with the first "reusable" instance. Similarly, once IEnumerable is resolved, trying to resolve IService no longer returns the last registration, but instead also returns the "reusable" decorated instance noted in the IEnumerable case

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

            var services = container.Resolve<IEnumerable<IDecoratedService>>();

            Assert.Collection(
                services,
                s =>
                {
                    Assert.IsType<DecoratorA>(s);
                    Assert.IsType<ImplementorA>(s.Decorated);
                },
                s =>
                {
                    Assert.IsType<DecoratorA>(s);
                    Assert.IsType<ImplementorB>(s.Decorated);
                });

            services = container.Resolve<IEnumerable<IDecoratedService>>();

            Assert.Collection(
                services,
                s =>
                {
                    Assert.IsType<DecoratorA>(s);
                    //Fails because s.Decorated is of type ImplementorB
                    Assert.IsType<ImplementorA>(s.Decorated);
                },
                s =>
                {
                    Assert.IsType<DecoratorA>(s);
                    Assert.IsType<ImplementorB>(s.Decorated);
                });
        }

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

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

            Assert.IsType<DecoratorA>(service);
            //Fails because s.Decorated is of type ImplementorA
            Assert.IsType<ImplementorB>(service.Decorated);
        }

[VonOgre]

And I think I have found the offending block of code for it in InstanceLookup.cs:

var sharing = _decoratorTargetComponent?.Sharing ?? ComponentRegistration.Sharing;

var resolveParameters = Parameters as Parameter[] ?? Parameters.ToArray();

if (!_activationScope.TryGetSharedInstance(ComponentRegistration.Id, out var instance))
{
	instance = sharing == InstanceSharing.Shared
		? _activationScope.CreateSharedInstance(ComponentRegistration.Id, () => CreateInstance(Parameters))
		: CreateInstance(Parameters);
}

In the scenario of the decorators, a given decorator instance is "promoted" into the shared instance collection if its targeted component is shared. However, since there is only one registration for the decorator (and subsequently only one ID), only a single instance of the decorator can ever be shared and will now be spread across all possible targets.

The instance being stored for sharing ideally should be stored with a composite ID of the decorator registration AND the target registration; however, since the ISharingLifetimeScope seems to be what I'd consider the "public API" and is taking GUIDs for IDs, we can't just concatenate strings or change the method signature at will... One possible way to keep things working is to leverage RFC 4122 §4.3, which basically describes how to generate a predicable GUID out of a string, with an optional namespace GUID being applied as a sort of salt to still avoid collisions. In the event a decorator is being promoted to being shared, a new GUID, based on the decorator's registration ID and namespaced by the target's registration ID, is generated and used for storing the shared instance. That way, each time a given decorator+target combo is encountered, the GUID will be predictable, but not get in the way of a different instance ALSO being promoted because of its target. Thoughts?

UPDATE:
I was able to confirm using the name-based UUID generation logic does fix the issue and (utility class aside) the change would look something like this in the same area of InstanceLookup.cs:

var sharing = _decoratorTargetComponent?.Sharing ?? ComponentRegistration.Sharing;
var sharedInstanceId = ComponentRegistration.Id;
if (_decoratorTargetComponent != null)
{
	sharedInstanceId = DeterministicGuid.Create(_decoratorTargetComponent.Id, sharedInstanceId.ToByteArray());
}

var resolveParameters = Parameters as Parameter[] ?? Parameters.ToArray();

if (!_activationScope.TryGetSharedInstance(sharedInstanceId, out var instance))
{
	instance = sharing == InstanceSharing.Shared
		? _activationScope.CreateSharedInstance(sharedInstanceId, () => CreateInstance(Parameters))
		: CreateInstance(Parameters);
}

[alistairjevans]

Hmm, good find! Looks like this is some emergent behaviour from the fixes for #963. Up until that point, decorators did not inherit the sharing scope from their wrapped component.

As for approach, the creation of your custom Guid appears fairly expensive, but we don't have benchmarks, so I don't know for sure (see my comments on the PR). We need to keep the amount of work done in InstanceLookup to a minimum, because it is a very hot path.

It might be a breaking change, but it feels like we should add overloads to the ISharingLifetimeScope interface for TryGetSharedInstance and CreateSharedInstance that take a secondary guid. Perhaps use a ValueTuple combining two guids in the sharing dictionary. I'd still want to know the performance impact, but there should be no/minimal additional allocations with that approach.

As an aside, this is one of a collection of issues we would hope to resolve by switching to the pipelines approach under discussion in #1096.

[VonOgre]

A very fair concern regarding the custom guid, to be sure! I'd limited its use to only when looking up a decorator, but could still be an impact along that path. I'll do some investigation on the benchmarks and expand on them if needed (there are some decorator benchmarks already available) to see what the impact would be, with either approach.

[alistairjevans]

Thanks @VonOgre. 👍

[VonOgre]

@alistairjevans - your instinct seems to have been spot on, since there was a noteworthy jump in timings on the existing benchmarks. I've been fiddling with BenchmarkDotNet to expand the coverage a bit to cover repeat resolutions, but it was slow going since I'd never used that framework before. I'll dig into the overload approach and see what comes out of it in the next couple of days.

[VonOgre]

Once #1116 is refined and approved, I'll have a fast follower with a fix for this issue, but I wanted to have the benchmarks evaluated on their own first