-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Provide a runCatching
that does not handle a CancellationException
but re-throws it instead.
#1814
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
Comments
runCatching
that does handle a CancellationException
but re-throws it instead.runCatching
that does not handle a CancellationException
but re-throws it instead.
Thanks for the proposal! Could you please elaborate why is it unexpected behaviour? E.g. why one may expect to use |
If the developer uses the StdLib |
@Anton-Spaans-Intrepid |
@fvasco Good question; maybe :-) @qwwdfsad
Running the above
This is not right. The If we change the code like this (replacing
this is printed out:
This is correct. The call to |
(looping in my personal github account: @streetsofboston) |
FWIW, the I also asked about a similar |
@Anton-Spaans-Intrepid Let's take a look at a more realistic example, please. What are the use-cases for |
@elizarov I also have this use case. My repository is private so I can't post the source, but just in general I've had to repeat the same pattern where I want to handle some exception in a suspending function somehow. For example I can log, or I can send over to a crash reporter. However, what if this exception is handling is a With |
Code snippet would be something like: suspend functionThatCanThrow() {
//some long running operation
}
suspend main() {
try {
}
catch(ex: Exception) {
if(ex is CancellationException) {
throw ex
} else {
LOG.errror(ex, "error calling function")
}
}
}
|
inline fun <reified T : Throwable> Result<*>.except(): Result<*> =
onFailure { if (it is T) throw it } It would allow you to: runCatching { mightThrowCancellationExceptionThatShouldNotBeCaught() }
.except<CancellationException>()
.onFailure { throwable /* Will never be of type CancellationException */ -> } My use case is that I materialize various errors from lower layers in an Android view model for its observers and I do some logging. This just so happens to be in a coroutine context (in suspend funs running in the view model coroutine scope), so this might throw |
Also, what's with OutOfMemoryErrors and similar? It's an antipattern (not sure whether it's disputed or not) to catch |
@vogthenn by that reasoning I wonder what the original use case is for |
@erikhuizinga https://github.com/Kotlin/KEEP/blob/master/proposals/stdlib/result.md Looks like intended use cases are postponing failure and internal within coroutines. |
It seems that the KEEP's use case 'Functional error handling' (specifically the example with The proposed idiomatic solution in that KEEP is to be explicit about any expected outcome: either always return a result (but don't throw), or return a nullable result (but don't throw), or materialise various outcomes, e.g. use sealed types of expected failures or results (but again don't throw). If applied ad nauseam, then the KEEP is right: you would only expect exceptions from the environment (e.g. out of memory) or from failure-heavy operations or libraries (e.g. Are the aforementioned use case and the proposed ways of handling errors in the function's signature contradictory? |
Using It'd be nice to be more explicit as to how to use exceptions. In the current state, APIs like PS: also, the fact that |
As noted in #1814 (comment) |
Indeed. I understand that Flow is in the coroutine world and |
Thank you Anton, really good suggestion. I have a question: how to deal with the TimeoutCancellationException? The following example fixes that behaviour: public suspend inline fun <R> runSuspendCatching(block: () -> R): Result<R> {
return try {
Result.success(block())
} catch (t: TimeoutCancellationException) {
Result.failure(t)
} catch (c: CancellationException) {
throw c
} catch (e: Throwable) {
Result.failure(e)
}
} What do you guys think about this? |
After Kotlin 1.7 underscore operator for type arguments, @erikhuizinga solution is even more appealing because you can use it that way: inline fun <reified T : Throwable, R> Result<R>.except(): Result<R> = onFailure { if (it is T) throw it }
// ...
.except<CancellationException, _>() the type of the item inside the result will be automatically inferred by the underscore, without the need to use |
We use The question is how can we add context information to the cancelation cause while the |
@Legion2 so how exactly do you wrap |
Yes we use it only for error reporting to have more glues where the exception originated from (the stack trace does not provide us with this information). We always rethrow them and now handle the cancelation exception specially and don't wrap it, because it is catched again by coroutine framework and converted into normal control flow again. So it does not make sense to alter cancelation exceptions. Btw. The fact that withTimeout function throws a cancellation exception is very bad API design. it only makes sense in the block which is cancelled, but the caller should receive another type of exception. Because the caller is not canceled if the callee times out. |
Any update on this ? Being able a catch a Why not instruct the compiler to automatically re-throw the CancellationException in a catch block surrounding a suspending function ? I can't see a decent usecase where this wouldn't be beneficial. To be honnest, I expected this behavior to be automatic since there's so many bytecode generated for coroutines. I expected the "magic CancellationException propagation" would be somewhat "protected" at the compilation level when generating the code behind coroutines. |
It always depends on the application, how one would handle different types of exceptions/errors. So there is no solution that fits them all. I think the best here is to create an excessive guide/docs on exception/error handing in kotlin, including coroutines and also examples from common kotlin libraries such as ktor. So users understand the available error handling features and limitations provided by kotlin and prevent miss use and bugs. |
@joffrey-bion There's nothing to do against On the "expected exceptions" part, as I said, I don't care if it's 400 / 500 / parsing error. I don't want my code to crash for something I don't care. That's quite "pedantic" of some API to say "your application should crash if you don't handle one the these terrible (undocumented) exceptions that has happened" (which is just an unreachable server because mobile devices are... mobile ?). The call just failed and that's alright for my usecase. Imagine I just want the temperature from some domotic HTTP device. Why should I care if it's 400 / 500 / parsing ? No need for extra boilerplate to handle an exception I don't see value in. That nightmare was some old Java stuff, it's 2024 now in Kotlin. We can do better. If it doesn't work, I don't get the result and I have a compile-time check (nullability / type safe) to ensure it. Much better. Much simpler. The whole Kotlin ecosystem is based on the nullability of a return to check if it succeeded or not instead of Exceptions. Roman wrote an entire article about it. Exception handling is still a miserable experience in Kotlin by design as Roman explained: use nullability, or sealed classes if you want fine-grained approach instead. Based on Kotlin paradigms, we can wrap callback based APIs (bad) to Coroutines (good). We can transform listeners ("bad") to Flows (good). But why can't we easily wrap exception based APIs (bad) to null / type-safe (good) ? You even say we shouldn't, which I don't understand why. Except for Ktor, I don't see any Exception we should try to catch from calling Kotlin libraries, and that's a huge improvement over Java in my eyes. |
You can't magically achieve good, Kotlin-idiomatic error-handling by wrapping an exception-based API into a EDIT: yep, see the section "Handling program logic errors" from the article you linked. cc @e5l: hi! In the messages above, @NinoDLC says that Ktor throws too many exceptions even when the Kotlin-idiomatic way would be not to fail but return something to let the programmer deal with it, forcing the user to wrap code in a |
So you can't have 0% crash, that's my point. Why did you suggest this as a goal at all?
Because all of those things require a (different) bugfix (and I didn't say 400 initially, but yeah 400 needs a bugfix too, while 404 may or may not). In your first paragraph, you seem to be ok with crashes if they result from developer mistakes (which I agree with!). Then why would these cases not count? In the same vein, why would you catch
Nobody is saying this. My point is that exception handling allows to catch different types of errors at different levels. I certainly don't want the app to crash, and that's why you can add higher-level / more generic error handling at different levels thanks to exceptions. I also certainly don't want a low-level HTTP call to return There are cases for catching
Using
I think you have misunderstood something in the article you linked. Let me quote it: "As a rule of thumb, you should not be catching exceptions in general Kotlin code. That’s a code smell. Exceptions should be handled by some top-level framework code of your application to alert developers of the bugs in the code and to restart your application or its affected operation. That’s the primary purpose of exceptions in Kotlin." This is almost the opposite of "you should catch everything and return null instead".
Do you have an example exception-based API that you consider bad? |
AFAIK, Ktor doesn't validate responses by default, so you're free to check exit codes yourself. If you want it to fail on error response codes, you have to actively opt-in with Or do you mean more hardcore exceptions like IO errors / connection closed? |
The arrow-kt library differentiates among
Is it totally incorrect? |
OFF:
I agree but currently there are lots of issues caused by the "fundamental design" of the cancellation implementation. There are related problems/issues/questions (like this one) showing that even advanced users are sometimes confused how to correctly handle errors. And if you are a very advanced user (or even a contributor), please think with the head of less advanced ones ;) Another huge problem is that it seems that the coroutines library does not seem to fix the fundamental issues because of backwards compatibility, which is a bad decision IMHO. Why not release a 2.0 version with enhanced APIs, especially in the area of error handling?
It would be awesome! Especially if this documentation would contain the known issues and their "workarounds" as well. |
@dkhalanskyjb and @joffrey-bion You completely missunderstand my points almost like a strawman, I'm only talking about environment-induced exceptions, not logical (programming) errors or even Between: I, personnally, as an application developper, choose 1. Obviously, for some low level or high complexity codebases, at one point, you need solution 2/. I understand that. But the choice should rely on the developper.
If I don't catch everything comming from Ktor, my end-user application will crash because their phone was on the subway and couldn't reach the server, triggering an IOException. What do you propose then ? Once again, I'm only speaking about "exceptions that aren't" in the sense they're not "exceptions", in the end they are merely expected possible failures because, once again, a mobile device is mobile and its internet connection is unstable.
Ktor yes. It feels not Kotlin at all in its call APIs. Why can't I have a simple Like the nullability feature in Java (the "billion dollar mistake"), I feel like Kotlin, because of interoperability with Java, didn't "fix" the exception problem and to this day, even in Kotlin, we still use exceptions to drive our code flux instead of type safe alternatives. |
@NinoDLC although I personally don't like the idea of
I agree with this but the root of Kotlin's success was Java compatibility, so we have to live with it. |
Totally agree, let's not forget our heritage! But in my opinion there should be APIs in Kotlin to handle this problem, like nullability, reified generics, smartcasting and other has been handled by the Kotlin compiler to provide a better coding experience when needed / possible. Moreover, Ktor should provide a "kotlin way" to use its APIs. This is the only lib I have to try/catch. |
Sorry if that's the case, I'm genuinely trying. I'm literally quoting your own sentences without paraphrasing when I reply, to avoid misunderstandings. On the other hand, it seems you really want to read "let the app crash" when I write "catch at a higher level", so it looks like you're making a strawman out of my arguments. Let's try to stick with what we're actually writing, so we don't put words in each other's mouths, because I agree this doesn't feel nice.
I totally agree with this, but then you prone catching everything including programming errors, which is a contradiction (or a tradeoff, but you didn't present it as a tradeoff). In your option 1, you don't mention it but you're catching all of these too. Also, you write type safety / no type safety in the options, but returning Also, those are definitely not the only 2 options. You don't seem to account for my suggestion anywhere: return null for what makes sense to be represented as null, let programmer errors bubble up (0 boilerplate), don't catch generic exceptions at a low level, let them bubble up but catch them at a higher level in your app to avoid app crashes and report them accordingly (in framework/architecture code, not business code, because we don't want this boilerplate there, I agree).
Catch the Also, feel free to use extensions. I don't think it's valid to blame the library API design for your decision to use
(NB: I'm assuming you mean I'm not sure about the point you're making here. These functions don't claim to never throw, they only claim to return null in specific documented cases. The first 2 return null when the input is not valid, the 3rd one returns null when the index is out of bound. Also, |
I didn't mean to strawman you, but I really don't understand... Whatever the level where I try/catch, I will have to Say, by chance, experience or reading 30 pages Ktor learn-to-do-everything guide, we know an All this miserable, die-and-retry "Java" experience is exactly why I wanted something else to be certain not to crash when I do a For now, this something else is use a terrible " But I had a miserable experience getting there, and every developper will get a miserable experience (let's face it, a compilation or IDE error is worth 1000 times reading the documentation), either because : And even with the Ktor problem aside, I expected a more "straightforward" way to handle Exceptions in Kotlin. Even more when its core "threading" APIs use
Once again, how can I know what I need to catch ? There's no checked exception in Kotlin (and there's |
Take a look at: Working with typed errors You can wrap the Ktor APIs with your own APIs, and have "checked exceptions" in Kotlin, even better than Java (with all the advantages and disadvantages of them). |
There are several options to wrap existing APIs according to your taste, like the
It is a legacy of Java, and arises from the non-pure-functional paradigm of Kotlin. We simply cannot do anything with it. But this discussion has gone slightly offtopic, maybe we should focus to |
It seems that there really should be a "coroutine-purposed" version of It's not immediately obvious to newcomers to coroutines that It seems like implementing a public inline fun <T, R> T.cooperativeCatch(block: T.() -> R): Result<R> {
return try {
Result.success(block())
} catch (e: CancellationException) {
throw e
} catch (e: Throwable) {
Result.failure(e)
}
} |
@mattshoe, #1814 (comment) |
@dkhalanskyjb yea, this discussion looks like it's been going on for some time. It's become quite unwieldy and pretty hard to follow structurally |
imo I've modified it to rethrow suspend fun <T, R> T.safeResult(
dispatcher: CoroutineDispatcher,
block: T.() -> R
): Result<R> = withContext(dispatcher) {
try {
Result.success(block())
} catch (e: Throwable) {
// Should not catch CancellationException
coroutineContext.ensureActive()
Result.failure(e)
}
} PS - if you want that block should also be a suspend function then do Code from the repo: https://github.com/ksharma-xyz/Krail/blob/main/core/coroutines-ext/src/main/kotlin/xyz/ksharma/krail/coroutines/ext/CoroutinesExt.kt#L21 |
Problem Description
The Kotlin StdLib contains a function named
runCatching
. It tries the provided lambda and catches anyThrowable
that is not caught or thrown by the lambda.However, using
runCatching
in Coroutines can have a somewhat hidden (and unexpected) side-effect of it catchingCancellationException
s and not re-throwing them. This could interfere with the Coroutines' Structured Concurrency, with its cancellation handling.Possible Solution
E.g. create a new function in kotlinx.coroutines that looks something like this
(not sure about the name 😄):
The
runSuspendCatching
will catch any exception and wrap it in aResult
, except forCancellationException
s that will be re-thrown instead. This will prevent unwanted interference with the cancellation handling of Coroutines.Notes
suspend
keyword is added to prevent this new function from being called in regular non-suspending/blocking code.runCatching
in a Coroutine/suspendable-context, a lint warning should warn the developer that handling cancellations may be compromised andrunSuspendCatching
should be used instead.The text was updated successfully, but these errors were encountered: