Document and fix dispatchYield codepath

[qwwdfsad]

No description provided.

[qwwdfsad]

I skipped the test, but I checked the original report, and it is resolved.

I feel uneasy writing a very dedicated yield-targeting stress test for the codepath that no longer exist

[dkhalanskyjb]

What you're proposing is better than what we have now, but it's still not ideal. We are just one step farther from seeing the same effect. LoggingDispatcher(Dispatchers.IO) will exhibit this behavior, as the author of LoggingDispatcher can't override dispatchYield. That's a problem we're also facing with Delay (people can't wrap Delay implementations and expect the same behavior).

I see the following options of dealing with this problem for good:

• Make the YieldContext marker used by Dispatchers.Unconfined available to all dispatchers (and not just those not needing a dispatch) and check its presence when dispatch'ing to the coroutine scheduler; remove dispatchYield completely. Probably too slow to check that on every dispatch?
• Apply the proposed fix, but also design and publish some support for wrapping coroutine dispatchers that takes care to propagate dispatchYield.
• Make dispatchYield a valid optimization in a wide range of scenarios (including things like "skip suspensions on Dispatchers.Main if the looper is empty anyway") by recognizing the coroutine not being yet dispatched to the correct execution context as a separate state. EDIT: ... and make dispatchYield public, as it will no longer be deeply tied to our implementation details.

Thoughts?

[qwwdfsad]

That's correct, and it seems like the proper solution includes a solution for Delay as well.
In general, it's not really sustainable to include @Internal* methods or superinterfaces into the class that we expect people to extend.

Probably too slow to check that on every dispatch?

Yes, this option we'd rather avoid -- quite a non-deterministic check for the sake of code path that is really rare.

I would merge this as is and, as a next step, would consider sticking to the third option.
Probably, making dispatchYield public should be enough, not sure about the yield optimization (as it's quite niche)

[dkhalanskyjb]

not sure about the yield optimization (as it's quite niche)

My idea actually wasn't to introduce the optimizations (they are even more niche than even yield(), which itself is niche) (though introducing the optimizations actually shouldn't be difficult) but to ensure that if someone does override dispatchYield in favor of a non-trivial optimized implementation, that implementation wouldn't break the way ours did in #4248. If we introduce dispatchYield for public implementation without fixing the state machine, the most obvious usage of dispatchYield becomes a footgun.