https://kotlinlang.org logo
Join Slack
Powered by
Hey ! I want to ask people around if `runCatching...
# coroutines
n
Hey ! I want to ask people around if 
runCatching
is in use in any of your Kotlin (or Android) projects ? I feel my team is abusing it and I'm trying to understand if there's any "pros" about using it in any coroutine-based codebase. My team just adopted this convention and I'm at loss of words. (Examples in thread for anyone interested)
Basically, new suspending code would look like it :
Copy code
class FetchFooUseCase @Inject constructor(
    private val synchronizeFooUseCase: SynchronizeFooUseCase,
    private val fooRepository: FooRepository,
) {

    suspend fun invoke(fooId: String): Result<ShowEntity> = runCatching {
        runCatching { fooRepository.getFooById(fooId) }.getOrNull()
            ?: (synchronizeFooUseCase.invoke(fooId) as RefreshFooFromServerResult.Success).foo
    }
}
which is used by :
Copy code
class GetBarsByFooIdUseCase @Inject constructor(
    private val fetchFooUseCase: FetchFooUseCase,
) {

    suspend fun invoke(fooId: String): Result<List<BarAggregate>> =
        runCatching {
            fetchFooUseCase.invoke(fooId).getOrThrow()
                .mapNotNull { runCatching { createBarAggregate(it) }.getOrNull() }
        }
}
Another example with Flows :
Copy code
class GetFooUpdatesUseCase @Inject constructor(
    private val fooDownloadStatusRepository: FooDownloadStatusRepository,
    private val fooFavoriteRepository: FooFavoriteRepository,
    private val fooProgressRepository: FooProgressRepository,
) {

    fun invoke(): Flow<FooUpdate> = merge(
        runCatching { fooFavoriteRepository.getFavoriteFooUpdates() }.getOrDefault(emptyFlow()),
        runCatching { fooProgressRepository.getProgressUpdates() }.getOrDefault(emptyFlow()),
        runCatching { fooDownloadStatusRepository.getDownloadStatusUpdates() }.getOrDefault(emptyFlow()),
    )
}
j
Here is what I think: https://stackoverflow.com/a/70847760/1540818 IMO, there is only a tiny fraction of use cases where 
runCatching
could be useful, and no business code should use it.
OMG this code is horrendous to read to be frank. I would love to see arguments in favor of this. It's also wrong because in suspending contexts this will catch 
CancellationException
, which would be incorrect 99% of the time
s
Yup, it's a 'bit' abused and you run the risk of catching and not re-throwing CancellationExceptions, breaking the structured concurrency model of coroutines....
n
Yes I shared this very interesting and thoughtful answer with my team (along with the excellent article by Roman), but they just dismissed it because "Roman has a background of backend developper so his thoughts won't apply to Android" [sic]
s
If they want to use railway-oriented programming, I suggest using Arrow's 
Either
class instead, where this is made easier and works well within the structured concurrency model of coroutines
n
Yes that was my point too, either with sealed classes (which we used before), or 
Either
obviously
s
Still, just ignoring exceptions by returning null or empty lists, etc, not properly modeling your errors is not a good way to go.
n
But their counter argument was "Domain should be safe" [sic]
j
Just because Android has a UI doesn't mean all exceptions should be caught at the lowest level. The mistake most people make here is that they confuse "use exceptions bubbling" with "don't handle exceptions". The point is that exceptions should be handled at a high-enough level so user actions can be met with user-level errors.
s
'Safe', as in , there won't be any exceptions?
n
I don't know to be honest, at that point I didn't even know what to say anymore 😂
j
Yeah I'd be eager to know how much safer they think the nested 
runCatching
makes things in the first example
s
Oh... you won't get any crashes from exceptions thrown from your domain, but you'll be in for for lot of surprises higher up the domain chain 🙂
j
Yeah this doesn't make any sense. You can prevent hard crashes at the highest level if you don't want that (and that makes sense), but turning every error into a null or something like that is just begging for subtle invisible bugs. Good luck tracking the source of the problems
n
Wrapping everything in 
runCatching
makes me afraid of "blank pages" where if you have any issue, instead of crashing (which is automatically reported back so we have metrics), you just have a non-functional UI (which is wayyy harder to detect).
s
@Nino I prefer sum-types separating good-results and errors (composition using sum-types like Either, for example) over combining them into one hierarchy (unless it's the UI-state you're modelling, where you fold both errors and good-data into one UI).
c
Same message as always when someone mentions 
runCatching
: it is NOT safe to use with coroutines. runCatching catches Throwable, which includes CancellationException. This stops coroutines from dying correctly: if a coroutine is cancelled, it will still be considered alive by its parent, leading to memory exhaustion and an eventuaal live-lock.
j
It is not safe to use without coroutines either 😆
c
If you want railroad-oriented programming, use Arrow's Either.catch, which handles CancellationException correctly (and also has an amazing DSL to make it nice to read).
n
unless it's the UI-state you're modelling, where you fold both errors and good-data into one UI
Yes that was that strategy we used before: • either a nullable type is returned if we didn't care about the error type (it just "failed", so 
null
is returned), so we would have a generic error message displayed in the view • either a sealed class providing the result or the error (to be able to differentiate between "NotFound" and "NoInternet" errors for example)
They wanted to use a "suspendRunCatching" function re-throwing the 
CancellationException
but it still seemed like a bad idea. Any thoughts ?
j
The guideline to use 
runCatching
all over the place, specifically using 
runCatching
at different levels in the code (effectively nesting them, locally or less locally), shows laziness on the part of the devs IMO. It shows that instead of reflecting on the proper place to handle and report errors, they want to "feel" safe and hide errors. This doesn't work in the long run.
c
@Nino it's very bad practice to catch all exceptions, no matter how you do it.
s
That would be a bit better, but still as bad as wrapping everything in try-catch and then basically 'ignoring' the catch by always returning null/empty. Errors should be modeled, even if it is just a simple model.
c
Also, you shouldn't always rethrow CancellationException. There are some cases in which they are genuine errors.
s
^^^ Doesn't happen often, but yes!
n
Interesting, I didn't know that! Do you have any usecase in mind where rethrowing CancellationException is a mistake ?
s
A Java lib could throw a CancellationException...
One that is an error, and shouldn't really cancel the coroutines...
@Joffrey Subclasses of CancellationExceptions are fine... they are still CancellationExceptions.
n
A java lib that knows and throws 
kotlin.coroutines.cancellation.CancellationException
? That's weird
s
There are java ones as well
j
Sorry I was too late to delete the message. This example was not the correct one
p
Copy code
public actual typealias CancellationException = java.util.concurrent.CancellationException
it’s just Java’s under the hood in the JVM
s
Hi Peter!
c
Well, "mistake" depends on what your objective is. Some channel functions return CancellationException when there are no elements. If you rethrow them, they'll invisibly cancel the calling coroutine, and you'll never get an error message.
n
Ok you just blew my mind Peter lol I would never expect this typealias to be done! Is there any reason for that ? It seems dangerous for coroutines to rely on it. Like any "lower java api" could unexpectedly cancel the coroutine 😮
s
Overall, CancellationExceptions should be re-thrown for coroutines. Occasionally, alas, a method throws such an exception incorrectly (they should've used another type of exception).... Exceptions always have been implemented/used terribly on Java.... 😕
c
j
So actually let me come back to the timeout example. The reason why I thought of 
TimeoutCancellationException
(subclass of 
CancellationException
) is that in this case, the exception is an actual error (it expresses a timeout), and it's not meant to cancel the current coroutine. So if the purpose of the over-confident 
runCatching
is to catch "errors", this timeout should be one of them. Just re-throwing it kinda defeats the purpose (NOT of 
withTimeout
, but of 
runCatching
in this case)
s
I read that article, and i'm not sure I agree with it entirely 🙂 Wonder what Roman would think about this article 🙂
j
Roman actually thinks this article is on point 😉 (from what I could tell)
s
Nice! Did he comment somewhere about it?
j
Yes, internally at JetBrains
But there are also some public issues about the fact that 
withTimeout
and channel functions (the one mentioned in the article you linked) throw subclasses of 
CancellationExceptions
in situations where the caller of the function is not cancelled
Those exceptions are not meant (or should not be meant) to cancel the coroutine of the caller. They show an actual error in the call, which is related in some way with the cancellation of some other coroutine. In these cases, I bet your colleagues pushing for 
runCatching
would actually want to catch those
Roman actually thinks this article is on point 😉 (from what I could tell)
Btw, I don't want to put words in your mouth @elizarov so please correct me if I'm wrong in saying this
o
OMG this code is horrendous to read to be frank. I would love to see arguments in favor of this. It's also wrong because in suspending contexts this will catch 
CancellationException
, which would be incorrect 99% of the time (edited)
FWIW - I have used these wrappers for properly handling CancellationExceptions...
Copy code
/**
 * Executes [block] and returns a [Result]. All exceptions thrown are
 * returned as a [Result.Failure] except a [kotlinx.coroutines.CancellationException]
 * which will be rethrown.
 *
 * See [Throwable.isCancellation] for more info.
 */
inline fun <T> runCatchingIgnoreCancelled(block: () -> T): Result<T> = runCatching(block)
  .onFailure { error ->
    if (error.isCancellation()) {
      throw error
    }
  }
Copy code
/**
 * Extension on a [Throwable] that returns true if it is of type [CancellationException]. Typically,
 * this type of Exception should _not_ be caught and should be bubbled up. Coroutines internally
 * uses [CancellationException] for cancelling of work, and they are ignored by all handlers by default.
 *
 * See [cancellation-and-exceptions](<https://kotlinlang.org/docs/exception-handling.html#cancellation-and-exceptions>)
 */
fun Throwable.isCancellation() = this is CancellationException && this !is TimeoutCancellationException
This was mainly useful in UseCase/domain layer, where the consumer/presentation layer didn't care about the lower level error or why something failed, but only if there was a result or not....
j
A note about this special case for 
TimeoutCancellationException
. Imagine your use case layer `await`s on an 
Deferred
produced by some 
async
in a different coroutine scope. If that other coroutine scope is cancelled (NOT the one of your use case doing the 
await()
), the 
await
will throw 
CancellationException
- which is just another error in this case, representing the absence of value. In this case you will re-throw and it will not go through your classic error handling that you intended to hook via 
runCatching
. Same goes for channels `send`/`receive`. Cancelling some other coroutine at the other end of the communication channel shouldn't really be considered like other cancellations by your code, but rather like an error (most of the time). See https://betterprogramming.pub/the-silent-killer-thats-crashing-your-coroutines-9171d1e8f79b?gi=b4555a1271e9 for more details about these "weird" `CancellationException`s.
This was mainly useful in UseCase/domain layer, where the consumer/presentation layer didn't care about the lower level error or why something failed, but only if there was a result or not
So you mean you decided on purpose that you don't want to have log reports of 
ClassNotFoundException
, 
ThreadDeath
, 
NoSuchMethodError
, 
StackOverflowError
, 
ArithmeticException
, 
IllegalArgumentException
etc. but you just want to swallow those errors and show an empty screen? What do you do in the upper layer with the failure case of those results?
o
If there's specific errors that you want to handle/map or do logging, you still can with chaining 
fold
or more logic in your 
onFailure
hook. It's up to you... I am simplifying some stuff here just to not derail the convo from what (I think?) was mainly the danger of not bubbling up 
CancellationException
properly... But quick/short answer for you, yes, in these cases, the UI required all data to be present, or none, and in the case of any error, there was specific UI shown for retrying, continuing and trying later, or contact info for more help etc... hope that is a bit more clear 🙂
t
This can be misused just like try/catch can be misused, doesn’t mean it’s wrong to use it as it takes the boilerplate out of things like rethrowing cancellation exceptions which many people do from try/catches anyway. normal 
runCatching
doesn’t do anything that try/catch doesn’t do except provide some syntactic sugar to make things like exception recovery and chaining events easier.
The way I see this best used is for cases where you want to put a general catch in to recover from an exception you didn’t expect. Generally I use this pattern for cases where “hey I don’t think I’ll have an exception here, but just in case I want to make sure it doesn’t crash the app and gets logged”. Cancellation exception generally in those cases is the one exception bubble up past because it still fits the pattern of “don’t crash the app” This is generally useful since just because you don’t think an exception will happen doesn’t mean you shouldn’t prepare for one. We generally never think crashes will happen, if we didn’t then we would have covered the case
c
@Thad It does one thing very differently: 
try-catch
is bad if you write 
catch (e: Throwable)
. 
runCatching
is always bad.
If you were forced to specify which exception you wanted to catch, 
runCatching
would be fine.
t
My case above cites a very specific case where it is not bad so I can say without doubt that your statement is not correct, it’s not always bad
you are not forced to specify what kind of exception you want to catch with try/catch, you can just specify all of them
j
I am simplifying some stuff here just to not derail the convo from what (I think?) was mainly the danger of not bubbling up CancellationException properly
Actually the point about cancellation exception is the side track. The main point of the question is whether using 
runCatching
everywhere is a sane idea.
c
In your case it is indeed fine to catch 
Throwable
. In my opinion it is not a sufficient use case to add a standard library method that breaks coroutines without saying anything in its documentation, tempting people to use it everywhere (and this thread has shown that people do, in fact, use it everywhere).
j
“hey I don’t think I’ll have an exception here, but just in case I want to make sure it doesn’t crash the app and gets logged”
Could you please elaborate on what you intend to do in the 
catch
(or 
onFailure
) in this case? Crashing the app does get logged, trying to recover from any 
Throwable
is wrong
c
IMO the rule should be: never use 
runCatching
. If you have a legitimate use case to catch Throwable, then do so explicitly.
t
You can have that opinion, I’m going to continue using it when appropriate
j
runCatching doesn’t do anything that try/catch doesn’t do except provide some syntactic sugar to make things like exception recovery and chaining events easier.
It hides a catch of 
Throwable
which is almost never a good idea to catch, unless you're writing a framework that runs user-provided code. It also adds boilerplate on the access of the successful value in the happy path.
o
Actually the point about cancellation exception is the side track. The main point of the question is whether using runCatching everywhere is a sane idea.
Gotcha, I guess my 2cents here is a definite no, it's not a rule/style/convention that should be used entirely in a coroutine based codebase. Only in places where it makes sense. (Like the above examples just shared potentially)
t
Could you please elaborate on what you intend to do in the 
catch
(or 
onFailure
) in this case? Crashing the app does get logged, trying to recover from any 
Throwable
is wrong
I intend to show an error and log it instead of having the user force exit the app, it’s a pretty crappy user experience the other way
c
The only place where it does make sense is at the very top of the code hierarchy (e.g. the main method, or in your exception handler if you do UI), where any exception whatsoever means everything has gone extremely wrong. It's a mistake to use anywhere else.
But even then, your UI library probably already has it somewhere, so it shouldn't even appear in your code.
j
I intend to show an error and log it instead of having the user force exit the app, it’s a pretty crappy user experience the other way
Then this should be at the top of the hierarchy, not deep in the business code
n
Yes, for example, if you catch a Throwable in Android, it could be for a OutOfMemoryError, which mean you're silencing the root cause of the OOM (and the next code that is fine but still need some memory would crash instead)
j
Also, your JVM is in an undefined state at this point. So if you try to keep going without crashing the app, some obscure things could happen
t
ok so agree it shouldn’t catch 
Throwable
preferably but instead 
Exception
that’s fair
c
Nope, same issue with 
Exception
.
You should only catch specific subclasses when you know they can happen (e.g. `IOException`…)
j
At least with 
Exception
you don't have the hard JVM issues, but still it's not advised to "catch everything". But in any case 
runCatching
catches 
Throwable
, so it settles the question about using 
runCatching
in many places in business code
c
For example: it's probably not a good idea to catch 
IllegalArgumentException
. If you ever get one, it means your code is wrong. There's no recovering from that. Also, 
CancellationException
is an 
Exception
, so you're still breaking coroutines.
t
How does that cover the case of “I don’t expect any errors”. By definition if I don’t think any exceptions can happen I don’t know what ones do
j
If you don't know, what makes you think you know how to recover?
c
You should only have a "catch everything" code at a single place at the very start of the program (ideally the 
main
method or immediately after that). Everywhere else in your codebase, you should not catch stuff you're not aware of.
t
Not if you do the 
runCatchingIgnoreCanceled
trick @OG posted earlier. But it’s up to the consumer to do the right things with the errors they are catching
Ok I still prefer to not crash the app when I could just post an error, but honestly this I think has devolved to an opinion argument and you are entitled to your opinion. It is fine for it to be in the standard lib and you just don’t have to use is. 
!!
is a kotlin feature that really should never (or rarely) be used but I don’t think I’ve ever heard someone argue it shouldn’t exist just because they don’t think it’s used often. If you don’t like it you don’t have to use it. I’ll continue using it safely (though I might make a new version of it to cover the Throwable case you accurately pointed out, I forgot it caught more than exception)
n
!!
helps migrating code from Java 😄
j
!! is a kotlin feature that really should never (or rarely) be used but I don’t think I’ve ever heard someone argue it shouldn’t exist just because they don’t think it’s used often
I think this was added for pragmatism, but I also believe no reasonable application should use it. It's always superior to use 
?: error("reason why you believe it's never null")
. Except in code golf.
c
!!
is an explicit mark for the reviewer that they should pay attention to that piece of the code. Most of use are taught in school to be very careful around 
catch (e: Throwable)
, for good reason. Most of us don't realize how dangerous 
runCatching
is, which is its main issue.
j
Ok I still prefer to not crash the app when I could just post an error
We don't disagree on this. What we're saying is that you should prevent the app from crashing in a central place, show the error popup, and then still exit. For this, you don't need 
runCatching
nor 
catch(Throwable)
anywhere in the business code, just in one place close to the entrypoint of your program.
o
Whoo.. this thread is moving miles a minute from when I last checked... If we put aside the runCatching vs traditional try/catch for a moment, In a world where you're using just try/catch to catch specific errors when calling suspend functions, (never catching Throwable or Exception), should you then always explicitly check and catch CancellationException (Since CancellationException does have special meaning in coroutines world) and handle them depending on your case (rethrow or possibly swallow)?
Or just never catch it and let it always bubble up?
n
The other way around I'd say. Catch the kind of Exceptions you want to catch, not just 
Exception
o
Reworded my message just to make sure it's a bit more clear what I mean
j
I just leave 
CancellationException
be in general. If I'm catching specific exceptions, 99.9% of the time this doesn't include 
CancellationException
, so it's fine to not care about it. If you do write a catch-all block somewhere, it's usually high-enough in the hierarchy and everything will be rethrown anyway in the 
catch
.
c
W.r.t. CancellationException: never catch it and let it bubble up, except when you're calling functions which can throw CancellationException in another case than when you're cancelled (e.g. withTimeout), in which case it's fine to catch it but you MUST check if you're cancelled or not immediately using 
ensureActive
.
E.g. this is ok:
Copy code
try {
    withTimeout(5000) {
        someNetworkRequest().toList()
    }
} catch (e: TimeoutCancellationException) { // I'm catching a subclass, so if I'm cancelled it will bubble up without creating zombies
    // Here, I know that the timeout failed and it's safe to recover however I want
    return emptyList()
}
o
Yup I figured TimeoutCancellationException would be different here yeah
j
I would say in the specific case of 
withTimeout
I would just use 
withTimeoutOrNull
and check for null. It is simply safer because you can be sure that it's your own timeout in that case. Otherwise you can have problems with nested 
withTimeout
n
Yes
Copy code
withTimeoutOrNull(5000) {
    someNetworkRequest().toList()
} ?: emptyList()
c
This creates zombies:
Copy code
try {}
catch (e: CancellationException) {}
This is fine, but less readable (and a bit slower) than catching TimeoutCancellationException
Copy code
try {}
catch (e: CancellationException) {
    ensureActive()

    // your stuff
}
n
null
was a nightmare in Java, 
null
a strength in Kotlin
c
Even in the case of 
withTimeout
, where it would be safe to catch 
CancellationException
, there's a nicer way to do things 😅 So just always find a way never to touch 
CancellationException
, everything will be easier this way
o
Tldr • runCatching is equivalent to try/catch with Throwable // dangerous! • Better to use try/catch and handle specific errors you care about • Typically you don't need to handle catching CancellationException, the exception is TimeoutCancellationException, but using withTimeoutOrNull is a better option
Did I get that right?
j
I think the part about timeout is a bit unclear. I would rephrase to: • 
runCatching
is equivalent to try/catch with Throwable // dangerous! • Better to use `try`/`catch` and handle specific errors you care about • Typically you don't need to handle catching 
CancellationException
if you follow the previous point • An exception to the previous point is catching 
TimeoutCancellationException
when using 
withTimeout
, but in that case using 
withTimeoutOrNull
is a better option And I would add that catching 
Exception
(or even 
Throwable
) can be useful in a very top-level place in order to avoid crashes and allow to terminate the application more gracefully. Catching 
Exception
can also be useful if you want to rethrow a more high level exception with the original one as cause.
o
I like the rewording there on TimeoutCancellationException. Thanks!
a
Very informative thread. If I get the discussion right, exceptions should bubble up to the top, e.g UI? Meaning your 
data
or 
domain
layer should not contain any error catching, let them throw, and you handle them in the top most level e.g the 
ViewModel
?
c
And *general error catching. If you call a function that can throw exceptions, it's fine to catch that specific kind of exceptions there. E.g. if you handle files, you can catch IOException, if you call Ktor functions you can catch NetworkException, etc. But only the very top of the app is allowed to catch everything
Your UI framework probably lets you register a global event error handler that does this for you, though.
As an aside, for the real reason runCatching exists: my understanding is it's useful to temporarily represent exceptions as data in order to rethrow them somewhere else, for example when converting synchronous functions into asynchronous ones. It's meant to encapsulate errors temporarily so you can rethrow them somewhere else, not to absorb them. That's why I think it should have stayed an internal detail of coroutines and not public API.
1455 Views
Open in Slack
PreviousNext
Powered by