modesttree / Zenject

Dependency Injection Framework for Unity3D
MIT License
2.55k stars 273 forks source link

FromSubContainerResolve().ByMethod().WithKernel() not working #160

Open AntonPetrov83 opened 4 years ago

AntonPetrov83 commented 4 years ago

This simple example does not work as expected. While Bar's constructor is called the Tick() method is never called.

using UnityEngine;
using Zenject;

public class SceneInstaller : MonoInstaller
{
    public override void InstallBindings()
    {
        Container.Bind<Foo>().FromSubContainerResolve().ByMethod(InstallFooFacade).WithKernel().AsCached().NonLazy();
    }

    private void InstallFooFacade(DiContainer subContainer)
    {
        subContainer.Bind<Foo>().AsCached();
        subContainer.BindInterfacesAndSelfTo<Bar>().AsCached();
    }
}

public class Foo
{
    [Inject] private Bar _bar;

    public Bar GetBar()
    {
        return _bar;
    }
}

public class Bar : ITickable
{
    public Bar()
    {
        Debug.Log("Bar constructed.");    
    }

    public void Tick()
    {
        Debug.Log("Bar ticked!");
    }
}

UPDATE: For now I can see that SubContainerCreatorUtil binds Kernel interfaces during Resolve-phase when InitializableManager already exists. That is why InitializableManager knows nothing about Kernel.

UPDATE 2: Changing code to this fixes the issue:

    public override void InstallBindings()
    {
        Container.Bind<Foo>().FromSubContainerResolve().ByMethod(InstallFooFacade).WithKernel().AsCached().NonLazy();
        Container.Resolve<Foo>(); // !!!!!!!!!!!!!!!!!!!!!
    }

UPDATE 3: This test fails because of early Resolve<InitializableManager>(); like the real application.

        [Test]
        public void TestByMethodEarlyResolveInitializableManager()
        {
            Container.Bind<FooFacade>().FromSubContainerResolve()
                .ByMethod(InstallFoo).WithKernel().AsSingle();

            ZenjectManagersInstaller.Install(Container);
            Container.ResolveRoots();

            // resolve InitializableManager before FooFacade.
            Container.Resolve<InitializableManager>();

            var facade = Container.Resolve<FooFacade>();

            Assert.That(!facade.Foo.WasInitialized);
            Container.Resolve<InitializableManager>().Initialize();
            Assert.That(facade.Foo.WasInitialized);
        }

UPDATE 4: Duplicate and related issues: https://github.com/modesttree/Zenject/issues/626 https://github.com/modesttree/Zenject/issues/574 https://github.com/svermeulen/Extenject/issues/13

UPDATE 5: The method of inheriting the Kernel described here works. https://github.com/svermeulen/Extenject/blob/master/Documentation/SubContainers.md#using-byinstaller--bymethod-with-kernel

So the interesting part is the difference between a manually bound Kernel-based class and the automatic Kernel setup in SubContainerCreatorUtil.

altunsercan commented 4 years ago

I have investigated the issue a bit and can confirm that the feature is bugged. Here is a summary:

Workaround: Making your Facade class (Foo) IInitializable and NonLazy at the same time seems to fix the issue.

Proper Fix: At first I though adding subcontainer kernel to parent kernel before Foo is initialized may be the solution. However that means that Kernel would start running regardless if Subcontainer is created lazy or not. This would particularly be problem for transient subcontainers. Often used with factories or pools.

So I think the proper fix is to immediately bind a SubcontainerKernelProxy to parent context if none exists. This would be a proxy kernel that reserves a spot on InitializationManager, TickableManager etc. But won't do anything until actual kernel is created. When the subcontainer actually initializes, subcontainer kernel would hook to proxy kernel to receive callbacks.

AntonPetrov83 commented 4 years ago

I think another possible solution is to use InitializableManager.Add() (and similar methods for other managers) on a parent container later when a sub-container is instantiated.

sbergen commented 4 years ago

Proper Fix: At first I though adding subcontainer kernel to parent kernel before Foo is initialized may be the solution. However that means that Kernel would start running regardless if Subcontainer is created lazy or not. This would particularly be problem for transient subcontainers. Often used with factories or pools.

So I think the proper fix is to immediately bind a SubcontainerKernelProxy to parent context if none exists. This would be a proxy kernel that reserves a spot on InitializationManager, TickableManager etc. But won't do anything until actual kernel is created. When the subcontainer actually initializes, subcontainer kernel would hook to proxy kernel to receive callbacks.

If I understood this correctly, this is the workaround I ended up implementing also. We have this concept of "persistent object installers", which can be used to create objects in a subcontainer that live across scene loads. We bind an PersistentObjectContext at the ProjectContext level (this is essentially what I believe you were referring to as SubcontainerKernelProxy), and then bind a Kernel and one of these in each subcontainer:

private class KernelRegisterer
{
    public KernelRegisterer(PersistentObjectContext context, Kernel kernel)
    {
        context.DisposableManager.Add(kernel);
        context.DisposableManager.AddLate(kernel);

        context.TickableManager.Add(kernel);
        context.TickableManager.AddLate(kernel);
        context.TickableManager.AddFixed(kernel);

        kernel.Initialize();
    }
}

For transient objects, you should implement removal here (as IDisposable) also, but in our use case, these live as long as the PersistentObjectContext, so it's not a problem to never unregister.

Eifet commented 9 months ago

Few years later, it's still a problem. Yet another workaround that I found, if someone is interested, is actually from examples. You just need to make your facade class to extend Kernel

public class Greeter : Kernel
{
    public Greeter()
    {
        Debug.Log("Created Greeter");
    }
}

And then you bind self and interfaces to

    public override void InstallBindings()
    {
        Container.BindAllInterfacesAndSelf<Greeter>()
            .To<Greeter>().FromSubContainerResolve().ByMethod(InstallGreeter).AsSingle().NonLazy();
    }