Hello All! I've published a Detekt ruleset that en...

# coroutines

[Rob]

Hello All!
I've published a Detekt ruleset that ensures correct handling of coroutine cancellation in catch blocks. Please try it out and provide feedback. Thanks!
https://github.com/zirman/robs-rules/

[ursus]

curious, why not check for rethrowing cancellationexception instead? first time im seeing this

[Rob]

Because a CancellationException is also thrown in cases where the coroutine is still active.
https://github.com/Kotlin/kotlinx.coroutines/issues/3658

[ursus]

i see but wasnt it the kotlin team that recommended the rethrowing?

[ursus]

or am I cargo culting?

[Rob]

In that thread @elizarov says this:

IMO, the great takeaway from the original post on the "The Silent Killer" is this pattern:

try {
    doStuff()
} catch (error: Throwable) {
    coroutineContext.ensureActive()
    handleError(error)
}
That should be the recommended pattern to use instead of trying to catch CancellationException and ignoring it. That is a clear documentation thing, but we might also consider doing some utilities to help with it.

[ursus]

well but rethrowing isnt ignoring, no?

[Rob]

Like I said, if the coroutine is still active, you can treat CoroutineCancellation like a normal exception and try to recover. Please read this as well to fully understand why rethrowing is not always the correct.
https://medium.com/better-programming/the-silent-killer-thats-crashing-your-coroutines-9171d1e8f79b

[louiscad]

ensureActive() is technically problematic if you wrap calls to flow's emit(…) with a try catch as operators like first() throw a CancellationException without a middle cancelled scope to tell you it's no longer active.

[ursus]

oh jeez 😀 so what do you do?

[louiscad]

I rethrow CancellationException and I don't use them for recoverable cancellation.

[ursus]

so the whole blog post is moot?

[Rob]

Can you provide a concrete example. I'm not sure I follow.

[louiscad]

Well, catching calls to emit is more than weird too. To me, both are totally fine, I avoid weird things that play with what coroutines rely on.

[louiscad]

Sure @Rob :

val stuff = flow {
    while (true) try {
        emit(whatever())
        delay(whoCaresHowLong)
    } catch (e: Exception) {
        e.ensureActive()
        reportExceptionSomehow(e)
    }
}.first()

This looks fine, but the loop won't be interrupted on the first value, and an infinite loop will ensue.

[louiscad]

In this example, rethrowing CancellationException works. This is because of the optimized implementation of first().

[ursus]

I currently always rethrow CE, so I'm now wondering if I should switch

[louiscad]

I think it's 100% fine, especially if you're not using CancellationException for recoverable cancellation (and I firmly believe it's not a good idea, even though I have been doing it a few times in the past)

[Rob]

e.ensureActive() should be currentCoroutineContext().ensureActive(). Exceptions don't implement ensureActive(). I don't see anything problematic.

[Rob]

Ok, now I see what you're saying.

[Rob]

I'm surprised that the flow doesn't have it's own Job. That is a subtle bug but really useful to know. I'll see if I can make it check for AbortFlowException

[Rob]

Heh, "never exposed". How would you check for a flow abort idiomatically?

[Rob]

This is the best I could come up with for handling both senarios. Since it's only needed when inside a FlowCollector , it should be detektable.

Untitled.kt

[Rob]

Interestingly using a channelFlow does create a new Job which is does get cancelled after getting the first element.