Fixing race condition in Scope #1561
Conversation
octa-one
force-pushed
the
bugfix/parameters-race
branch
from
84bbc98
to
1811bb6
Compare
| @@ -71,6 +71,7 @@ kotlin { | |||
| dependsOn commonMain | |||
| dependencies { | |||
| implementation 'org.jetbrains.kotlin:kotlin-stdlib-js' | |||
| implementation "co.touchlab:stately-concurrency:1.2.2" | |||
There was a problem hiding this comment.
I'm approving this PR because the entire code make senses. But I don't know if @kotzilla-io thinks in go on with add or be accoupled with other library.
It's not a techinical issue, it's just approach, ok?
There was a problem hiding this comment.
This library is already in use for the native platform. To be more specific, for the co.touchlab.stately.concurrency.Lock.
I just included the same version for the JS platform.
So no new dependencies were added.
| @@ -56,7 +57,7 @@ data class Scope( | |||
| private val _callbacks = arrayListOf<ScopeCallback>() | |||
|
|
|||
| @KoinInternalApi | |||
| val _parameterStack = ArrayDeque<ParametersHolder>() | |||
| val _parameterStackLocal = ThreadLocal<ArrayDeque<ParametersHolder>>() | |||
There was a problem hiding this comment.
I understand your PR description and why you are doing this but I can't help to think these modifications may introduce side effects we are not aware of.
The previous implementation would lock in any modification in the collection elements, for example:
KoinPlatformTools.synchronized(this@Scope) {
// Locks until addFirst finishes, collection modification.
_parameterStack.addFirst(parameters)
}
The new implementation seems to only lock during the variable read but not when the collection elements are modified. For example:
// Lock in access...
val localDeque = _parameterStackLocal.get()
// Further operations are not thread safe:
localDeque.addFirst(parameters)
localDeque.removeFirstOrNull()Again, I understand that is intentional based on your description but maybe we want to be conservative here and still handle elements modification in a type safe manner?
There was a problem hiding this comment.
Thank you for the review!
_parameterStackLocal.get() will return different collections for different threads. So there cannot be a race condition here. Each thread will work exclusively with its own collection.
There was a problem hiding this comment.
That is interesting, thank you for clarifying. Sounds good to me so.
arnaudgiuliani
added
core
status:checking
|
Checking in progress 👍 |
|
It'sa serious case here. Do you have a unit test to reproduce this race condition problem? |
|
The test provided in this comment reproduces the issue for me, if I remove the |
|
I will deal with conflicts. I'm testing this PR 👍 |
arnaudgiuliani
added
status:accepted
|
pushed your branch rebased on 3.5.0: bugfix/parameters-race Repushing it from Koin repo |
… parallele resolution fix - #1561"
A race condition may occur when using the same Scope from different threads.
Now only add and remove calls are synchronized. And operations between these calls can be in a race.
Imagine that 2 threads are simultaneously resolving instances from some scope. These instances need parameters to be created. This means that some of their dependencies will be found in
_parameterStack.We call the
resolveInstancemethod simultaneously from both threads.The first thread puts the parameters on the
_parameterStack. The second thread does the same. Next, the first thread calls_parameterStack.firstOrNull()and gets parameters from the second thread.It turns out that the threads swapped their parameters.
This could be solved by additional synchronization, but in fact the threads don't need to be synchronized here. Resolution of particular dependencies takes place on a single thread, the main thing is to make sure that the stack is not changed from the outside during the resolution.
ThreadLocal is suitable for this purpose.
And since we made
_parameterStackthread local, we don't need to additionally synchronize access to it and can removeKoinPlatformTools.synchronized(this).