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

Review branch for #4208 #4370

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Review branch for #4208 #4370

wants to merge 3 commits into from

Conversation

dkhalanskyjb
Copy link
Collaborator

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.

* - 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.
*/
private val threadStateToRecover = commonThreadLocal<Pair<CoroutineContext, Any?>?>(Symbol("UndispatchedCoroutine"))
Copy link
Collaborator

@qwwdfsad qwwdfsad Mar 11, 2025

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 one UndispatchedCoroutine can overwrite the other UndispatchedCoroutine 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 qwwdfsad self-requested a review March 11, 2025 10:32
Copy link
Collaborator

@qwwdfsad qwwdfsad left a 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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* See the followin, boiled down example with inlined `withContinuationContext` body:
* See the following, boiled down example with inlined `withContinuationContext` body:

@dkhalanskyjb
Copy link
Collaborator Author

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:

class Oh {
    @Test
    fun smokeTest() {
        runBlocking {
            withContext(C()) {
                yield()
            }
            assertEquals(2, restores)
        }
    }

    private var restores = 0

    inner class C(): ThreadContextElement<Unit>, CoroutineContext.Key<C> {
        override fun updateThreadContext(context: CoroutineContext) {
        }

        override fun restoreThreadContext(context: CoroutineContext, oldState: Unit) {
            restores++
        }

        override val 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants