https://kotlinlang.org logo
Join Slack
Powered by
Hello. I have a feature request, wanna discuss qui...
# orbit-mvi
o
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?
m
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?
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.
o
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
Copy code
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)
    }
as far as I understand coroutines, then it affects only the topmost coroutine launched from scope, so we’re *(almost) fine
m
Ok, I see what you mean. I'll run some tests and have a think about this.
👍 1
I would have thought the parent scope gets torn down anyway. A simple unit test will help verify our assumptions
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.
outlook not good, but I’ll run these tests later this afternoon anyway 🙂
o
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
Copy code
scope.launch {
                for (msg in dispatchChannel) {
                    supervisorScope{
  launch(settings.handler + Dispatchers.Unconfined) { pluginContext.msg() }
                }
            }
}
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
m
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.
o
totally agree with you: behaviour must not change by default – orbit should throw as usual, smth like
Copy code
internal val defaultExceptionHandler by lazy { CoroutineExceptionHandler { _, t -> throw t } }
and
Copy code
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 😉
m
Yeah, cool
I was thinking we should make the error handler optional and have am alternative code path in the orbit event loop instead
👍 1
So as not to pollute the stack trace
But yeah let's talk about it around real code. Thanks a lot!
o
@Mikolaj Leszczynski PR is ready https://github.com/orbit-mvi/orbit-mvi/pull/39
m
Thanks, I will take a look at it sometime this week!
👍 1
all reviewed now
o
looks reasonable, thanks! I’ll push changes later today
👍 1
m
alternatively we can merge it as-is and I can fix this myself, up to you
o
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
m
okay - in that case I’ll merge it as is. Thanks a lot for your contribution!
o
my pleasure to contribute ✊
7 Views
Open in Slack
PreviousNext
Powered by