Merge pull request #1643 from InsertKoinIO/bugfix/parameters-race

Fixing race condition in Scope - Fixed for 3.5.0
InsertKoinIO · Sep 4, 2023 · 88b3223 · 88b3223
2 parents e7cbcf5 + 0a6422c
commit 88b3223
Show file tree

Hide file tree

Showing 8 changed files with 80 additions and 44 deletions.
diff --git a/core/koin-core/build.gradle b/core/koin-core/build.gradle
@@ -75,6 +75,7 @@ kotlin {
             dependsOn commonMain
             dependencies {
                 implementation 'org.jetbrains.kotlin:kotlin-stdlib-js'
+                implementation "co.touchlab:stately-concurrency:1.2.2"
             }
         }
 

diff --git a/core/koin-core/src/commonMain/kotlin/org/koin/core/scope/Scope.kt b/core/koin-core/src/commonMain/kotlin/org/koin/core/scope/Scope.kt
@@ -33,6 +33,7 @@ import org.koin.ext.getFullName
 import org.koin.mp.KoinPlatformTimeTools
 import org.koin.mp.KoinPlatformTools
 import org.koin.mp.Lockable
+import org.koin.mp.ThreadLocal
 import kotlin.reflect.KClass
 
 @Suppress("UNCHECKED_CAST")
@@ -58,7 +59,7 @@ class Scope(
     private val _callbacks = arrayListOf<ScopeCallback>()
 
     @KoinInternalApi
-    val _parameterStack = ArrayDeque<ParametersHolder>()
+    val _parameterStackLocal = ThreadLocal<ArrayDeque<ParametersHolder>>()
 
     private var _closed: Boolean = false
     val logger: Logger get() = _koin.logger
@@ -223,19 +224,17 @@ class Scope(
             throw ClosedScopeException("Scope '$id' is closed")
         }
         val parameters = parameterDef?.invoke()
+        var localDeque: ArrayDeque<ParametersHolder>? = null
         if (parameters != null) {
             _koin.logger.log(Level.DEBUG) { "| >> parameters $parameters " }
-            KoinPlatformTools.synchronized(this@Scope) {
-                _parameterStack.addFirst(parameters)
-            }
+            localDeque = _parameterStackLocal.get() ?: ArrayDeque<ParametersHolder>().also(_parameterStackLocal::set)
+            localDeque.addFirst(parameters)
         }
         val instanceContext = InstanceContext(_koin.logger, this, parameters)
         val value = resolveValue<T>(qualifier, clazz, instanceContext, parameterDef)
-        if (parameters != null) {
+        if (localDeque != null) {
             _koin.logger.debug("| << parameters")
-            KoinPlatformTools.synchronized(this@Scope) {
-                _parameterStack.removeFirstOrNull()
-            }
+            localDeque.removeFirstOrNull()
         }
         return value
     }
@@ -244,41 +243,31 @@ class Scope(
         qualifier: Qualifier?,
         clazz: KClass<*>,
         instanceContext: InstanceContext,
-        parameterDef: ParametersDefinition?,
-    ) = (
-        _koin.instanceRegistry.resolveInstance(qualifier, clazz, this.scopeQualifier, instanceContext)
-            ?: run {
-                // try resolve in injected param
-                _koin.logger.debug("|- ? t:'${clazz.getFullName()}' - q:'$qualifier' look in injected parameters")
-                _parameterStack.firstOrNull()?.getOrNull<T>(clazz)
-            }
-            ?: run {
-                // try resolve in scope source
-                _koin.logger.debug("|- ? t:'${clazz.getFullName()}' - q:'$qualifier' look at scope source")
-                _source?.let { source ->
-                    if (source::class == clazz && qualifier == null) {
-                        _source as? T
-                    } else {
-                        null
-                    }
-                }
-            }
-            ?: run {
-                // try resolve in other scopes
-                _koin.logger.debug("|- ? t:'${clazz.getFullName()}' - q:'$qualifier' look in other scopes")
-                findInOtherScope<T>(clazz, qualifier, parameterDef)
+        parameterDef: ParametersDefinition?
+    ) = (_koin.instanceRegistry.resolveInstance(qualifier, clazz, this.scopeQualifier, instanceContext)
+        ?: run {
+            _koin.logger.debug("|- ? t:'${clazz.getFullName()}' - q:'$qualifier' look in injected parameters")
+            _parameterStackLocal.get()?.firstOrNull()?.getOrNull<T>(clazz)
+        }
+        ?: run {
+            _koin.logger.debug("|- ? t:'${clazz.getFullName()}' - q:'$qualifier' look at scope source" )
+            _source?.let { source ->
+                if (source::class == clazz && qualifier == null) {
+                    _source as? T
+                } else null
             }
-            ?: run {
-                // in case of error
-                if (parameterDef != null) {
-                    KoinPlatformTools.synchronized(this@Scope) {
-                        _parameterStack.removeFirstOrNull()
-                    }
-                    _koin.logger.debug("|- << parameters")
-                }
-                throwDefinitionNotFound(qualifier, clazz)
+        }
+        ?: run {
+            _koin.logger.debug("|- ? t:'${clazz.getFullName()}' - q:'$qualifier' look in other scopes" )
+            findInOtherScope<T>(clazz, qualifier, parameterDef)
+        }
+        ?: run {
+            if (parameterDef != null) {
+                _parameterStackLocal.remove()
+                _koin.logger.debug("|- << parameters")
             }
-        )
+            throwDefinitionNotFound(qualifier, clazz)
+        })
 
     @Suppress("UNCHECKED_CAST")
     private fun <T> getFromSource(clazz: KClass<*>): T? {

diff --git a/core/koin-core/src/commonMain/kotlin/org/koin/mp/KoinPlatformTools.kt b/core/koin-core/src/commonMain/kotlin/org/koin/mp/KoinPlatformTools.kt
@@ -32,3 +32,9 @@ expect object KoinPlatformTools {
 }
 
 expect open class Lockable()
+
+expect open class ThreadLocal<T>() {
+    fun get(): T?
+    fun set(value: T?)
+    fun remove()
+}
diff --git a/core/koin-core/src/commonTest/kotlin/org/koin/core/ParameterStackTest.kt b/core/koin-core/src/commonTest/kotlin/org/koin/core/ParameterStackTest.kt
@@ -28,6 +28,6 @@ class ParameterStackTest {
             koin.get<Simple.MyStringFactory> { parametersOf(KoinPlatformTools.generateId()) }
         }
 
-        assertTrue(koin.scopeRegistry.rootScope._parameterStack.isEmpty())
+        assertTrue(koin.scopeRegistry.rootScope._parameterStackLocal.get()!!.isEmpty())
     }
 }
diff --git a/core/koin-core/src/commonTest/kotlin/org/koin/core/ParametersInjectionTest.kt b/core/koin-core/src/commonTest/kotlin/org/koin/core/ParametersInjectionTest.kt
@@ -74,8 +74,8 @@ class ParametersInjectionTest {
         val koin = app.koin
         val a: Simple.MySingleAndNull = koin.get { parametersOf(42) }
 
-        assertEquals(42, a.ms.id)
-        assertTrue { koin.scopeRegistry.rootScope._parameterStack.isEmpty() }
+        assertEquals(42, a.ms.id, "id is the right injected")
+        assertTrue("parameter stack is empty") { koin.scopeRegistry.rootScope._parameterStackLocal.get()?.isEmpty() == true }
     }
 
     @Test

diff --git a/core/koin-core/src/jsMain/kotlin/org/koin/mp/PlatformTools.kt b/core/koin-core/src/jsMain/kotlin/org/koin/mp/PlatformTools.kt
@@ -1,5 +1,22 @@
+/*
+ * Copyright 2017-present the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
 package org.koin.mp
 
+import co.touchlab.stately.concurrency.ThreadLocalRef
 import org.koin.core.context.GlobalContext
 import org.koin.core.context.KoinContext
 import org.koin.core.logger.*
@@ -38,3 +55,5 @@ actual object KoinPlatformTools {
 }
 
 actual typealias Lockable = Any
+
+actual typealias ThreadLocal<T> = ThreadLocalRef<T>
diff --git a/core/koin-core/src/jvmMain/kotlin/org/koin/mp/KoinPlatformTools.kt b/core/koin-core/src/jvmMain/kotlin/org/koin/mp/KoinPlatformTools.kt
@@ -1,3 +1,19 @@
+/*
+ * Copyright 2017-2023 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
 package org.koin.mp
 
 import org.koin.core.context.GlobalContext
@@ -22,3 +38,5 @@ actual object KoinPlatformTools {
 }
 
 actual typealias Lockable = Any
+
+actual typealias ThreadLocal<T> = java.lang.ThreadLocal<T>
diff --git a/core/koin-core/src/nativeMain/kotlin/org/koin/mp/KoinPlatformTools.kt b/core/koin-core/src/nativeMain/kotlin/org/koin/mp/KoinPlatformTools.kt
@@ -2,6 +2,7 @@ package org.koin.mp
 
 import co.touchlab.stately.concurrency.Lock
 import co.touchlab.stately.concurrency.withLock
+import co.touchlab.stately.concurrency.ThreadLocalRef
 import org.koin.core.context.KoinContext
 import org.koin.core.context.globalContextByMemoryModel
 import org.koin.core.logger.Level
@@ -33,3 +34,5 @@ actual object KoinPlatformTools {
 actual open class Lockable {
     internal val lock = Lock()
 }
+
+actual typealias ThreadLocal<T> = ThreadLocalRef<T>