https://kotlinlang.org logo
#orbit-mvi
Title
# orbit-mvi
o

Oleksii Malovanyi

03/30/2021, 2:47 PM
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

Mikolaj Leszczynski

03/30/2021, 4:08 PM
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

Oleksii Malovanyi

03/30/2021, 6:24 PM
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

Mikolaj Leszczynski

03/31/2021, 5:08 AM
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

Oleksii Malovanyi

03/31/2021, 8:12 AM
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

Mikolaj Leszczynski

04/05/2021, 12:59 PM
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

Oleksii Malovanyi

04/05/2021, 1:03 PM
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

Mikolaj Leszczynski

04/05/2021, 1:05 PM
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

Oleksii Malovanyi

04/06/2021, 6:45 AM
@Mikolaj Leszczynski PR is ready https://github.com/orbit-mvi/orbit-mvi/pull/39
m

Mikolaj Leszczynski

04/06/2021, 6:46 AM
Thanks, I will take a look at it sometime this week!
👍 1
all reviewed now
o

Oleksii Malovanyi

04/07/2021, 6:53 AM
looks reasonable, thanks! I’ll push changes later today
👍 1
m

Mikolaj Leszczynski

04/08/2021, 6:30 AM
alternatively we can merge it as-is and I can fix this myself, up to you
o

Oleksii Malovanyi

04/08/2021, 6:33 AM
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

Mikolaj Leszczynski

04/08/2021, 6:34 AM
okay - in that case I’ll merge it as is. Thanks a lot for your contribution!
o

Oleksii Malovanyi

04/08/2021, 6:34 AM
my pleasure to contribute
5 Views