Open kevincianfarini opened 4 months ago
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).
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 ?
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
.
Maybe this issue warrants a structured-concurrency label as well?
@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
What do we have now?
Currently,
CancellationException
inherits fromIllegalStateException
. My understanding is this was done to maintain compatibility with Java'sCancellationException
.What should be done instead?
Coroutines < 2.0.0
Coroutines >= 2.0.0
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.
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.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: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 likeerror
,check
andcheckNotNull
. 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 ofCancellationException
.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: https://github.com/Kotlin/kotlinx.coroutines/issues/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 catchesCancellationException
.The upsides of your proposal.
CancellationException
when they broadly catchException
orRuntimeException
. (This is not foolproof if they also catchThrowable
).IllegalStateException
versusCancellationException
. Throwing generic instances of ISE was made popular by Kotlin functions likecheck
andcheckNotNull
.CancellationException
which also suffers from this issue.The downsides of your proposal that you already see.
CancellationException
. This decision was originally made with intention, but I am not sure what practical benefits it provides us.