unitycontainer / microsoft-dependency-injection

Unity.Microsoft.DependencyInjection package
Apache License 2.0
63 stars 36 forks source link

Generic Types are resolved to the same across scopes #14

Closed pksorensen closed 6 years ago

pksorensen commented 6 years ago

Given the following test

    public interface ITest2
    {

    }
    public class TestOptions
    {

    }
   public interface ITest1
    {

    }
    public class Test1 : ITest1, ITest2
    {

        public Test1(IOptions<TestOptions> options)
        { 

        }

    }
    public interface IMygenericTest<T>
    {

    }

    public class MyGenericTest<T> : IMygenericTest<T>
    {

    }
        [TestMethod]
        public void Test9()
        {

            var c = new UnityContainer();

            var s1 = new ServiceCollection().AddOptions();
            var s2 = new ServiceCollection().AddOptions();

            s1.AddSingleton<ITest1, Test1>();
            s2.AddSingleton<ITest1, Test1>();
            s1.AddSingleton(typeof(IMygenericTest<>), typeof(MyGenericTest<>));
            s2.AddSingleton(typeof(IMygenericTest<>), typeof(MyGenericTest<>));

            var sp1 = s1.BuildServiceProvider(c.CreateChildContainer());

            var t1 = sp1.GetRequiredService<ITest1>();

            var sp2 = s2.BuildServiceProvider(c.CreateChildContainer());
            var t2 = sp2.GetRequiredService<ITest1>();

            //Works for above, but not below

            Assert.AreNotSame(t2, t1);
            Assert.AreNotSame(t2.GetHashCode(), t1.GetHashCode());

            var h1 = sp1.GetRequiredService<IMygenericTest<int>>();
            var h2 = sp2.GetRequiredService<IMygenericTest<int>>();
            Assert.IsNotNull(h1, "notnull");
            Assert.AreNotSame(h1, h2, "sameobj");

        }

the test fails with "sameobj" indicating that h1 and h2 are the same.

I would expect that to be two different objects as its being build from two different child containers.

I added the same for normal classes which works fine. See the ITest1 resolution above.

I am looking into how generics are resolved

pksorensen commented 6 years ago

Here is a just unity example of same

    [TestMethod]
    public void Test10()
    {

        var c = new UnityContainer();

        var c1 = c.CreateChildContainer();
        var c2 = c.CreateChildContainer();

        c1.RegisterType(typeof(IMygenericTest<>), typeof(MyGenericTest<>),new ContainerControlledLifetimeManager());

        var t1 = c1.Resolve<IMygenericTest<int>>();
        Assert.IsNotNull(t1);

        c2.RegisterType(typeof(IMygenericTest<>), typeof(MyGenericTest<>), new ContainerControlledLifetimeManager());

        var t2 = c2.Resolve<IMygenericTest<int>>();
        Assert.IsNotNull(t2);

        Assert.AreNotSame(t2, t1);

    }

What should we expect here.

If this is expected, then how do i make this work in aspnet core when they have signleton registrations.

My app is hosting two services in the same process and having a shared root container, but different service providers.

I noticed this behavior because one api started resolving services from the other api

pksorensen commented 6 years ago

[TestClass] public class OpenGenericFixture {

    public interface IMygenericTest<T>
    {

    }

    public class MyGenericTest<T> : IMygenericTest<T>
    {

    }

    [TestMethod]
    public void OpenGenericTypesShouldNotBeSame()
    {

        var c = new UnityContainer();

        var c1 = c.CreateChildContainer();
        var c2 = c.CreateChildContainer();

        c1.RegisterType(typeof(IMygenericTest<>), typeof(MyGenericTest<>), new ContainerControlledLifetimeManager());

        var t1 = c1.Resolve<IMygenericTest<int>>();
        Assert.IsNotNull(t1);

     //   c2.RegisterType(typeof(IMygenericTest<>), typeof(MyGenericTest<>), new ContainerControlledLifetimeManager());

        var t2 = c2.Resolve<IMygenericTest<int>>();
        Assert.IsNotNull(t2);

        Assert.AreNotSame(t2, t1);

    }
}

in this example i would expect var t2 = c2.Resolve<IMygenericTest<int>>(); to throw not regiatered exception

pksorensen commented 6 years ago

So heres the results:

I found that https://github.com/unitycontainer/container/blob/v5.x/src/UnityContainer.Resolution.cs#L64 when using GetRegistration is returning the same InternalRegistration for both calls to Resolve in the unit test above.

c1.RegisterType(typeof(IMygenericTest<>), typeof(MyGenericTest<>), new ContainerControlledLifetimeManager());
            c2.RegisterType(typeof(IMygenericTest<>), typeof(MyGenericTest<>) ,new ContainerControlledLifetimeManager());

            var t1 = c1.Resolve<IMygenericTest<int>>();
            Assert.IsNotNull(t1);
            var t2 = c2.Resolve<IMygenericTest<int>>();
            Assert.IsNotNull(t2);

This then makes https://github.com/unitycontainer/container/blob/v5.x/src/Strategies/LifetimeStrategy.cs#L32 return the same ContainerControlledLifetimeManager at the second resolution instead of a new one and the value is reused instead of a new.

pksorensen commented 6 years ago

So, for child containers it looks like it uses the parent containers registrations.

Would the fix be to have child containers contain their own registrations and only resolve to parent registrations when it attempted its own registrations. I dont see other ways to fix this.

update

Looks like this is how its implemented currently.

Is this then what happens: When the first resolution happens, then it first ask child 1 for the registration which do not exist, then it asks the parent which dont exists but using GetOrAdd its now added to the parent. Then the second resolution will try against child 2 and then the parent which now has it and it resolves this.

pksorensen commented 6 years ago

this fixes the issue

 private void SetupChildContainerBehaviors()
        {
            _registrations = new HashRegistry<Type, IRegistry<string, IPolicySet>>(ContainerInitialCapacity);
            IsTypeRegistered = IsTypeRegisteredLocally;
            GetRegistration = GetOrGetParentOrAddCurrent;
            Register = AddOrUpdate;
            GetPolicy = Get;
            SetPolicy = Set;
            ClearPolicy = Clear;
        }

        private IPolicySet GetOrGetParentOrAddCurrent(Type type, string name)
        {
            var a = Get(type, name);
            if(a == null)
            {
                a = _parent.Get(type, name);
            }
            if (a == null)
                a = GetOrAdd(type, name);

            return a;
        }

but broke

        [TestMethod]
        public void ChainOfContainers()
        {
pksorensen commented 6 years ago

@ENikS Let me know if I am on track and if it should be moved to the Container repository.

pksorensen commented 6 years ago

I would argue that it makes sense that the behavior GetOrAdd on parent when not existing is wrong - but since it is a breaking change. How can we make this a opt in change and get it fixed now instead of a larger version bumb?

pksorensen commented 6 years ago

this sovled it. all tests passes including my new one.

  private void SetupChildContainerBehaviors()
        {
            _registrations = new HashRegistry<Type, IRegistry<string, IPolicySet>>(ContainerInitialCapacity);
            IsTypeRegistered = IsTypeRegisteredLocally;
            GetRegistration = GetOrGetParentOrAddCurrent;
            Register = AddOrUpdate;
            GetPolicy = Get;
            SetPolicy = Set;
            ClearPolicy = Clear;
        }

        private IPolicySet GetOrGetParentOrAddCurrent(Type type, string name)
        {
            var a = Get(type, name);
            if(a == null)
            {
                var parent = _parent;

                while (a == null && parent != null)
                {
                    a = parent.Get(type, name);
                    parent = parent._parent;
                }
            }
            if (a == null)
                a = GetOrAdd(type, name);

            return a;
        }