Fix a race condition when using parametersOf
#2306
Conversation
`Scope` was incorrectly using `ThreadLocal` leading to a race condition. The actual issue was not putting `ArrayDeque` in a thread-local variable but creating the thread-local variable itself. Several concurrent threads could have created several instances of the `parameterStack` thread-local variable and only the last one would have used. The correct usage of `ThreadLocal` is to instantiate it directly and let the `ThreadLocal` machinery to deal with synchronization. This commit also adds a test that was failing prior to the fix. I've also added `iosSimulatorArm64` to the `:core:koin-test-coroutines` to test the fix on iOS. This change fixes InsertKoinIO#2305.
|
|
||
| private fun getOrCreateParameterStack(): ArrayDeque<ParametersHolder> { | ||
| return parameterStack?.get() ?: ArrayDeque<ParametersHolder>().let { parameterStack = ThreadLocal(); parameterStack?.set(it) ; it } | ||
| val oldStack = parameterStack.get() |
There was a problem hiding this comment.
You remove ThreadLocal capacity to set those parameters?
There was a problem hiding this comment.
I don't. If the ThreadLocal parameterStack holds a parameters list already, it's returned.
If it's not, a new list is created and the existing parameterStack is updated.
The code works similarly to how it worked before (albeit not a one liner, I found it confusing to read) with one difference - instead of creating a new instance of a ThreadLocal variable we only create a list instance and update the existing ThreadLocal variable. This is how ThreadLocal is supposed to work.
There was a problem hiding this comment.
Pull request overview
This PR fixes a race condition in Scope’s use of thread-local parameters when resolving dependencies with parametersOf and adds a regression test that previously failed. It also enables iosSimulatorArm64 for the koin-test-coroutines module to validate the fix on iOS.
- Replace racy recreation of ThreadLocal with a single, stable ThreadLocal field managed via get/set/remove.
- Add a multi-threaded test ensuring parametersOf works concurrently.
- Enable iosSimulatorArm64 target for test coverage on iOS.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| projects/core/koin-core/src/commonMain/kotlin/org/koin/core/scope/Scope.kt | Fix race by instantiating a single ThreadLocal<ArrayDeque> and managing per-thread stacks via get/set/remove; minor import cleanup. |
| projects/core/koin-test-coroutines/src/commonTest/kotlin/org/koin/test/ParametersOfMultiThreadedTest.kt | New regression test that launches concurrent resolution using parametersOf and verifies both components resolve. |
| projects/core/koin-test-coroutines/build.gradle.kts | Add iosSimulatorArm64() target to run the new test on iOS simulator. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
projects/core/koin-core/src/commonMain/kotlin/org/koin/core/scope/Scope.kt
Outdated
Show resolved
Hide resolved
It is not reset anymore, no need for it to be a var
Not that it matters, as the scope is being closed and the memory will be released anyway, but it's more accurate and robust to call `remove` instead of clearing the list manually.
2 participants
Scopewas incorrectly usingThreadLocalleading to a race condition. The actual issue was not puttingArrayDequein a thread-local variable but creating the thread-local variable itself. Several concurrent threads could have created several instances of theparameterStackthread-local variable and only the last one would have used.The correct usage of
ThreadLocalis to instantiate it directly and let theThreadLocalmachinery to deal with synchronization.This commit also adds a test that was failing prior to the fix. I've also added
iosSimulatorArm64to the:core:koin-test-coroutinesto test the fix on iOS.This change fixes #2305.