Parameter resolution is not threadsafe

[gerbiljames]

Describe the bug
When parameters are supplied (passed in through a ParametersHolder) to Koin they get added to _parameterStack on the Scope, this works fine if only one object is being resolved at a time. If more than one object is being resolved simultaneously on different threads then this resolution breaks down as there is no way to ensure that for each object that the correct ParametersHolder is at the top of the stack. Additionally, if any object fails resolution (using getOrNull()) then the entire stack is cleared, this can cause required parameters to be lost.

I've seen this in production with Android Workers, the stack traces look like:

No definition found for class:'androidx.work.WorkerParameters' q:''. Check your definitions!
at org.koin.core.scope.Scope.throwDefinitionNotFound(Scope.kt:298)
at org.koin.core.scope.Scope.resolveValue(Scope.kt:268)
at org.koin.core.scope.Scope.resolveInstance(Scope.kt:231)
at org.koin.core.scope.Scope.get(Scope.kt:210)
...internal DSL stuff...
at org.koin.core.instance.InstanceFactory.create(InstanceFactory.kt:52)
at org.koin.core.instance.FactoryInstanceFactory.get(FactoryInstanceFactory.kt:38)
at org.koin.core.registry.InstanceRegistry.resolveInstance$koin_core(InstanceRegistry.java:116)
at org.koin.core.scope.Scope.resolveValue(Scope.kt:246)
at org.koin.core.scope.Scope.resolveInstance(Scope.kt:231)
at org.koin.core.scope.Scope.get(Scope.kt:210)
at org.koin.core.scope.Scope.getOrNull(Scope.kt:175)
at org.koin.androidx.workmanager.factory.KoinWorkerFactory.createWorker(KoinWorkerFactory.kt:47)
at androidx.work.DelegatingWorkerFactory.createWorker(DelegatingWorkerFactory.java:71)
at androidx.work.WorkerFactory.createWorkerWithDefaultFallback(WorkerFactory.java:83)
at androidx.work.impl.WorkerWrapper.runWorker(WorkerWrapper.java:243)
at androidx.work.impl.WorkerWrapper.run(WorkerWrapper.java:145)
at androidx.work.impl.utils.SerialExecutorImpl$Task.run(SerialExecutorImpl.java:96)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1137)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:637)
at java.lang.Thread.run(Thread.java:1012)

Not all of our workers are initialised using koin, when WorkManager is initialised it attemps to do any work that is pending which causes it to start several workers at once on multiple background threads. If any of these workers are not koin workers then they (correctly) fail resolution and this wipes the parameter stack. Any workers which were still being resolved at this point lose their WorkerParameters and this results in the above stacktrace.

There's also a possibility that koin could supply the wrong parameters as the stack could be modified by another thread during object resolution.

To Reproduce
This isn't easy to reproduce due to the nature of multi-threading but analysis of the Scope code paints a pretty clear picture of whats going on here.

Expected behavior
Parameter resolution is always threadsafe, even when using constructor DSL

Koin project used and used version (please complete the following information):
koin-android version 3.3.3
koin-androidx-workmanager version 3.3.3

Additional moduleDefinition
n/a

[NoZomIBK]

Ran into same / similar issue
When creating instances of the same class with different parameters in different threads the injected parameters might come from a different thread. In my usecase i had jobs hashing files if no hash exists and query them for hash lookup. It resulted in all lookup jobs having the same file as injected parameter instead of the given file as parameter.

While trying to create a minimal test for that issue i got the No definition found Exception instead of the actual problem.
My Test:

    @Test
    fun `test koin parallel`() = runTest {
        val koin = koinApplication {
            modules(module {
                factoryOf(::TestData)
            })
        }.koin

        val testDataCreators = (0..1000).map {
            async(context = Dispatchers.IO) {
                it to koin.get<TestData> { parametersOf(TestDataHolder(it)) }
            }
        }
        val testData = testDataCreators.map { it.await() }

        testData.forEach {
            it.first `should be equal to` it.second.value.value
        }
    }

    data class TestData(val value: TestDataHolder)
    data class TestDataHolder(val value: Int)

[marcellogalhardo]

Genuine question: does the problem only happens when using Constructor DSL or does it happens with the standard Module DSL too? From @gerbiljames stack-trace and @NoZomIBK code sample, it seems to be a problem related to how Koin does dependency resolution when using Injected Parameters on a multi-thread environment, and hence, it is a broader issue and not due to Constructor DSL usage.

[gerbiljames]

You're right @marcellogalhardo, I initially thought that the ParametersDefinition was queried when not using Constructor DSL but this is not the case. This is much broader issue and essentially makes koin broken in multi-threaded environments.

[octa-one]

I also had the same problem, this fix (#1561) should help

[arnaudgiuliani]

let's follow on the PR