toothpick icon indicating copy to clipboard operation
toothpick copied to clipboard

The problem with cyclic injects checker

Open morder opened this issue 3 years ago • 7 comments

Hello! Unfortunately, I got an unexpected crash with toothpick.

How to reproduce the crash

val parentScope = KTP.openScope("App")
val childScope = KTP.openScopes("App", "Child")

childScope.installModules(module {
   bind(...) // here is I bind some isolated(internal) instances that I need only in "Child" scope
   bind(IFace::class.java).to(Face::class.java).singleton() // also I bind some Interface to provide outside (like dagger's component)
})

// I want to provide some interface from my child scope to parent scope
parentScope.installModules(module {
    bind(IFace::class.java).toProviderInstance { childScope.getInstance(IFace::class.java) }.providesSingleton()
})

this code throws

Class xxx.xxx.IFace creates a cycle:

                              ===============================
                             ||                             ||
                             \/                             ||
   xxx.xxx.IFace  ||
                             ||                             ||
                              ===============================

this code is working, but it creates instance immediately

     bind(IFace::class.java).toInstance(childScope.getInstance(IFace::class.java)).singleton()

If I remove any cycles checking from toothpick, everything is working as I expected

the problem with this code from RuntimeCheckOnConfiguration.java

final LinkedHashSet<Pair> linkedHashSet = cycleDetectionStack.get();
if (linkedHashSet.contains(pair)) { // pair = IFace|null
  throw new CyclicDependencyException(Pair.getClassList(linkedHashSet), clazz);
}

we are trying to get IFace instance from child scope, but we already trying to get it from parent scope cycles checking code can't understand it :)

morder avatar Sep 26 '20 07:09 morder

Hi, this usecase is a bit strange to me, could you please explain the real usecase ? What do you need to achieve ?Why not to just inject Iface in the parentscope as well as in the child scope ? If the binding is done in the parent, the child scope will benefit from this binding if not overriden.Additionally, a the scope structure is a tree, it allow by design to have second child scope. How would you solve this ?Everyscope have to be sufficient, so a rootscope has to live/survive without any child scope from my point of view (at least)

afaucogney avatar Sep 26 '20 09:09 afaucogney

@afaucogney Hi. I will try to explain my case

  1. I have a gradle module that I want to share between my apps (for example AdsModule)
  2. AdsModule should provide IAdsManager to all apps
  3. AdsModule has some dependencies, for example Context, Application, ISomeData, and so on.
  4. AdsModule contains its own toothpick's module with inner bindings.
  5. Each app should get IAdsManager from AdsModule and insert it into toothpick's App Scope.

My solution is to create the function

internal class AdsModule : Module() {
    init {
        bind(...) // internal
        bind(...) // internal
        bind(...) // internal
        .... // internal
        bind(IAdsManager::class.java).to(AdsManager::class.java).singleton()
    }
)

fun installAdsScope(parentScope: Scope) {
    val scope = KTP.openScopes(parentScope.name, "AdsScope")
    scope.installModules(AdsModule())

    parentScope.installModules(module {
        bind(IAdsManager::class.java).toProviderInstance { scope.getInstance(IAdsManager::class.java) }.providesSingleton()
    })
}

this code is similar to dagger's Component

@Singleton
@Component(
    modules = [
        AdsModule::class
    ], dependencies = [
        Dependencies::class // dependencies from parent's component
    ]
)
interface AdsComponent : IAdsManager {
    @Component.Factory
    interface Builder {
        fun build(dependencies: Dependencies): AdsComponent // provides IAdsManager
    }
}

The code from my App

val scope = KTP.openScope("AppScope")
scope.installModules(...)
installAdsScope(scope)

I don't want to bind everything from AdsModule to App Scope, I just want to install exactly one interface IAdsManager

morder avatar Sep 28 '20 12:09 morder

Your usecase is fine to me, but the following is not clear to me:

parentScope.installModules(module {
    bind(IAdsManager::class.java).toProviderInstance { scope.getInstance(IAdsManager::class.java) }.providesSingleton()
})

To Inject the IAdsManager, in your app, you have to definitively bind it in the root scope. Then there are 2 options:

  • Do the job in app
  • Do the job in module, but triggered in a way by the app.

By doing the job in the module, you can hide the complexity on the manager instantiation, but nevertheless you have to init the sdk somewhere. AdsManager.setup() ou AdsManager.inject()...

You may bind the IAdsManager to a provider or directly to the instance, as you want.

Then IAdsManager should be available at rootscope level everywhere in the app. (do not forget to call inject()).

PS: To remind what I have tried to explain early, a top scope of a tree, cannot/must not depend from one of its child, because by design this top scope may have other children or no child at all, and it should be kept persistent.

I hope I helped a bit.

afaucogney avatar Sep 28 '20 13:09 afaucogney

parentScope.installModules(module {
    bind(IAdsManager::class.java).toProviderInstance { scope.getInstance(IAdsManager::class.java) }.providesSingleton()
})

Here is I want to bind an interface without creating of instance right now. Because it's possible that IAdsManager will never instantiate because we don't open some screen. For example, I open some screen (ViewModel), with injected IAdsManager, and then I call IAdsManager.init() to do network request. I want to do the job(create an instance of a manager) when it needed, for example when ViewModel is instantiated.

IAdsManager - it's just for example. Overall I'm talking about an approach to solve this problem.

This code is solving the problem, but it creates the instance right in the app. I don't like it.

parentScope.installModules(module {
    bind(IAdsManager::class.java).toProvider(scope.getInstance(IAdsManager::class.java)).singleton()
})

PS: To remind what I have tried to explain early, a top scope of a tree, cannot/must not depend from one of its child, because by design this top scope may have other children or no child at all, and it should be kept persistent.

I agree, and I think the toothpick is not designed to solve my problem clearly.

morder avatar Sep 29 '20 06:09 morder

I think this is not the design concept of ktp that block you in finding solutions. My point was more about general concept.

I think there are 2 possible solutions to your problems :

1/ use a lazy injection.

  • with the lazy keyword of ktp
  • or with an intermediate object that will be use to init and provide the instance when needed.

2/ bind your manager in the viewmodel of the dedicated screen. (Not before)

afaucogney avatar Sep 29 '20 07:09 afaucogney

fix https://github.com/stephanenicolas/toothpick/pull/428

morder avatar Jun 17 '21 13:06 morder

Really unfortunate that this is still an issue :( In my case, I needed to added same bind in submodule onto the same interface for ImageLoader to provide automatic analytics logger for images. IMHO it's pretty reasonable scenario

Nexen23 avatar Nov 08 '23 21:11 Nexen23