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

Fixing race condition in Scope - Fixed for 3.5.0 #1643

Merged
merged 1 commit into from
Sep 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/koin-core/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ kotlin {
dependsOn commonMain
dependencies {
implementation 'org.jetbrains.kotlin:kotlin-stdlib-js'
implementation "co.touchlab:stately-concurrency:1.2.2"
}
}

Expand Down
71 changes: 30 additions & 41 deletions core/koin-core/src/commonMain/kotlin/org/koin/core/scope/Scope.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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
Expand Down Expand Up @@ -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
}
Expand All @@ -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? {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 19 additions & 0 deletions core/koin-core/src/jsMain/kotlin/org/koin/mp/PlatformTools.kt
Original file line number Diff line number Diff line change
@@ -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.*
Expand Down Expand Up @@ -38,3 +55,5 @@ actual object KoinPlatformTools {
}

actual typealias Lockable = Any

actual typealias ThreadLocal<T> = ThreadLocalRef<T>
18 changes: 18 additions & 0 deletions core/koin-core/src/jvmMain/kotlin/org/koin/mp/KoinPlatformTools.kt
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -22,3 +38,5 @@ actual object KoinPlatformTools {
}

actual typealias Lockable = Any

actual typealias ThreadLocal<T> = java.lang.ThreadLocal<T>
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -33,3 +34,5 @@ actual object KoinPlatformTools {
actual open class Lockable {
internal val lock = Lock()
}

actual typealias ThreadLocal<T> = ThreadLocalRef<T>
Loading