CancellationException can be thrown not only to indicate cancellation

[dkhalanskyjb]

https://betterprogramming.pub/the-silent-killer-thats-crashing-your-coroutines-9171d1e8f79b raises a good point: CancellationException should indicate that the coroutine in which it is raised is cancelled, but it's not always actually the case:

    fun testFoo() {
        val scope = CoroutineScope(Dispatchers.IO)
        val f = scope.async {
            delay(1000)
        }
        scope.cancel()
        var result = false
        runBlocking {
            launch {
                f.await() // throws CancellationException, silently stopping the `launch` coroutine
                result = true
            }.join()
        }
        check(result) { "issue" }
    }

Here, the programmer didn't make the mistake of catching a CancellationException and rethrowing it somewhere else, but still, the CancellationException leaked from a different scope.

[dkhalanskyjb]

Need to dive deep into the implementation to see if this is possible, but it looks like modifying Deferred to throw a non-CancellationException may not be a good solution.

Consider this simplified scenario:

runBlocking {
  val result = async(Dispatchers.Default) {
    delay(5.seconds)
  }
  launch(Dispatchers.Unconfined) {
    result.await()
  }
  delay(1.seconds)
  cancel() // cancel the common scope
}

Here, we possibly have a race: the global scope is cancelled, which causes the coroutines in async and launch also to be cancelled. However, it can happen that this sequence of events happens before launch gets cancelled:

1. async gets cancelled,
2. the Deferred learns about the cancellation exception,
3. await finishes with a non-CancellationException in the launch,
4. which causes the launch to fail with a non-CancellationException,
5. with which the entire runBlocking fails!

So, the code can fail with one exception or rarely with another. This is just one simplified example of a broader class of races that we could introduce.

[dovchinnikov]

await (probably another function since this one is taken) should return kotlin.Result

• if await resumes with CE, then it means the current coroutine was cancelled;
• if await resumes with Result, then async was completed before the current coroutine was cancelled, and it represents a value/CE/failure of the async.

[dovchinnikov]

The above could be implemented as an extension function:

suspend fun Deferred<T>.awaitResult() : Result<T> {
  return try {
    Result.success(await())
  }
  catch (ce: CancellationException) {
    // this can "override" the original CE, but the current coroutine cancellation is preferred, so it's okay
    currentCoroutineContext().job.ensureActive()
    Result.failure(t)
  }
  catch (t: Throwable) {
    // resume awaitResult with the original throwable result to give a chance to handle it
    // even if the current coroutine is cancelled concurrently
    Result.failure(t)
  }
}

[dkhalanskyjb]

This is a robust solution, but non-Kotlin-style. Kotlin tries to keep hierarchies of return types as thin as possible to save the users the boilerplate of unpacking the results. For example, Map<Int, String?>::get doesn't return an Optional<String?>, it just returns String?.

[dovchinnikov]

Well, this is as minimal as Value | Throwable union can be at the moment

[dkhalanskyjb]

Just a Value (with the implicit Throwable that can surface at any point in a JVM program) is even more minimal.

[dovchinnikov]

But then you lose info about the origin of the Throwable, which is the very problem here. To check the origin, one has to catch the CE and use ensureActive manually.

[dovchinnikov]

Another approach is to expose a subclass of CE, which will mean "another coroutine was cancelled":

try {
  await()
}
catch (dce: DeferredCancellationException) { // name TBD
  // deferred was cancelled
}
catch (ce: CancellationException) {
  // this coroutine was cancelled
}

[fvasco]

Another approach is to expose a subclass of CE, which will mean "another coroutine was cancelled":

What should happen if the code in async fails with a DeferredCancellationException?

[elizarov]

If you are using deferred.await() in our code and you want to distinguish the case where the coroutine you're awaiting for was cancelled or not, you already can do it by explicitly checking deferred.isCancelled. However, I don't think the need to make this distinction happens very often.

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.

However, there is also one thing that can be fixed in the coroutines library itself in a backwards compatible way. The current state of affairs is that the coroutine that dies by a rogue CancellationException indeed does so completely silently, even though it is not cancelled itself. This can be clearly demonstrated by the following code:

suspend fun main() {
    val cancelledDeferred = CompletableDeferred<Int>()
    cancelledDeferred.cancel()
    supervisorScope {
        // This coroutine fails in a regular way
        launch(CoroutineName("one")) {
            error("Regular failure")
        }
        // This coroutine fails while waiting on cancelled coroutine
        launch(CoroutineName("two")) {
            cancelledDeferred.await()
        }
    }
}

Playground link

Neither coroutine "one" nor coroutine "two" is cancelled, both fail with exception, yet only the exception for the failure of the first coroutine is shown. We can consider changing this logic to report that the second coroutine has failed with CancelletationException, too.

[dovchinnikov]

The current state of affairs is that the coroutine that dies by a rogue CancellationException indeed does so completely silently, even though it is not cancelled itself

It can be handled by treating CE as failure if the current coroutine was not actually cancelled.
The coroutine can be cancelled concurrently, though, but the concurrent cancellation behaviour with a caught CE should be equivalent to the behaviour with a caught error("Regular failure").

Treating CEs in a non-cancelled coroutine as failure would imply that the manual cancellation should be done differently:

// instead of this:
launch {
  throw CancellationException() // would be a failure
}

// one would need to use this:
launch {
  coroutineContext.cancel()
  coroutineContext.ensureActive() // this CE will be caught in cancelled state -> not a failure
}

Now, a function for cancel() + ensureActive() which returns Nothing is useful regardless of how this would be resolved: cancel() (comparing to throw CancellationException()) triggers the cancellation of the child coroutines right away without waiting for CE to be caught by the coroutine.

[wkornewald]

If you are using deferred.await() in our code and you want to distinguish the case where the coroutine you're awaiting for was cancelled or not, you already can do it by explicitly checking deferred.isCancelled. However, I don't think the need to make this distinction happens very often.

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.

Please don't make that the recommended pattern. The point of CancellationException is that it's the only exception that safely bubbles up to the next checkpoint (be that launch or some try block with a custom subclass of CancellationException). This bubbling behavior is very important. The proposed pattern is bad because intermediate try-catch layers will believe there is an error instead of controlled backtracking. We use CancellationException as a signal for safe abortion in our codebase because that's the only exception which gets actively ignored by intermediate code layers.

The proper solution is to fix the APIs where possible and introduce new fixed APIs where backwards compatibility is needed. The ensureActive() workaround is imprecise and not a true causal relationship.

[revintec]

I was debugging something like this for two days(in a production environment, had to add many log/counter and deploy and wait a reproducer, do that again for at least 20 times, VERY bad experience), even suspected at one point that it was a compiler BUG...

// simplified reproducer, the original is more complicated
// thus it's even harder to debug such kind of control-flow issue
suspend fun structuredConcurrency(toBeProcessedData:Collection<Any>){
    coroutineScope{
        for(data in toBeProcessedData)
            launch{handleData(data)}
    }
    println("all data was processed")
}

the all data was processed got printed so I assume that all data was processed(seriously, is this code looks anything wrong for anybody?
but guess what, it's not, and NO EXCEPTIONS IS LOGGED! it looks like a mysterious break was in the for loop, the coroutine just vanishes... no return, no exception, how is that possible...

well, this is because deep down in the handleData(data) callstack, someone introduced withTimeout... a reasonable change, but blow up the whole thing :(

the point is, throwing CancellationException IS NOT THE SAME as cancelling a coroutine! ANYTHING CAN THROW ANY EXCEPTION, isn't that the point of structured concurrency? if we call something, it has to EITHER RETURN OR THROW, not vanishing without a trace...

Anyway I diverge, I agree with @elizarov "We can consider changing this logic to report that the second coroutine has failed with CancelletationException, too", and here's my advice:

1. when cancelling a coroutine, the automatically produced CancellationException should have a private mark on it / or the cancelling coroutine should have a private mark of the current propagating CancellationException
2. if the marked CancellationException propagated, and is in the correct structured concurrency way, keep the current behavior
3. if any other exception propagated, or if the afore mentioned CancellationException is not propagating in the correct structured concurrency way(for example the case mentioned in the original post, or the user grab that reference and throwing them in another coroutine etc), treat them as error(throw outwards and don't swallow them
4. please don't complicate things by introducing Result or un-subclassing TimeoutCancellationException from CancellationException(backwards incompatible and very hard to find in tons of catch(e:CancellationException){...}