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:

) : 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

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) { …

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

@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.:

private fun refreshAccount() = intent {
        executeUseCase { refreshAccountUseCase() }
    }.onSuccess {
            //
    }.onFailure {
            //
    }
}

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

we can rethrow cancellation exceptions

yes, exactly

inline fun <R> runSuspendCatching(block: () -> R): Result<R> {
    return try {
        Result.success(block())
    } catch (cancellationException: CancellationException) {
        throw cancellationException
    } catch (e: Throwable) {
        Result.failure(e)
    }
}

I don’t know if forwarding exceptions to it manually is the right thing to do tbh

I need to fix the test from my PR first, sorry 😞

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 🙂

yes, but what’s the benefit?

not messing around with jobs

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

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.

sorry for being a bit aggressive, Monday my bad 😞

Relax, we’ll get it fixed 😉

FYI, I’ve pushed the fix for the cancellation test

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.

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

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.

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

Fair enough

thanks @Mikolaj Leszczynski I owe you 🍻