Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The problem with cyclic injects checker #420

Open
morder opened this issue Sep 26, 2020 · 7 comments
Open

The problem with cyclic injects checker #420

morder opened this issue Sep 26, 2020 · 7 comments
Assignees

Comments

@morder
Copy link

morder commented Sep 26, 2020

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 :)

@afaucogney
Copy link

afaucogney commented Sep 26, 2020 via email

@morder
Copy link
Author

morder commented Sep 28, 2020

@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

@afaucogney
Copy link

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.

@morder
Copy link
Author

morder commented Sep 29, 2020

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.

@afaucogney
Copy link

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)

@dlemures dlemures self-assigned this Oct 4, 2020
@morder
Copy link
Author

morder commented Jun 17, 2021

fix
#428

@Nexen23
Copy link

Nexen23 commented Nov 8, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants