CancellationException inheriting from IllegalStateException can be error prone

[kevincianfarini]

What do we have now?

Currently, CancellationException inherits from IllegalStateException. My understanding is this was done to maintain compatibility with Java's CancellationException.

What should be done instead?

Coroutines < 2.0.0

@Deprecated("Use CancellationSignal") 
open class CancellationException : IllegalStateException

class CancellationSignal : CancellationException()

Coroutines >= 2.0.0

class CancellationSignal : Throwable()

Why?

Over my several years using kotlinx-coroutines, I've noticed that this is something that regularly tricks people and unintentionally breaks coroutine cancellation. Most recently I've run into this while reviewing code that handles our authentication path. During authentication, if the server returns a token that is not valid for whatever reason (in this case the schema value is nullable, but this value should never actually be null) we throw an ISE.

suspend fun authenticate(): Token {
  val response: AuthenticationResponse = TODO("Do the server authentication")
  return Token(
    jwt = checkNotNull(response.jwt) { "Response JWT was null!" }
  )
}

There's very few instances where you'd actually want to catch IllegalStateException, but the one we found ourselves in was ensuring that a user was not in a partially logged in state if acquiring their token failed. In a sense, authentication had to be transactional. We have other services we "authenticate" with aside from acquiring a JWT. Below is roughly analogous to the code I reviewed.

suspend fun login(email: String, password: String) {
  try {
    service.authenticate(...)
  } catch (e: CustomNetworkError) {
    rollbackLoginTransaction()
    showErrorMessage()
  } catch (e: IllegalStateException) {
    rollbackLoginTransaction()
    showErrorMessage()
  }
}

There's a subtle bug here. My other coworker, who is a senior engineer, did not realize that CancellationException inherited from ISE. After pointing this out, he added the following line:

suspend fun login(email: String, password: String) {
  try {
    service.authenticate(...)
  } catch (e: CustomNetworkError) {
    rollbackLoginTransaction()
    showErrorMessage()
  } catch (e: IllegalStateException) {
    rollbackLoginTransaction()
+  if (e is CancellationException) throw e
    showErrorMessage()
  }
}

I've run into issues like this one fairly frequently in code review. This issue is particularly prevalent with more junior engineers who catch broad Exception types; that issue is more easily recognizable and mitigated though. Catching ISE is a more subtle failure that many senior engineers don't even realize is an issue on our team. This is further complicated by Kotlin's offering of utilities like error, check and checkNotNull. Encountering generic instances of ISE in Kotlin is fairly common, and most instances where you might need to catch ISE can be error prone because of CancellationException.

My intention is to raise this issue as a consideration for the next major release of kotlinx-coroutines. Lots of people have raised error prone code that accidentally catches CancellationException (see: #1814). While we can't solve every single error scenario here while also using exceptions to propagate cancellation, I believe that we should consider reducing the likelihood that someone accidentally catches CancellationException.

The upsides of your proposal.

1. It helps mitigate inexperienced engineers from accidentally catching CancellationException when they broadly catch Exception or RuntimeException. (This is not foolproof if they also catch Throwable).
2. It helps differentiate between catching an IllegalStateException versus CancellationException. Throwing generic instances of ISE was made popular by Kotlin functions like check and checkNotNull.
3. It decouples this library from Java's CancellationException which also suffers from this issue.

The downsides of your proposal that you already see.

1. This would be a breaking change.
2. We would decouple ourselves from Java's CancellationException. This decision was originally made with intention, but I am not sure what practical benefits it provides us.

[joffrey-bion]

I am not really commenting on the proposal here (I'm definitely not against it), but I strongly disagree with the main example provided, and thus the listed upside 2.

You should most likely never catch IllegalStateException (I have yet to see a case where this is beneficial, and this example is not one of them). If you need to catch ISE because you know of the specific situation where it is thrown (like the checkNotNull for your token), I would 100% vote for using a dedicated exception type there and catch this one specifically. It is both more readable and more correct, because the intent is to catch this specific instance of ISE, not any ISE, so we can use the type system to express this.

That said, I'm also quite surprised that CancellationException is a subtype of IllegalStateException (I did not know that, and I have quite a bit of experience with coroutines).

[AlexTrotsenko]

As far as I see, CancellationException is defined as:

public actual typealias CancellationException = java.util.concurrent.CancellationException

So perhaps the plan was to deal with Java's java.util.concurrent.CancellationException as if it was CancellationException thrown from coroutines ?

[dkhalanskyjb]

Note that this is a change not just in the library, but in the language as well. The exception is defined in the standard library https://github.com/JetBrains/kotlin/blob/0938b46726b9c6938df309098316ce741815bb55/libraries/stdlib/src/kotlin/coroutines/cancellation/CancellationExceptionH.kt#L13, and the compiler notifies the Apple targets that it is fine for suspend functions to throw this CancellationException.

[LouisCAD]

Maybe this issue warrants a structured-concurrency label as well?

[qwwdfsad]

@LouisCAD structured-concurrency is for a specific (backwards-compatible) project that aims to solve all (most) of the labeled issues with a dedicated API layer. This one, unfortunately, is much more profound