JasperFx / lamar

Fast Inversion of Control Tool and Successor to StructureMap
https://jasperfx.github.io/lamar
MIT License
563 stars 118 forks source link

Nested containers creating new nested containers #378

Closed RickBlouch closed 10 months ago

RickBlouch commented 1 year ago

The problem is that when there are more than 2 nested containers in a call flow, there are multiple different instances of a Singleton. The following test demonstrates the scenario. I spent some time on a fix for the issue and came up with the following commit, which also shows the full unit test code.

I'm curious if you have any feedback on the getSingletonScope method and the proposed change in general. Happy to clean up the logic and submit a PR if you provide direction. This change also resolves Setter injection using the correct scope, which had the same issue as constructor injection before it was switched to use the holdingScope.

Having 3 nested containers in a call flow is probably indicative of a bad design and I could see not supporting it in general. As part of identifying this issue in my app I learned that the Azure WebJobs SDK is creating the first scoped container internally so I don't need to and have resolved my troubles by removing it, but wanted to attempt a fix for this so it doesn't bite someone else if possible.

[Fact]
public void SingletonScopeIsIncorrect_WhenNestedContainersAre3LevelsDeep()
{
    var container = new Container(c =>
    {
        c.AddSingleton<SingletonType>();

        c.AddScoped<ScopedA>(); // Depends on SingletonType
        c.AddScoped<ScopedB>(); // Depends on ScopedA
    });

    var singleton = container.GetInstance<SingletonType>();

    using var nested1 = container.GetNestedContainer();
    using var nested2 = nested1.GetInstance<IContainer>().GetNestedContainer();
    using var nested3 = nested2.GetInstance<IContainer>().GetNestedContainer();

    var scopedB = nested3.GetInstance<ScopedB>();
    singleton.ShouldBeTheSameAs(scopedB.A.SingletonType);
}

This issue is similar to #357 (which was resolved by this commit) and has to do with Singleton resolution / scoping in general.

jeremydmiller commented 10 months ago

@RickBlouch Finally looking at this.

jeremydmiller commented 10 months ago

The test by itself was enough. Couple line code change after that, no worries. Thanks for doing this, and sorry for the delay.