Review branch for #4208

[dkhalanskyjb]

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.

[qwwdfsad]

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]

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

[qwwdfsad]

Suggested change

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

[dkhalanskyjb]

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.

[dkhalanskyjb]

Root cause of the problem described above:

runBlocking {
    val result = suspendCoroutine { uCont ->
        uCont.resume(uCont is CoroutineStackFrame)
    }
    assertTrue(result)
}

fails on Native, but succeeds on the JVM. Because of this, Continuation<*>.updateUndispatchedCompletion on Native immediately exits.

[qwwdfsad]

Worth noting that it's JVM-specific

[domgew]

@dkhalanskyjb @qwwdfsad @zuevmaxim
Is there any ETA for when this could be available?

[murfel]

@domgew definitely not in the next month, more likely in a while, I'm afraid.

[whyoleg]

Hey! Are there any plans to finalize this PR? Is any help needed? :)

[dkhalanskyjb]

@whyoleg, yes, there are plans to finalize this PR. What's holding it back is a subtle dependency on the JVM-specific behavior in the code of ThreadContextElement. Fixing that requires spending several days on rewriting some code in the internal coroutines machinery. So far, an opportunity to do such a deep dive without being sidetracked by the more urgent tasks has not presented itself. Since this task requires knowledge of the kotlinx.coroutines internals, I don't expect outside help to arrive here. That said, if someone is feeling ambitious, we will gladly accept the contribution.