Android UI dispatcher and isDispatchNeeded

[Khang-NT]

Could we change implementation of method HandlerContext.isDispatchNeeded() to be more flexible:

public class HandlerContext(
    private val handler: Handler,
    private val name: String? = null,
    val alwaysDispatch: Boolean = true
) : CoroutineDispatcher(), Delay {

    override fun isDispatchNeeded(context: CoroutineContext): Boolean {
        return alwaysDispatch || Looper.myLooper() == handler.looper
    }

}

[qwwdfsad]

Please elaborate why do you need it.
Maybe it's worth to make HandlerContext open instead?

[Khang-NT]

I want to consume values of a ReceiveChannel in UI context.

launch(UI) {
    receiveChannel.consumeEach { value ->
        // stuff
    }
}

In the other side, SendChannel can send() on either main thread or background thread, I expect that if send(value) is invoked on main thread and alwaysDispatch == false, the value will pass to consumer immediately without dispatching to next event loop cycle.

I'm not sure 100% if isDispatchNeeded = false then the value will pass to consumer immediately if both send() and receive() are called on main thread. But I need that behavior to make coroutine channel acting like Android LiveData in my library:
https://github.com/Khang-NT/Kotlin-Coroutines-Lifecycle-Aware
(LiveData#setValue(newValue) will dispatch newValue immediately to all observers in same event loop.)

[qwwdfsad]

Instead of extending HandlerContext you can use delegation.

// You can name it UI or ConditionalUI or whatever
object ConditionalDispatcher : CoroutineDispatcher() {

    override fun dispatch(context: CoroutineContext, block: Runnable) = UI.dispatch(context, block)

    override fun isDispatchNeeded(context: CoroutineContext): Boolean {
        return Looper.myLooper() !== Looper.getMainLooper()
    }
}

Does it solve your problem?

[Khang-NT]

I also think of some solutions can solve my problem.

With my opinion, my problem is quite general so I open this issue whether we can official support within the library.

[bohsen]

Related to #258

[qwwdfsad]

We can't implement it as is, because accidental usage of such dispatcher will lead to spurious StackOverflowError reproducible sometimes.

We should provide something more explicit, e.g. new coroutine start mode or additional extension like UI.unconfined or UI.immediate.

Feel free to provide your option, the only requirement is that in place invocation should be explicit

[GeoffreyMetais]

I agree dispatching is somehow difficult to apprehend, and explicit dispatching option is mandatory.

I ended up doing a dedicated function to easily manage dispatching in UI:

fun uiJob(dispatch: Boolean = uiDispatch, block: suspend CoroutineScope.() -> Unit) : Job {
    return launch(UI, if (dispatch) CoroutineStart.DEFAULT else CoroutineStart.UNDISPATCHED, block = block)
}

With uiDispatch returns true if current thread is not UI thread. So I can explicitely select dispatching or let it automatic.

Would rather be a kotlinx-coroutines(-android) helper

[qwwdfsad]

Thanks for the feedback!
Your workaround works well for your use-case, but it's hard for the library to maintain it: we have a lot of dispatchers, a lot of builders and a lot of [default] parameters.

I'll try to prototype UI.some_extension as it's much easier to generalize

[GeoffreyMetais]

That would be great, I could get rid of my workaround :)

[LouisCAD]

@qwwdfsad I like the UI.some_extension design. I personally currently have a launchInUi(…) extension method, similar to @GeoffreyMetais's one, except it throws if not called on the UI thread, so there's no doubt about whether it's executed immediately or dispatched. It's started immediately, without initial dispatch, or fails fast.

About the technique to check the UI thread, I dislike using Looper comparison because Lopper.mainLooper() has a synchronized block (that is not really needed), and Looper.myLooper() does a ThreadLocal lookup.
This is some overhead that can be saved by caching the ui thread to a field, and just check it over the current thread, as I do here: https://github.com/LouisCAD/Splitties/blob/beb45da8ede57383aec489ac9d5630e79db60cc9/uithread/src/main/java/splitties/uithread/UiThread.kt#L30

[adamp]

This might be orthogonal to some of the issues you're trying to address here but...

If the primary issue is latency between posted Handler messages you might look first at skipping the vsync barriers that delay messages until after a pending frame. (The actual Looper message processing is quite fast on its own.) The api Handler.createAsync was added in API 28 to make this easier. Messages posted to an async handler are still ordered relative to one another, but they do not wait for pending frames to measure/layout/draw before running.

The constructor it uses has been there in AOSP nearly since the vsync barriers debuted in Jellybean so a hacky backport is possible in a support/androidx lib, we just haven't added it there yet. You can do your own for testing with something like this:

private val asyncHandlerConstructor = try {
    Handler::class.java.getConstructor(
            Looper::class.java,
            Handler.Callback::class.java,
            Boolean::class.java
    )
} catch (t: Throwable) {
    null
}

fun createAsyncHandler(
        looper: Looper = Looper.myLooper(),
        callback: Handler.Callback? = null
): Handler = when {
    Build.VERSION.SDK_INT >= 28 -> Handler.createAsync(looper, callback)
    Build.VERSION.SDK_INT >= 17 -> asyncHandlerConstructor?.newInstance(looper, callback, true)
            ?: Handler(looper, callback).also {
                Log.d("createAsyncHandler", "Couldn't create Handler as async!")
            }
    else -> Handler(looper, callback)
}

Then you can construct a HandlerContext with an async handler for the main thread that doesn't get blocked by UI operations:
val Main = HandlerContext(createAsyncHandler(Looper.getMainLooper()), "main")

Some folks working with RxAndroid saw some significant performance/latency benefits from this in testing and there's a PR over there in progress to use the same underlying mechanism: ReactiveX/RxAndroid#416

[qwwdfsad]

@LouisCAD
Do you have any performance numbers? Was this check so expensive that you've explicitly rewritten your code to mitigate it?

I don't mind additionally optimizing it, but it's good to know it's worth it. Looper comparison is user-friendly: even if javadoc/name is unclear, it's easy to understand what's going on in implementation after looking at single check, while solution with caching threads doesn't have this property.

@adamp Thanks! Let's see how this PR is going. One already can create coroutines dispatcher over any Handler whether it's async or not. If it's really useful, we can provide global constant like UI which uses async handler over main looper, name is the only problem :)

[adamp]

It might be better in the long term to make the UI dispatcher async by default.

The whole vsync barrier thing was a requirement for us to keep behavioral compatibility with a lot of existing code at the time but it's almost never what you want in new code and the latency hit is pretty huge. The pattern it was made to protect is this:

myView.setText("some text")
myView.post { doSomethingWith(myView.width) }

Where the width of a view isn't updated to reflect the newly set text until after measure/layout. Prior to vsync in jellybean requestLayout as performed by setText was implemented as a simple handler post so the operations were serialized.

I can't think of a good reason why someone would write a suspending version of something like TextView.setText that resumed after the frame such that this behavior would need to be preserved for the UI dispatcher. (And if they did, they could resume based on a post to a non-async Handler internally anyway.) Since the coroutines libs are still experimental it would be nice to push people toward dispatchers that skip them by default before there's a bunch of legacy code to think about. It's too bad RxAndroid can't go back in time and make this the default too.

[qwwdfsad]

Created #427 for discussing it

[LouisCAD]

@qwwdfsad I had written tests for this before taking my decision: https://github.com/LouisCAD/Splitties/blob/beb45da8ede57383aec489ac9d5630e79db60cc9/checkedlazy/src/androidTest/java/splitties/checkedlazy/PerformanceTest.kt

[adamp]

Regarding the original problem - getting dispatch behavior similar to LiveData where downstream behavior can interrupt or otherwise affect the rest of the dispatch - it sounds like you either want observers that need to affect the dispatch to use Unconfined or the wrapped dispatcher proposed above. However, using anything other than Unconfined here means that under some circumstances that might not be easily reproduced, your observer won't be able to affect the upstream dispatch, but it might assume it can. That sounds like a recipe for bugs.

It also looks like skipping dispatch in the manner proposed here also breaks some ordering guarantees. Consider:

1. Message A is posted to the Handler
2. Message B is posted to the Handler
3. Message A executes, performs some operation that results in processing and dispatch of Message C
4. Message C skips dispatch and executes immediately since it's on the main thread already
5. Message B executes after Message C has executed

If message B and C were the result of dequeuing some sort of work from the same queue, they're now out of order. This might happen many layers deep in the associated code and be very difficult to track.

A number of RxJava users in the Android community have written Schedulers that behave this way and got bit by ordering bugs like the above. The answer on that side of things has usually been either, "don't use observeOn if you need it to run synchronously" (Unconfined) or to address the latency issues that come from the vsync barriers if that's the motivation.

[LouisCAD]

If you want Message B to run after Message A and before Message C, with coroutines, you just have to launch a single coroutine, which executes sequentially (i.e. in order), instead of launching multiple coroutines. The syntax of coroutines make it hard to have a bug like you describe unintentionally, regardless of whether there's dispatch or not.

…

On Mon, Jul 9, 2018, 8:19 PM Adam Powell ***@***.***> wrote: Regarding the original problem - getting dispatch behavior similar to LiveData where downstream behavior can interrupt or otherwise affect the rest of the dispatch - it sounds like you either want observers that need to affect the dispatch to use Unconfined or the wrapped dispatcher proposed above. However, using anything other than Unconfined here means that under some circumstances that might not be easily reproduced, your observer won't be able to affect the upstream dispatch, but it might assume it can. That sounds like a recipe for bugs. It also looks like skipping dispatch in the manner proposed here also breaks some ordering guarantees. Consider: 1. Message A is posted to the Handler 2. Message B is posted to the Handler 3. Message A executes, performs some operation that results in processing and dispatch of Message C 4. Message C skips dispatch and executes immediately since it's on the main thread already 5. Message B executes after Message C has executed If message B and C were the result of dequeuing some sort of work from the same queue, they're now out of order. This might happen many layers deep in the associated code and be very difficult to track. A number of RxJava users in the Android community have written Schedulers that behave this way and got bit by ordering bugs like the above. The answer on that side of things has usually been either, "don't use observeOn if you need it to run synchronously" (Unconfined) or to address the latency issues that come from the vsync barriers if that's the motivation. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#381 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGpvBaio7HbCr0QY2qoQa_WfEVpffwgaks5uE56UgaJpZM4UYMo5> .

[JakeWharton]

Nothing about coroutines precludes the ability to have the above problem. The use a single coroutine represents only a fraction of the interaction with a single-threaded scheduler.

[adamp]

It's harder, for sure. It could still interact poorly with other code a couple layers away though, and giving it prominent placement in the API could lead people to think that it's a general purpose, "go faster" button when it may not be addressing the right problems.

It seems like the example code at the beginning of the thread assumes that the channel consumer will fully finish processing the item before the producer continues dispatching rather than just receive the item from the channel, this being the LiveData behavior described. Without an API guarantee that the dispatch won't continue until after processing of the item and not just receipt, relying on unconfined dispatch behavior alone to do this seems brittle.