Oleksii Malovanyi
07/16/2021, 3:09 PMintent{}
– 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/58appmattus
07/17/2021, 5:31 PMintent(registerIdling = false) { …
Mikolaj Leszczynski
07/17/2021, 8:17 PMsimple-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 @appmattusOleksii Malovanyi
07/19/2021, 9:15 AMintent
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 coroutinesrunCatchnig
directly, as it breaks cancellation as wellMikolaj Leszczynski
07/19/2021, 9:16 AMOleksii Malovanyi
07/19/2021, 9:16 AMinline fun <R> runSuspendCatching(block: () -> R): Result<R> {
return try {
Result.success(block())
} catch (cancellationException: CancellationException) {
throw cancellationException
} catch (e: Throwable) {
Result.failure(e)
}
}
CoroutineExceptionHandler
under the hoodMikolaj Leszczynski
07/19/2021, 9:18 AMcoroutineScope
and passing it the exception handlerOleksii Malovanyi
07/19/2021, 9:20 AMMikolaj Leszczynski
07/19/2021, 9:21 AMeven 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 moreAs I mentioned before, I’m not suggesting we keep it as a CoroutineExceptionHandler 🙂
Oleksii Malovanyi
07/19/2021, 9:21 AMMikolaj Leszczynski
07/19/2021, 9:21 AMOleksii Malovanyi
07/19/2021, 9:24 AMMikolaj Leszczynski
07/19/2021, 9:26 AMOleksii Malovanyi
07/19/2021, 9:28 AMMikolaj Leszczynski
07/19/2021, 9:28 AMOleksii Malovanyi
07/19/2021, 10:36 AMappmattus
07/19/2021, 11:04 AMOleksii Malovanyi
07/19/2021, 11:11 AMexecuteUseCase
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 focuseddispatchChannel
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
07/19/2021, 7:13 PMOleksii Malovanyi
07/19/2021, 7:23 PMMikolaj Leszczynski
07/19/2021, 7:32 PM4.1.3
- now we need to wait for the buildOleksii Malovanyi
07/19/2021, 7:42 PM