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

Oleksii Malovanyi

07/16/2021, 3:09 PM
Hey guys, I’ve found a nasty issue: the neverending flow collection from the
intent{}
– it’s never canceled on parent’s
scope.cancel
in case there is an
exceptionHandler
installed, e.g.: something like this would create a mem. leak:
Copy code
) : ViewModel(),
    ContainerHost<ProfileState, ProfileEffect> {

    override val container: Container<ProfileState, ProfileEffect> = container(
        initialState = ProfileState(authorized = null, account = null),
        savedStateHandle = savedStateHandle,
        settings = Container.Settings(exceptionHandler = exceptionHandler.asCoroutineExceptionHandler()),
    ) {
        intent {
            subscribeToSessionUseCase().collect {
                reduce { state.copy(authorized = it != null) }
            }
        }
    }
Here is the sneak peak of the fix – I gonna create a test for that, but wonna let you know early https://github.com/orbit-mvi/orbit-mvi/pull/58
okay, PR is ready for review
a

appmattus

07/17/2021, 5:31 PM
Thanks, I’ll take a proper look on Monday :) As an FYI if you were to ever have an espresso test where there’s an infinite flow you should disable idle resource handling and handle them yourself. If you don’t then it will never go idle. Use:
intent(registerIdling = false) { …
m

Mikolaj Leszczynski

07/17/2021, 8:17 PM
Oleksy, thank you for the initiative. I had a look at your PR and while I’m happy with the fix, I feel a bit uneasy we’re doing non-standard logic around jobs to make the original solution work. My worry is that continuing down this path might cause other unexpected behaviour. I had another look myself and found an alternative solution that I’ve verified against your unit test (and the rest of our test suite). I’ve pushed it under
simple-exception-handler-test
The TL;DR • getting rid of supervisor scope • wrapping coroutine code inside the launched intent coroutine with
coroutineScope
- this will make any exceptions thrown by coroutines launched within catchable without the need to use an exception handler as explained in this article https://www.lukaslechner.com/why-exception-handling-with-kotlin-coroutines-is-so-hard-and-how-to-successfully-master-it/ • surrounding the new scope with
runCatching
to handle exceptions This is a very rough around the edges draft right now as I’m just forwarding exceptions to the handler defined in settings. My point is we wouldn’t even need one with this approach, we’re handling exceptions as defined by your test cases and we’re keeping the structured concurrency in place without having to recreate it. Happy to hear your thoughts @Oleksii Malovanyi @appmattus
o

Oleksii Malovanyi

07/19/2021, 9:15 AM
@Mikolaj Leszczynski thanks for the deep explanation. I feel a bit lost with the structured concurrency, and for now I can’t distinguish which solutions would be the best. Linking the newly created scope with the parent one via the job seems logical to me. Also, it has 1 serious benefit -> anyone inside the
intent
could use the underlying context implicitly to get the CoroutineExceptionHandler under the hood. E.g.:
Copy code
private fun refreshAccount() = intent {
        executeUseCase { refreshAccountUseCase() }
    }.onSuccess {
            //
    }.onFailure {
            //
    }
}
Copy code
suspend inline fun <R> executeUseCase(block: () -> R): Result<R> {
    val currentExecContext = coroutineScope { coroutineContext }
    val exceptionHandler = currentExecContext[CoroutineExceptionHandler]
    return runSuspendCatching(block)
        .onFailure {
            exceptionHandler?.handleException(currentExecContext, it)
        }
}
So, I’d go with the pure context based solution, as it doesn’t add new custom logic, but uses standart mechanisms of coroutines
Also, FYI we can’t use
runCatchnig
directly, as it breaks cancellation as well
m

Mikolaj Leszczynski

07/19/2021, 9:16 AM
we can rethrow cancellation exceptions
or do you mean something else
o

Oleksii Malovanyi

07/19/2021, 9:16 AM
yes, exactly
Copy code
inline fun <R> runSuspendCatching(block: () -> R): Result<R> {
    return try {
        Result.success(block())
    } catch (cancellationException: CancellationException) {
        throw cancellationException
    } catch (e: Throwable) {
        Result.failure(e)
    }
}
but anyway, we can’t use underlying
CoroutineExceptionHandler
under the hood
even more, the class we pass to orbit’s container (CoroutineExceptionHandler) missleads -> as it would be never used as it is, it would be used more like a custom handler class which is not connected with coroutines infrastructure any more
m

Mikolaj Leszczynski

07/19/2021, 9:18 AM
I don’t know if forwarding exceptions to it manually is the right thing to do tbh
I wonder if a hybrid solution is possible i.e. using
coroutineScope
and passing it the exception handler
will have to test
o

Oleksii Malovanyi

07/19/2021, 9:20 AM
I need to fix the test from my PR first, sorry 😞
m

Mikolaj Leszczynski

07/19/2021, 9:21 AM
even more, the class we pass to orbit’s container (CoroutineExceptionHandler) missleads -> as it would be never used as it is, it would be used more like a custom handler class which is not connected with coroutines infrastructure any more
As I mentioned before, I’m not suggesting we keep it as a CoroutineExceptionHandler 🙂
we could do with a simple function
o

Oleksii Malovanyi

07/19/2021, 9:21 AM
yes, but what’s the benefit?
m

Mikolaj Leszczynski

07/19/2021, 9:21 AM
not messing around with jobs
I’ll investigate if we can keep the standard mechanism but simplify it
the only reason you need this fix is cause we separate the coroutine out from the parent’s context through supervisor job - if there’s a way we can avoid it that’s what we should do
I’m sure there’s an alternative that will keep both of us happy 🙂
will investigate in the evening
o

Oleksii Malovanyi

07/19/2021, 9:24 AM
Sure, if you feel like so, need to try definitely. But I don’t see what’s wrong with the current solution and fix: • we’re creating a child job for each intent as we don’t want it to break all the tree up (supervisor) • we still want it be dependant on parent scope, so we link it to the parent job
m

Mikolaj Leszczynski

07/19/2021, 9:26 AM
Nothing wrong with it - I just see that we’re starting to complicate things and am worried about hidden consequences. If we can simplify this we should, if we can’t we’ll go with your approach.
o

Oleksii Malovanyi

07/19/2021, 9:28 AM
sorry for being a bit aggressive, Monday my bad 😞
m

Mikolaj Leszczynski

07/19/2021, 9:28 AM
Relax, we’ll get it fixed 😉
o

Oleksii Malovanyi

07/19/2021, 10:36 AM
FYI, I’ve pushed the fix for the cancellation test
👍 1
a

appmattus

07/19/2021, 11:04 AM
My concern with the use of coroutineexceptionhandlers is to me it’s feels like if you need one something isn’t being done right. Obviously I don’t know enough about the context of what you’re trying to achieve Oleksii so it’s not to say you should never need one. The fact is if an exception has occurred and not been handled properly then what does this mean for your current state? For example: fun doSomething() = intent { reduce { Loading } val data = someFunctionThatErrors() reduce { Ready(data) } } If someFunctionThatErrors() throws an exception then your existing state is left mangled with a value that shouldn’t be there. In some sense you need to handle the exception appropriately in every intent block. So what does your exception handler do? While it could “fix” the state, is that right? Probably not. The best you can do is log the exception.
o

Oleksii Malovanyi

07/19/2021, 11:11 AM
In my experience, the exceptionHandler is best for 2 things: • prevents production from crashing; • centralises logging of the non-expected warnings/fatals. For the case you’ve wrote in our current project we do always wrap the potential failing execution site with the wrapper (see
executeUseCase
from above in this thread) the same as we do for other coroutine-related stuff -> via the
coroutineContext[CoroutineExceptionHandler]
, which is quite good application of dependency inversion principle in my taste. Now the talk about a small fix which actually causes a mem. leak. Maybe, if you want to discuss the existence of the handler itself, I’d recommend to start another discussion to keep this focused
@Mikolaj Leszczynski I have another small observation regarding the mem. leak to think about -> I’ve noticed the
dispatchChannel
from the
RealContainer
is never cancelled -> so there could be a situation, probably, where the channel would have a buffered message but the receiver coroutine is already cancelled. I guess we should cancel/close the channel when scope completes as well. Smth like this could work (but requires some caution in terms of send invocation then):
👀 1
m

Mikolaj Leszczynski

07/19/2021, 7:13 PM
Hey I probably won't be able to look at it today, I had one hell of a Monday... Will pick it up as soon as I have some spare time.
🙏 1
o

Oleksii Malovanyi

07/19/2021, 7:23 PM
what I wanted to say how I feel: this is purely a hotfix and I’d appreciate if we treat it as so and maybe postponed some long-term refactoring or discussions for a better times
m

Mikolaj Leszczynski

07/19/2021, 7:32 PM
Fair enough
I’ve merged and tagged
4.1.3
- now we need to wait for the build
o

Oleksii Malovanyi

07/19/2021, 7:42 PM
thanks @Mikolaj Leszczynski I owe you 🍻
16 Views