Calling ensureActive instead of re-throwing CancellationException in suspendRunCatching

[azabost]

In suspendRunCatching, a CancellationException gets re-thrown:

https://github.com/android/nowinandroid/blob/689ef92e41427ab70f82e2c9fe59755441deae92/core/data/src/main/kotlin/com/google/samples/apps/nowinandroid/core/data/SyncUtilities.kt#L58

However, according to this post (further acknowledged here), it's more advisable to ensureActive() instead. Maybe like this?

private suspend fun <T> suspendRunCatching(block: suspend () -> T): Result<T> = try {
    Result.success(block())
} catch (cancellationException: CancellationException) {
    currentCoroutineContext().ensureActive() // if _our_ coroutine was cancelled - respect the cancellation
    Result.failure(cancellationException) // otherwise, just treat it as a failure
} catch (exception: Exception) {
    Log.i(
        "suspendRunCatching",
        "Failed to evaluate a suspendRunCatchingBlock. Returning failure Result",
        exception,
    )
    Result.failure(exception)
}

What do you think?

[Jaehwa-Noh]

I think the current code is fine.

The passing cancelation throw is the basic concept of coroutine in structured concurrency.

If you think it is silent cancel. You can insert log before throw exception.

What do you think of it?

[azabost]

The passing cancelation throw is the basic concept of coroutine in structured concurrency.

Have you read the linked article, though? Its subtitle literally says: "There’s only one safe way to deal with cancellation exceptions in Kotlin, and it’s not to re-throw them" (emphasis added), suggesting that the current code, which simply rethrows, could be improved.

What do you think?

[Jaehwa-Noh]

I just share my opinion. I see that article briefly and search that ensureActiviry source code.

That source code also throws cancellation.

public fun Job.ensureActive(): Unit {
    if (!isActive) throw getCancellationException()
}

And this project code using for a sync worker and throw cancellation will occur Result.retry()

[azabost]

Here's the difference between what you currently have and what I propose based on the linked article:

Before	After

[Jaehwa-Noh]

When in after case is normally end the Job. So that parent coroutine doesn't cancel any other child jobs.

When the throw occurs in a child firstly and it goes up to parent case, ensureActive always doesn't throw cancellationException because parent is not canceled that moment.