- Notifications
You must be signed in to change notification settings - Fork1.9k
Review branch for #4208#4370
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:develop
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| * - It's never accessed when we are sure there are no thread context elements | ||
| * - It's cleaned up via [ThreadLocal.remove] as soon as the coroutine is suspended or finished. | ||
| */ | ||
| privateval threadStateToRecover= commonThreadLocal<Pair<CoroutineContext,Any?>?>(Symbol("UndispatchedCoroutine")) |
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.
Is it a legal pattern to use in this place?
K/N thread locals are name-bound, which means that oneUndispatchedCoroutine can overwrite the otherUndispatchedCoroutine thread local when they are nested on the same thread.
So something like
withContext(..forcePopulationOfThreadStateRecover...) { withContext(..doItAgain..) {}}might actually overwrite the state. Is it the case or am I missing something?
qwwdfsad left a comment
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.
Tracking the history of changes would be hard, taking that git considers all that as removal-addition of files. Not sure if we can do anything with that though
| /** | ||
| * The state of [ThreadContextElement]s associated with the current undispatched coroutine. | ||
| * It is stored in a thread local because this coroutine can be used concurrently in suspend-resume race scenario. | ||
| * See the followin, boiled down example with inlined `withContinuationContext` body: |
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.
| *See thefollowin, boiled down example with inlined `withContinuationContext` body: | |
| *See thefollowing, boiled down example with inlined `withContinuationContext` body: |
dkhalanskyjb commentedApr 8, 2025
A heads-up: after looking at@qwwdfsad's comment, I was wondering why no test caught this problem if it's a real one and went to investigate. I've noticed that some tests were not ported from the JVM to other targets and decided to try to move them. However, seemingly unrelated tests failed: for some reason, outside the JVM, thread context elements don't get cleaned up. This fully deterministic, single-threaded test fails on Native, but passes on the JVM: classOh { @TestfunsmokeTest() { runBlocking { withContext(C()) {yield() } assertEquals(2, restores) } }privatevar restores=0innerclassC(): ThreadContextElement<Unit>, CoroutineContext.Key<C> {overridefunupdateThreadContext(context:CoroutineContext) { }overridefunrestoreThreadContext(context:CoroutineContext,oldState:Unit) { restores++ }overrideval key:CoroutineContext.Key<*> get()=this }} Curiously, I don't think it's the problem reported by@qwwdfsad, as this persists even when I change the implementation of thread-local variables on Native to account for the problem. I don't yet understand what's going on here. |
dkhalanskyjb commentedMay 5, 2025
Root cause of the problem described above: runBlocking {val result= suspendCoroutine { uCont-> uCont.resume(uContisCoroutineStackFrame) } assertTrue(result)}fails on Native, but succeeds on the JVM. Because of this, |
| * | ||
| * Usage note: | ||
| * | ||
| * This part of the code is performance-sensitive. |
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.
Worth noting that it's JVM-specific
domgew commentedAug 2, 2025
@dkhalanskyjb@qwwdfsad@zuevmaxim |
murfel commentedAug 4, 2025
@domgew definitely not in the next month, more likely in a while, I'm afraid. |
To simplify reviewing#4208, I've rebased the changes provided there into three clearly separated commits. After this PR gets approved, the changes introduced here will be backported to#4208, and we'll merge that.