Support yield in immediate dispatchers

[elizarov]

Fixes #1474

[JakeWharton]

Thanks!

[elizarov]

@JakeWharton Note, that if somebody creates a wrapper around Unconfined dispatcher or writes their own dispatcher like this:

object MyUnconfined : CoroutineDispatcher() {
    override fun isDispatchNeeded(context: CoroutineContext): Boolean = false
    override fun dispatch(context: CoroutineContext, block: Runnable) = block.run() // !!!
}

Then yield() in such an MyUnconfined dispatcher implementation would never work. It will not be able to actually "yield". So, using yield() to work-around java.lang.reflect.Proxy checked exception problem is not 100% fool-proof. I wonder if we can figure out a solution that works all the time.

[JakeWharton]

The first thing I reached for was delay(1), but it made me sad.

[elizarov]

The first thing I reached for was delay(1), but it made me sad.

The other potential problematic case would be coroutine running without a dispatcher at all (a truly unconfined coroutine). The existing and updated version of yield is implemented as "nop" in this case. It does not do anything. So, I see two conceptual ways to solve it:

1. Retrofit-like libraries that use Proxy can rely on their own thread-pool to schedule continuation.resumeWithException to happen later in their own thread.

2. We can update implementation of yield in such a way that it ether use dispatcher-provided ability to yield thread or if there is no dispatcher or dispatcher is unconfined, then yield() would use our built-in DefaultExecutor to yield --- that same executor that is used as a fallback to implement delay(...) for dispatchers that do not support it natively.

[elizarov]

@JakeWharton I gave a bit more thought to the second idea. It is not really good. The change in this PR (support yield for immediate) is good. But if we provide a hard guarantee that "yield always suspends" then it means that in some simple situations without a dispatcher execution after yield() must resume in another thread. This would be surprising.

However, it is not surprising if some code that uses a remote-call library like retrofit without a dispatcher gets resumed in another thread. So, the first option to work around Proxy exception handling seems better to me. I'd suggest to copy and use the following function instead of yield() as an exception work-around:

import kotlin.coroutines.*
import kotlin.coroutines.intrinsics.*

suspend fun ensureSuspension() = suspendCoroutineUninterceptedOrReturn<Unit> {
    Dispatchers.Default.dispatch(it.context, Runnable { it.intercepted().resume(Unit) })
    COROUTINE_SUSPENDED
}

UPDATE: Corrected to use suspendCoroutineUninterceptedOrReturn.

You'd use just like you use yield() now inside your Proxy:

try { 
   // do something that might throw exception w/o suspending
} catch (e: Throwable) {
   ensureSuspension()
   throw e // rethrow exception after guaranteed suspension 
}

[qwwdfsad]

Well done, please squash before merging

[elizarov]

I've found a problem with this change. We try to dispatch to dispatchers that return false from isDispatchNeed to support immediate dispatcher. Now consider a user that had accidentally wrapped Unconfined dispatcher for whatever reason. Maybe they have some tracking framework and need their own CorouteDispatcher wrapper or whatever. With such a wrapper on top of Unconfined calling yield from a loop would cause StackOverflowError. I've added a test to demonstrate it.

As a fix, yield now adds a special coroutine context element that is detected by Unconfined dispatcher so they still work correctly in tandem even when Unconfined dispatcher is wrapped. I've also updated docs for CoroutineDispatcher.dispatch.

The downside of this change is that a few extra objects are created on each yield call. I have another implementation strategy based on ThreadLocal which I've tried and it also works, but I've found the resulting code to be somewhat more cryptic.

Please, review.