Remove running on `EmptyCoroutineContext #307
Conversation
nomisRev
changed the title
nomisRev
requested review from
1Jajen1,
JorgeCastilloPrz,
aballano,
danimontoya,
raulraja and
a team
| */ | ||
| internal class ForwardCancellable { | ||
| internal class ForwardCancellable(private val ctx: CoroutineContext) { |
There was a problem hiding this comment.
Is this, or should this, be configurable in any way in the api of the methods using it?
There was a problem hiding this comment.
Yes, this could definitely be configurable. I'm still wondering a bit, since running cancel tokens on an IOPool is probably safest.
Now the're most likely to run on the default pool, but for the current usages I think this makes most sense. It's used in bracketCase and it's derived methods, and cancellable. It makes the release and CancelToken to be run with the same ctx as their use/acquire and f functions.
| timerConn | ||
| ) { timeOut -> | ||
| // Launch timer on current thread (Unintercepted) with default ctx (sleep returns there), and timer connection | ||
| // Launch on current thread, since it will immediately fork to sleeper scheduler, and free current Thread |
There was a problem hiding this comment.
I don't get this part, aren't you launching below on the Computation pool thread and not the current one?
There was a problem hiding this comment.
suspend { sleep(duration) } is started with startCoroutineUnintercepted which means it immediately starts running without getting intercepted() which means it won't get scheduled on ComputationPool.
However sleep requires a ContinuationInterceptor to be installed, otherwise, it cannot shift away from the sleep scheduler when it returns from sleeping. This is problematic since nothing is supposed to run on the scheduling sleep.
So by using startCoroutineUnintercepted and ComputationPool we can launch the sleeping fiber without first intercepting/scheduling, and we install ComputationPool as a ContinuationInterceptor for sleep to safely return there.
Running tokens on
EmptyCoroutineContextremoves theCoroutineContextfrom thesuspendprogram, and potentially makes us lose state. Instead we should always favorUnintercepted+coroutineContext/Continuation#contextto runsuspendcode on current thread with existingCoroutineContextstate.