Hello. I have a feature request, wanna discuss quickly: what do you think about new property for `Co...

# orbit-mvi

[Oleksii Malovanyi]

Hello. I have a feature request, wanna discuss quickly: what do you think about new property for Container.Settings of type CoroutineExceptionHandler, so we could have a default exception handler for Android ViewModel?

[Mikolaj Leszczynski]

Hey @Oleksii Malovanyi, thanks for your feedback!

This is a nice idea. I think the best solution here is to change orbitDispatcher: CoroutineDispatcher to orbitContext: CoroutineContext , this would let you do what you want cause you can just go orbitContext = Dispatchers.Default + CoroutineExceptionHandler {…}. WDYT?

[Mikolaj Leszczynski]

You have to keep in mind that exceptions caught by such a handler will usually be fatal to the container anyway (scope gets destroyed). I can see it being useful for e.g. error logging though.

[Oleksii Malovanyi]

that’s why I’d go not with a orbitContext, maybe, but a separate dispatcher. We could avoid container fatality by using context+handler as a argument to launch

override fun orbit(orbitFlow: suspend ContainerContext<STATE, SIDE_EFFECT>.() -> Unit) {
        if (initialised.compareAndSet(expect = false, update = true)) {
            scope.produce<Unit>(Dispatchers.Unconfined) {
                awaitClose {
                    settings.idlingRegistry.close()
                }
            }
            scope.launch(settings.handler) {
                for (msg in dispatchChannel) {
                    launch(Dispatchers.Unconfined) { pluginContext.msg() }
                }
            }
        }
        dispatchChannel.offer(orbitFlow)
    }

[Oleksii Malovanyi]

as far as I understand coroutines, then it affects only the topmost coroutine launched from scope, so we’re *(almost) fine

[Mikolaj Leszczynski]

Ok, I see what you mean. I'll run some tests and have a think about this.

[Mikolaj Leszczynski]

I would have thought the parent scope gets torn down anyway. A simple unit test will help verify our assumptions

[Mikolaj Leszczynski]

CoroutineExceptionHandler is invoked only on uncaught exceptions — exceptions that were not handled in any other way. In particular, all children coroutines (coroutines created in the context of another Job) delegate handling of their exceptions to their parent coroutine, which also delegates to the parent, and so on until the root, so the CoroutineExceptionHandler installed in their context is never used.

[Mikolaj Leszczynski]

outlook not good, but I’ll run these tests later this afternoon anyway 🙂

[Oleksii Malovanyi]

CoroutineExceptionHandler context element on a root coroutine can be used as generic catch block for this root coroutine and all its children where custom exception handling may take place.

so by the docs I guess it should work when installed into the root coroutine of the scope. The only thing that warns me is the channel range for msg - if child coroutine throws, the whole for-each parent coroutine is stopped, and then it seems like the msg(s) receiving process is stopped till the next call of the orbit func. Maybe, it should be implemented with the help of additional supervisorScope

scope.launch {
                for (msg in dispatchChannel) {
                    supervisorScope{
  launch(settings.handler + Dispatchers.Unconfined) { pluginContext.msg() }
                }
            }
}

[Oleksii Malovanyi]

as I was expecting, you can’t install handler into scope or the root coroutine (as the scope is broken then, yes). In the opposite supervisorScope trick works well. Should I create a PR? Got changes ready anyway

[Mikolaj Leszczynski]

I was actually going to write to you as I finally found some time to give it thought this morning. I agree, your solution will work, but let’s sync up on the approach to take.

I don’t think this should be the default behaviour, we prefer a fail-fast approach. Why?

• In the handler we don’t necessarily know which coroutine the exception came from unless you take some extra steps, like setting an extra CoroutineName , so specific error handling can be difficult
• With the default approach everything unhandled ends up in crashlytics. With the handler you have to set up some sort of non-fatal error logging in place in production, otherwise this could hide genuine issues.
In light of this I think we can include this as an option, with the default being the current behaviour.

Happy for you to open a PR, please take the above into account.

[Oleksii Malovanyi]

totally agree with you: behaviour must not change by default – orbit should throw as usual, smth like

internal val defaultExceptionHandler by lazy { CoroutineExceptionHandler { _, t -> throw t } }

and

public class Settings(
        public val sideEffectBufferSize: Int = Channel.UNLIMITED,
        public val idlingRegistry: IdlingResource = NoopIdlingResource(),
        public val orbitDispatcher: CoroutineDispatcher = Dispatchers.Default,
        public val orbitExceptionHandler: CoroutineExceptionHandler = defaultExceptionHandler,
        public val backgroundDispatcher: CoroutineDispatcher = defaultBackgroundDispatcher,
    )

I’ll open PR soon, so we could proceed the discussion around the real code 😉

[Mikolaj Leszczynski]

Yeah, cool

[Mikolaj Leszczynski]

I was thinking we should make the error handler optional and have am alternative code path in the orbit event loop instead

[Mikolaj Leszczynski]

So as not to pollute the stack trace

[Mikolaj Leszczynski]

But yeah let's talk about it around real code. Thanks a lot!

[Oleksii Malovanyi]

@Mikolaj Leszczynski PR is ready https://github.com/orbit-mvi/orbit-mvi/pull/39

[Mikolaj Leszczynski]

Thanks, I will take a look at it sometime this week!

[Mikolaj Leszczynski]

all reviewed now

[Oleksii Malovanyi]

looks reasonable, thanks! I’ll push changes later today

[Mikolaj Leszczynski]

hey, I think you’ve missed https://github.com/orbit-mvi/orbit-mvi/pull/39#issuecomment-814651280

[Mikolaj Leszczynski]

alternatively we can merge it as-is and I can fix this myself, up to you

[Oleksii Malovanyi]

oh, really – I’ve missed it. Thanks for pointing out! I could fix it later in the evening – but if you have some time earlier, please, take it

[Mikolaj Leszczynski]

okay - in that case I’ll merge it as is. Thanks a lot for your contribution!

[Oleksii Malovanyi]

my pleasure to contribute ✊