Hey guys, I’ve found a nasty issue: the neverendin...

# orbit-mvi

[Oleksii Malovanyi]

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

[Oleksii Malovanyi]

okay, PR is ready for review

[appmattus]

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

[Mikolaj Leszczynski]

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]

Untitled.kt

[Oleksii Malovanyi]

@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

[Oleksii Malovanyi]

Also, FYI we can’t use runCatchnig directly, as it breaks cancellation as well

[Mikolaj Leszczynski]

we can rethrow cancellation exceptions

[Mikolaj Leszczynski]

or do you mean something else

[Oleksii Malovanyi]

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

[Oleksii Malovanyi]

but anyway, we can’t use underlying CoroutineExceptionHandler under the hood

[Oleksii Malovanyi]

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

[Mikolaj Leszczynski]

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

[Mikolaj Leszczynski]

I wonder if a hybrid solution is possible i.e. using coroutineScope and passing it the exception handler

[Mikolaj Leszczynski]

will have to test

[Oleksii Malovanyi]

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

[Mikolaj Leszczynski]

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 🙂

[Mikolaj Leszczynski]

we could do with a simple function

[Oleksii Malovanyi]

yes, but what’s the benefit?

[Mikolaj Leszczynski]

not messing around with jobs

[Mikolaj Leszczynski]

I’ll investigate if we can keep the standard mechanism but simplify it

[Mikolaj Leszczynski]

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

[Mikolaj Leszczynski]

I’m sure there’s an alternative that will keep both of us happy 🙂

[Mikolaj Leszczynski]

will investigate in the evening

[Oleksii Malovanyi]

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

[Mikolaj Leszczynski]

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.

[Oleksii Malovanyi]

sorry for being a bit aggressive, Monday my bad 😞

[Mikolaj Leszczynski]

Relax, we’ll get it fixed 😉

[Oleksii Malovanyi]

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

[appmattus]

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.

[Oleksii Malovanyi]

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

[Oleksii Malovanyi]

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

[Mikolaj Leszczynski]

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.

[Oleksii Malovanyi]

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

[Mikolaj Leszczynski]

Fair enough

[Mikolaj Leszczynski]

I’ve merged and tagged 4.1.3 - now we need to wait for the build

[Oleksii Malovanyi]

thanks @Mikolaj Leszczynski I owe you 🍻