Koin - Crashes when activity opened multiple times

ataulm commented 5 years ago

2019-07-06 19:56:30.740 8477-8477/com.muvi E/AndroidRuntime: FATAL EXCEPTION: main
    Process: com.muvi, PID: 8477
    java.lang.RuntimeException: Unable to start activity ComponentInfo{com.muvi/com.muvi.film_detail.FilmDetailActivity}: org.koin.core.error.DefinitionOverrideException: Already existing definition or try to override an existing one: [type:Factory,primary_type:'com.muvi.film_detail.FilmDetailViewModel']
        at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2805)
        at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2883)
        at android.app.ActivityThread.handleRelaunchActivity(ActivityThread.java:4726)
        at android.app.ActivityThread.-wrap18(Unknown Source:0)
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1619)
        at android.os.Handler.dispatchMessage(Handler.java:106)
        at android.os.Looper.loop(Looper.java:164)
        at android.app.ActivityThread.main(ActivityThread.java:6523)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:438)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:857)
     Caused by: org.koin.core.error.DefinitionOverrideException: Already existing definition or try to override an existing one: [type:Factory,primary_type:'com.muvi.film_detail.FilmDetailViewModel']
        at org.koin.core.registry.BeanRegistry.addDefinition(BeanRegistry.kt:144)
        at org.koin.core.registry.BeanRegistry.saveDefinition(BeanRegistry.kt:101)
        at org.koin.core.registry.BeanRegistry.saveDefinitions(BeanRegistry.kt:71)
        at org.koin.core.registry.BeanRegistry.loadModules(BeanRegistry.kt:49)
        at org.koin.core.KoinApplication.loadModulesAndScopes(KoinApplication.kt:66)
        at org.koin.core.KoinApplication.modules(KoinApplication.kt:60)
        at org.koin.core.KoinApplication.modules(KoinApplication.kt:44)
        at org.koin.core.context.GlobalContextKt.loadKoinModules(GlobalContext.kt:86)
        at com.muvi.film_detail.FilmDetailActivity.onCreate(FilmDetailActivity.kt:35)
        at android.app.Activity.performCreate(Activity.java:7013)
        at android.app.Activity.performCreate(Activity.java:7004)
        at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1214)
        at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2758)
        at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2883) 
        at android.app.ActivityThread.handleRelaunchActivity(ActivityThread.java:4726) 
        at android.app.ActivityThread.-wrap18(Unknown Source:0) 
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1619) 
        at android.os.Handler.dispatchMessage(Handler.java:106) 
        at android.os.Looper.loop(Looper.java:164) 
        at android.app.ActivityThread.main(ActivityThread.java:6523) 
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:438) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:857)

ataulm commented 5 years ago

It crashes if you try to go into the same activity multiple times. E.g. open a film, then press back and open a film.

Similar issue if you rotate the screen.

org.koin.core.error.DefinitionOverrideException:
Already existing definition or try to override an existing one:
[type:Factory,primary_type:'com.muvi.film_detail.FilmDetailViewModel']

ataulm commented 5 years ago

Had a quick look. It's because we're loading modules with the same types defined without unloading them in between:

Feed --click film--> FilmDetail (1) --press back--> Feed --click another film--> FilmDetail (2)

Even though FilmDetail (2) is a new Activity, we're calling loadKoinModules(feedModule) again, which loads definitions into Koin that already exist, e.g. the FilmDetailViewModel factory bean.

There's a few way I can see to solve this but I'm not sure which is correct :grimacing:

1. Unload modules in onDestroy

We load the modules in onCreate. So we can unload them in onDestroy. This is what I did in the PR that I'm about to open, but it's not a good solution:

FilmDetail (1) --click actor--> ActorDetail --click another film--> FilmDetail (2)

We'll still get the crash, because we never called onDestroy for FilmDetail (1).

2. Reload: unload before every load

So this one should work, but I'm unsure what the cost of loading and unloading modules are :sweat_smile:

fun reloadKoinModules(vararg modules: Module) {
    unloadKoinModules(*modules)
    loadKoinModules(*modules)
}

The benefit of this one is that we can define a public function for our project and it's a single line swap whenever we load modules:

- loadKoinModules(feedModule)
+ reloadKoinModules(feedModule)

Where would this live? We'd probably have to introduce that core module that things depend on for utils :grimacing:

3. Lazy loadKoinModules

Mario takes this approach here.

Each Activity/Fragment where we load the module would instead call a top-level function in the file.

private val loadFeature by lazy { loadKoinModules(feedModule) }
private fun injectFeature() = loadFeature

internal class FeedFragment : Fragment() {

    override fun onCreate(savedInstanceState: Bundle?) {
        ...
        injectFeature() // instead of `loadKoinModules(feedModule)`
    }

This one has more boilerplate per injection site.

Modules never get unloaded. Does this mean that we're keeping a reference to everything that we inject? :thinking: Or is it just that we're keeping how to build these things (the definitions, factories etc) which is less of an issue.

I think Koin ought to handle this for us :thinking: Or failing that, allow us to check if a module or definition is loaded.

ataulm commented 5 years ago

ah we can also set a module-wide option (or on individual factories) to allow overriding definitions.

I set this instead; I think this should only be an issue if we define two or more types with different implementations (or behaviour) which I don't think will ever be the case :thinking:

ataulm / muvi