New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CancellationException inheriting from IllegalStateException can be error prone #4073
Comments
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 That said, I'm also quite surprised that |
As far as I see, CancellationException is defined as:
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 |
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: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 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: #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.The text was updated successfully, but these errors were encountered: