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
Fix race condition in scope resolveInstance #1465
Conversation
…ondition resulting in this stack trace: Fatal Exception: java.util.NoSuchElementException ArrayDeque is empty. kotlin.collections.ArrayDeque.removeFirst (ArrayDeque.kt:145) kotlin.collections.ArrayDeque.removeFirstOrNull (ArrayDeque.java:157) org.koin.core.scope.Scope.resolveInstance (Scope.kt:244) org.koin.core.scope.Scope.get (Scope.kt:204) org.koin.core.scope.Scope.getOrNull (Scope.kt:172) ArrayDeque is not a thread-safe collection, by wrapping calls to parameterStack in a synchronized block it makes sure the race condition cannot happen. This fixes InsertKoinIO#1149
@@ -49,6 +49,7 @@ kotlin { | |||
dependencies { | |||
implementation kotlin("test-common") | |||
implementation kotlin("test-annotations-common") | |||
implementation "org.jetbrains.kotlinx:kotlinx-coroutines-test:1.6.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not testImplementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's within the scope of commonTest
.
Great job! This PR solves exactly the problem I have been chasing for the past few days. I am waiting for it to be merged and released ASAP. |
Let me check it. Need to see perf impact also. |
Good job @npresseault 👍 |
Fix an issue where concurrent calls to koin.get would create a race condition resulting in this stack trace:
ArrayDeque is not a thread-safe collection, by wrapping calls to parameterStack in a synchronized block it makes sure the race condition cannot happen.
Also added a test that reproduced quite easily the problem.
Closes #1149