Zenject icon indicating copy to clipboard operation
Zenject copied to clipboard

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

Open AntonPetrov83 opened this issue 4 years ago • 4 comments

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.

AntonPetrov83 avatar Jul 24 '20 16:07 AntonPetrov83

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

  • WithKernel works as Binding a new Kernel to subcontainer and binding the interfaces of that Kernel to parent subcontainer. In theory when the parent container starts InitializableManager will pick up IInitializable binding of the subcontainer kernel and resolve it. Causing the internal kernel to start.

  • However this WithKernel registration happens during subcontainer creation. Which only happens when Foo is resolved. When Foo is not IInitializable even if it is NonLazy there is a chance that the InitializableManager resolves before Foo. If this happens the List<IInitializable> will resolve before subcontainer kernel is ever added to the bindings. So it won't initialize.

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.

altunsercan avatar Jul 25 '20 15:07 altunsercan

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.

AntonPetrov83 avatar Jul 25 '20 16:07 AntonPetrov83

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.

sbergen avatar Oct 26 '20 14:10 sbergen

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();
    }

Eifet avatar Feb 01 '24 12:02 Eifet