Skip to content
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

Provide a runCatching that does not handle a CancellationException but re-throws it instead. #1814

Open
Anton-Spaans-Intrepid opened this issue Feb 18, 2020 · 71 comments

Comments

@Anton-Spaans-Intrepid
Copy link

Problem Description

The Kotlin StdLib contains a function named runCatching. It tries the provided lambda and catches any Throwable 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 catching CancellationExceptions 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 😄):

public inline suspend fun <R> runSuspendCatching(block: () -> R): Result<R> {
    return try {
        Result.success(block())
    } catch(c: CancellationException) {
        throw c
    } catch (e: Throwable) {
        Result.failure(e)
    }
}

The runSuspendCatching will catch any exception and wrap it in a Result, except for CancellationExceptions that will be re-thrown instead. This will prevent unwanted interference with the cancellation handling of Coroutines.

Notes

  • The suspend keyword is added to prevent this new function from being called in regular non-suspending/blocking code.
  • When code calls the StdLib's runCatching in a Coroutine/suspendable-context, a lint warning should warn the developer that handling cancellations may be compromised and runSuspendCatching should be used instead.
@Anton-Spaans-Intrepid Anton-Spaans-Intrepid changed the title Provide a runCatching that does handle a CancellationException but re-throws it instead. Provide a runCatching that does not handle a CancellationException but re-throws it instead. Feb 18, 2020
@qwwdfsad
Copy link
Collaborator

Thanks for the proposal!

Could you please elaborate why is it unexpected behaviour? E.g. why one may expect to use runCatching that actually rethrows some of the exceptions? What are the use-cases and why runSuspendCatching is preferred over something more conventional?

@Anton-Spaans-Intrepid
Copy link
Author

@qwwdfsad

If the developer uses the StdLib runCatching { ... } and the runCatching wraps a suspendable piece of code (i.e. it could throw a CancellationException), the runCatching should re-throw any CancellationException. If not the suspendable caller of runCatching may not get cancelled and the Structured Concurrency of Coroutines breaks down.

@fvasco
Copy link
Contributor

fvasco commented Feb 19, 2020

@Anton-Spaans-Intrepid
Do you think that the current runCathing function should rethrow any InterruptedException?

@Anton-Spaans-Intrepid
Copy link
Author

Anton-Spaans-Intrepid commented Feb 19, 2020

@fvasco Good question; maybe :-)
But this is about CancellationExceptions when using runCatching in a Coroutine.

@qwwdfsad
Here is an example of where using runSuspendCatching can fix the cancellation-handling issue caused by runCatching:

fun main() {
    val screenScope = CoroutineScope(Dispatchers.IO)

    println("Start")

    screenScope.launch {
        println("Launched")

        val result = runCatching {
            delay(1000)
            4
        }.getOrElse { 0 }

        writeToDatabase(result)
    }

    Thread.sleep(500)

    // User leaves the screen.
    screenScope.cancel()

    println("Job was cancelled: ${screenScope.coroutineContext[Job]?.isCancelled}")
    println("Done")
}

suspend fun writeToDatabase(result: Int) {
    println("Writing $result to database")
}

Running the above main function prints out:

Start
Launched
Writing 0 to database
Job was cancelled: true
Done

This is not right. The screenScope was cancelled, the call to writeToDatabase should not have happened. But it did happen, because the runCatching did not re-throw the CancellationException thrown by the delay(1000) statement.

If we change the code like this (replacing runCatching with runSuspendCatching):

fun main() {
    val screenScope = CoroutineScope(Dispatchers.IO)

    println("Start")

    screenScope.launch {
        println("Launched")

        val result = runSuspendCatching {
            delay(1000)
            4
        }.getOrElse { 0 }

        writeToDatabase(result)
    }

    Thread.sleep(500)

    // User leaves the screen.
    screenScope.cancel()

    println("Job was cancelled: ${screenScope.coroutineContext[Job]?.isCancelled}")
    println("Done")
}

suspend fun writeToDatabase(result: Int) {
    println("Writing $result to database")
}

this is printed out:

Start
Launched
Job was cancelled: true
Done

This is correct. The call to writeToDatabase was not made at all, which is the desired behavior (try replacing runSuspendCatching with just a plain run and remove the getOrElse; you'll get the same result).

@Anton-Spaans-Intrepid
Copy link
Author

(looping in my personal github account: @streetsofboston)

@mhernand40
Copy link

mhernand40 commented Feb 19, 2020

FWIW, the catch operator of Flow already specifically handles CancellationException in that it will re-throw it if the Throwable instance is of type CancellationException.

I also asked about a similar runCatching variant for suspend functions in https://medium.com/@mhernand40/thank-you-for-the-reply-dd01cf846ab7.

@elizarov
Copy link
Contributor

@Anton-Spaans-Intrepid Let's take a look at a more realistic example, please. What are the use-cases for runCatching in an application code? Can you give a code snippet that motivated you to come with this proposal?

@recheej
Copy link
Contributor

recheej commented Aug 10, 2020

@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 CancellationException? I think in most cases you want to ignore this exception when logging or your crash reporter.

With CancellationException I'll want to rethrow it or just ignore it. It would be nice to just have some runCatching function that does this behavior for you. You can of course write it pretty simply yourself (that's what I ended up doing), but I think it'd be nice if the library included it for you. I think in general there's a lack of documentation on best practices on handling CancellationException from the library. Maybe you can include such a thing and also link to this utility function?

@recheej
Copy link
Contributor

recheej commented Aug 10, 2020

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")
            }
       }
   }

@erikhuizinga
Copy link

runCatching is expected to run catching. It would be very strange if some exceptions would not be caught by it. I can imagine the usefulness of a new function Result<*>.except<reified T : Throwable>() or Result<*>.throwIfIsInstance<reified T : Throwable>() that would rethrow. For example:

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 CancellationException at any time. I don't want to catch and materialise those, so in my use case I just want to rethrow them so that all parents can handle them too: they might need to do some finalising before they can cancel properly.

@vogthenn
Copy link

Also, what's with OutOfMemoryErrors and similar? It's an antipattern (not sure whether it's disputed or not) to catch Throwable in Java, and I'd guess it also is in Kotlin.
Example: I don't think that it's really expected by the user to ignore OutOfMemoryErrors and NOT dump the JVM heap, even if you configured the JVM to dump.
Catching Throwable will do just that (again: Not 100% sure about Kotlin, not the expert, yet).

@erikhuizinga
Copy link

@vogthenn by that reasoning I wonder what the original use case is for runCatching, because you make it sound like an antipattern without legitimate uses.

@ephemient
Copy link

@erikhuizinga https://github.com/Kotlin/KEEP/blob/master/proposals/stdlib/result.md Looks like intended use cases are postponing failure and internal within coroutines.

@erikhuizinga
Copy link

It seems that the KEEP's use case 'Functional error handling' (specifically the example with runCatching { doSomethingSync() }) is one that easily introduces the side effect being discussed in the current issue: it catches all throwables instead of the ones that are expected.

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. CancellationException).

Are the aforementioned use case and the proposed ways of handling errors in the function's signature contradictory?

@martinbonnin
Copy link

martinbonnin commented Sep 20, 2021

Using runCatching {} is tempting to handle the I/O errors described in https://elizarov.medium.com/kotlin-and-exceptions-8062f589d07. There are some cases where listing all possible exceptions is complex, especially when they come from deeply nested, potentially JDK/third party SDKs.

It'd be nice to be more explicit as to how to use exceptions. In the current state, APIs like runCatching {} and Flow.catch {} make it easy to use exception for control flow which can be dangerous as showcased here.

PS: also, the fact that Result was only available from runCatching {} for some time was an incentive to use the dangerous runCatching {}

@ephemient
Copy link

It'd be nice to be more explicit as to how to use exceptions. In the current state, APIs like runCatching {} and Flow.catch {} make it easy to use exception for control flow which can be dangerous as showcased here.

As noted in #1814 (comment) Flow.catch {} does re-throw CancellationException.

@martinbonnin
Copy link

As noted in #1814 (comment) Flow.catch {} does re-throw CancellationException.

Indeed. I understand that Flow is in the coroutine world and runCatching is not, so it somehow makes sense. But it also makes things more confusing that one of them is "cancellation aware" while the other is not.

@gresolio
Copy link

gresolio commented Feb 2, 2022

Thank you Anton, really good suggestion.

I have a question: how to deal with the TimeoutCancellationException?
In most cases, the timeout should be treated as a failure result.
But TimeoutCancellationException inherits from the CancellationException, so it will be re-thrown.

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?

@lamba92
Copy link

lamba92 commented Jun 27, 2022

It seems that the KEEP's use case 'Functional error handling' (specifically the example with runCatching { doSomethingSync() }) is one that easily introduces the side effect being discussed in the current issue: it catches all throwables instead of the ones that are expected.

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. CancellationException).

Are the aforementioned use case and the proposed ways of handling errors in the function's signature contradictory?

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 *.

@Legion2
Copy link
Contributor

Legion2 commented Jul 19, 2022

We use runCatching for wrapping any exceptions into new exceptions which add context information. We need to catch all exception, because we call 3rd party function which do not specify which exceptions they can throw. The reason for doing this is to add debug information to the exception stacktrace, because often the call stack of a single exception is not complete and only contains the stack of the current coroutine. So we add runCatching before we do a suspend call and wrapp any exceptions. As a result we have the call stack from all coroutines. However if we wrap a CancellationException info a normal RuntimeException and throw the RuntimeException, the cancellation becomes an error.

The question is how can we add context information to the cancelation cause while the CancellationException is traveling down the call stack, so that if we finally log the exception we have all debug information we need?

@joffrey-bion
Copy link
Contributor

joffrey-bion commented Jul 25, 2022

We use runCatching for wrapping any exceptions into new exceptions which add context information

@Legion2 so how exactly do you wrap OutOfMemoryError, and why do you even want to wrap it at all? Using runCatching catches Throwable, not Exception. Plenty of which you shouldn't even try to catch (probably almost all Error descendants). It's not an API that is meant for business code, but rather something for framework code that needs to report exceptions in a different way, such as the coroutines library itself.

@Legion2
Copy link
Contributor

Legion2 commented Jul 25, 2022

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.

@NinoDLC
Copy link

NinoDLC commented Oct 7, 2022

Any update on this ?

Being able a catch a CancellationException in a coroutine doesn't make any sense to me. In a perfect world, it should be impossible. It only happens because coroutines are based on Exceptions to propagate state. To me, it's an anti-pattern for a coroutine to be able to prevent the termination of its scope. A coroutine can specify it needs to "terminate gracefully" with a finally and / or with NonCancellable, but that should be it.

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.

@dkhalanskyjb
Copy link
Collaborator

dkhalanskyjb commented Feb 14, 2024

Bad news: there's no version of runCatching that correctly supports what the coroutines library currently has.

Here's some code that shows the issue: https://pl.kotl.in/sgZzeKnJP

In essence, because of #3658, it's not correct to just assume that getting a CancellationException means that the current coroutine was cancelled: an operation (for example, Deferred.await) could throw CancellationException just because it stored one from another coroutine. In this case, the way to go is to check the current coroutine for cancellation, and if the coroutine isn't cancelled, it means the CancellationException came was just rethrown from some other coroutine. The recommended pattern if you absolutely must catch every exception (and you typically don't!) is:

try {
  operation()
} catch (e: Throwable) {
  if (e is CancellationException) ensureActive() // will throw a `CancellationException` if the current coroutine was cancelled
  processException(e) // process (for example, log) the exception from `operation`
}

It's also not correct to assume that a CancellationException is safe to ignore if the current coroutine was not cancelled. For example, Flow can throw CancellationException from emit to signal completion, but the current coroutine is not cancelled. The recommended pattern if you absolutely must catch every exception (and you typically don't!) is:

try {
  emit(value)
} catch (e: Throwable) {
  if (e is CancellationException) throw e
  processException(e)
}

@NorbertSandor
Copy link

NorbertSandor commented Feb 14, 2024

if you absolutely must catch every exception (and you typically don't!)

Thanks for the info, until now I thought that arrow's NonFatal() and nonFatalOrThrow() is a good pattern :(

@NorbertSandor
Copy link

NorbertSandor commented Feb 14, 2024

What I don't understand: these kind of bugs/issues should be of very high priority because they make the whole coroutines system unsafe. Or - at least for me - it feels unsafe, I have to think about each try-catch how to correctly handle CancellationException. It is sure that a newbie will make huge mistakes and will think that the coroutines system is unreliable...

@joffrey-bion
Copy link
Contributor

A newbie shouldn't catch all exceptions, that's the bad pattern to avoid, and easy to spot. Coroutines CancellationException is just one example of an exception you shouldn't have tried to catch.

@NinoDLC
Copy link

NinoDLC commented Feb 14, 2024

A newbie shouldn't catch all exceptions, that's the bad pattern to avoid, and easy to spot. Coroutines CancellationException is just one example of an exception you shouldn't have tried to catch.

100% disagree.

Very simple usecase : I do a GET call on Ktor and I don't care at all about any exception. I just want the .body() deserialized as my data class or null, with a 0% chance of crash, now or later. You can't achieve that without catch (e: Exception).

@joffrey-bion
Copy link
Contributor

joffrey-bion commented Feb 15, 2024

@NinoDLC well for the record catch(e: Exception) doesn't achieve 0% chance of crash either, any Error thrown will break this, such as OOM, ThreadDeath, StackOverflowError, etc. It's kind of a fallacy to expect that you can avoid crashes this way. The code in your catch block can probably throw as well.

Also, are you returning null in case of ClassNotFoundException? Don't you need to distinguish this from a 404? Don't you need to distinguish a 404 from a 503 or 500? You probably want to respectively return null, retry, or show the user an "internal error on our end" kind of message. Certainly the code handling the general error message display will not be next to the httpClient.get() (probably a couple architecture layers above, so the exception bubbling is what you want here). So I would repeat, catch(Exception) would be the newbie mistake in this case, and probably should be caught during code review, along with the CancellationException problem.

@Legion2
Copy link
Contributor

Legion2 commented Feb 15, 2024

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.

@NinoDLC
Copy link

NinoDLC commented Feb 16, 2024

@joffrey-bion There's nothing to do against Error by definition : the machine is on an unspecified state (like as you said, process is dying or strack overflow) and any new line of code won't probably work anyway. Even the Java documentation clearly states we shouldn't catch it: An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch. Most such errors are abnormal conditions.. And that's ok if the code crashes there. Either there's a mistake on the developper part or the process is dead anyway, so who cares ? The crash will appear on the crash tool we use and we can fix the bug.

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.

@dkhalanskyjb
Copy link
Collaborator

dkhalanskyjb commented Feb 16, 2024

But why can't we easily wrap exception based APIs (bad) to null / type-safe (good) ?

null should be used when possible for recoverable errors, and exceptions should be used for programmer bugs. If your program is in an incorrect state, it's incorrect for it to continue executing, better to notify the programmer about their mistake and restart the program, or who knows what else the program may do? Kotlin's core libraries throw exceptions all the time when it's expected that the programmer was wrong, returning null instead when it's expected that the programmer can reasonably react to the error. Often, both throwing and null-returning versions are provided to support both kinds of use cases.

You can't magically achieve good, Kotlin-idiomatic error-handling by wrapping an exception-based API into a runCatching, because that would mean saying, "I can handle all exceptional situations, I guarantee that this code always behaves correctly, no matter what exception it's failing with." And that is a really tough thing to guarantee! When presented with several exceptions that you want to wrap in a Kotlin-idiomatic way, you have to decide which exceptions you can react to and which indicate an exceptional condition. Otherwise, you get things like 100% CPU consumption while waiting in a loop for your network connection to restore, or you get silently swallowed indicators that your Wi-Fi adapter has broken, or something like that: there very often are things a program can't meaningfully react to, and it's desirable to use exceptions in such cases and crash (or at least log) everything.

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 catch (e: Exception). Maybe this feedback could be actionable (or maybe there are already ways to work around that).

@joffrey-bion
Copy link
Contributor

joffrey-bion commented Feb 16, 2024

There's nothing to do against Error by definition

So you can't have 0% crash, that's my point. Why did you suggest this as a goal at all?

Why should I care if it's 400 / 500 / parsing ?

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 IllegalArgumentException, which is a developer mistake too?

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"

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 null for every error. How would I know I need to report this to my metrics (and fix the things that are bugs) if the bugs yield the same result as "there is no value right now"?

There are cases for catching Exception, but usually this implies rethrowing a more specific one with a cause (that's ok). But here we're talking about returning null, which is a big no-go for me.

No need for extra boilerplate to handle an exception I don't see value in.

Using runCatching + Result management adds boilerplate, exception bubbling is free of syntax (it's the default). Also, you cannot compare the amount of boilerplate of the runCatching approach with multiple catch blocks handling different errors, because these don't do the same thing. If you have one generic catch-all (which kinda emulates the runCatching way), then you can compare, but the whole point is that this block wouldn't be at this level, so again you wouldn't see the boilerplate.

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.

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".

Why can't we easily wrap exception based APIs (bad) to null / type-safe (good) ?

Do you have an example exception-based API that you consider bad?

@joffrey-bion
Copy link
Contributor

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.

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 expectSuccess = true.

Or do you mean more hardcore exceptions like IO errors / connection closed?

@NorbertSandor
Copy link

NorbertSandor commented Feb 16, 2024

There's nothing to do against Error by definition

So you can't have 0% crash, that's my point. Why did you suggest this as a goal at all?

The arrow-kt library differentiates among Errors by using the following condition:

public actual fun NonFatal(t: Throwable): Boolean =
  when (t) {
    is VirtualMachineError, is ThreadDeath, is InterruptedException, is LinkageError, is ControlThrowable, is CancellationException -> false
    else -> true
  }

Is it totally incorrect?
(Let's now ignore that it is obviously incorrect to always handle CancellationException as "fatal", there are lots of open cancellation-related Kotlin and library issues and questions.)

@NorbertSandor
Copy link

NorbertSandor commented Feb 16, 2024

OFF:

It always depends on the application

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 ;)
(And error handling is the key of successful software, nowdays the "happy path" can almost be programmed by chimpanzees with the help of IDEs, AIs, etc. :) )

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?

create an excessive guide/docs on exception/error handing in kotlin ... prevent miss use and bugs

It would be awesome! Especially if this documentation would contain the known issues and their "workarounds" as well.

@NinoDLC
Copy link

NinoDLC commented Feb 17, 2024

@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 Errors. Obviously even in my code I'll require() some parameters to not be completely illogical. Obviously in these cases I want my program to crash because it's becoming illogical anyway, so better now than later.

Between:
1/ erasing all environment-induced exceptions "information" to null + not having to worry about a rogue crash for some random thrown exception I don't care + type safety
and
2/ fine grained and boilerplate approach to try / catch exception handling + occasionnal crashes because of missing documentation (or bumping a version that introduce a new thrown exception for example...) + no type safety

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.

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".

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.

Do you have an example exception-based API that you consider bad?

Ktor yes. It feels not Kotlin at all in its call APIs. Why can't I have a simple getOrNull() / postOrNull() / etc... like we have Int.parseOrNull(), Duration.parseOrNull(), List.getOrNull() and so like we do in the rest of the Kotlin ecosystem... I'm not saying this is the only APIs that should be available for Ktor, because obiously we need some fine grained approach for at least telling the difference between a connectivity error and a server error. And in this case, I imagined they would be available as a type-safe model (sealed classes and others) instead of exceptions.

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.

@NorbertSandor
Copy link

NorbertSandor commented Feb 18, 2024

@NinoDLC although I personally don't like the idea of postOrNull(), I agree that error handling in Ktor is far from optimal. There are cases when you can handle errors only by catching all exceptions: https://youtrack.jetbrains.com/issue/KTOR-2630/Client-Handle-network-exceptions-in-the-commonMain-module

in Kotlin, we still use exceptions to drive our code flux instead of type safe alternatives

I agree with this but the root of Kotlin's success was Java compatibility, so we have to live with it.
But I'm also waiting for type-safe alternatives like arrow-kt's Raise to become more widespread and used at least in new code bases. But these alternatives will never be part of libraries like Ktor because of backwards compatibility reasons. So either you create your own wrappers around Ktor's functions as you like (just as you can implement postOrNull() today) or accept that exceptions will remain with us forever...

@NinoDLC
Copy link

NinoDLC commented Feb 18, 2024

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.

@joffrey-bion
Copy link
Contributor

You completely missunderstand my points almost like a strawman

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'm only talking about environment-induced exceptions, not logical (programming) errors or even Errors. Obviously even in my code I'll require() some parameters to not be completely illogical. Obviously in these cases I want my program to crash because it's becoming illogical anyway, so better now than later.

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 null for everything doesn't give information about anything, just like using Any? everywhere doesn't bring any type safety. On the JVM any call can fail with a throwable, that's an implicit part of any function signature. Using null doesn't change this. Proper type safety would be to use a sealed type for the errors, but then won't you complain about the boilerplate to handle all these errors in every call?

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).

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 ?

Catch the IOException either on the spot or at a level that makes sense for this call? I mean, I'm only saying not to catch a general Exception at a low level without knowing what you're catching (again, please do handle them at a higher level, I'm not saying to let the app crash).

Also, feel free to use extensions. I don't think it's valid to blame the library API design for your decision to use catch(Exception) in business code. You're saying that all your calls to Ktor should be wrapped in catch(Exception) due to poor Ktor design, yet you're also writing "I'm only speaking about "exceptions that aren't"". If the API doesn't suit your needs, you can definitely write your own extensions on top of Ktor's functions, and use those instead. I still reckon those extensions should catch just what you need to catch - the infamous "exceptions that aren't" as you put it -, and you could turn the result into sealed types for instance. So these extensions would still not require catch(Exception) IMO, because you still want IAEs, ISEs, etc. to bubble up (at least that's what you wrote earlier too). Even if you did catch Exception in general in these extensions, those are no longer business code, and I'm almost fine with those. But you're talking about putting catch(Exception) throughout your code, which is different.

It feels not Kotlin at all in its call APIs. Why can't I have a simple getOrNull() / postOrNull() / etc... like we have Int.parseOrNull(), Duration.parseOrNull(), List.getOrNull() and so like we do in the rest of the Kotlin ecosystem

(NB: I'm assuming you mean String.toIntOrNull when writing Int.parseOrNull())

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.
If you look at the code, String.toIntOrNull() doesn't catch any exception at all, and Duration.parseOrNull only catches a specific exception too. Both of those could definitely throw exceptions in case of other issues. They don't contain a catch(Exception) guard or anything, and that's not the point.

Also, toIntOrNull() is not more idiomatic Kotlin than toInt() (which seemed to be your point). It's all about expressing a different intention: toInt() means you don't expect an invalid String here, and it would be a programmer's error to let one arrive at that point, whereas toIntOrNull() explicitly expects invalid input and deals with it by handling the null result. Ktor offers the same choice by providing expectSuccess = true|false. The case for IOException is indeed debatable, but that's an orthogonal discussion I believe.

@NinoDLC
Copy link

NinoDLC commented Feb 18, 2024

On the other hand, it seems you really want to read "let the app crash" when I write "catch at a higher level"

I didn't mean to strawman you, but I really don't understand... Whatever the level where I try/catch, I will have to catch (Exception) somewhere if I don't want Ktor to crash my application. My question is : why do every single developer using Ktor need to know every single Exceptions that can possibly arise from a HttpClient.get, otherwise they face a crash at runtime (or face a rogue coroutine by swallowing CancellationException) ? We can agree I don't strawman there, and that's a terrible surface API in my eyes. That's what I call "not type-safe", because there's no check done at compile time. I may be using the wrong terminology here, english is not my native language, but I guess you get my point.

Say, by chance, experience or reading 30 pages Ktor learn-to-do-everything guide, we know an IOException exists. Nothing in the API code tells us that (contrary of ? or a sealed class) but we follow Ktor principles, we catch IOException instead of Exception. Now our backend (or some shady 3rd party backend we can't do anything about) returns a different response. Then another wave of crashes arrives because ResponseException (or whatever the Exception that would be raised, I found no clear documentation, the documentation can be lacking). Even after reading the documentation, I'm not sure what exceptions can arise when calling HttpClient.get.

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 get on Ktor.

For now, this something else is use a terrible "SafeHttpClient" that wraps every get and post in a try/catch (Exception) (+ ensureActive). And as you said, that's OK. Kotlin is extensible. I agree.

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 :
1/ They don't even think about exceptions and handle a poor QA / client reception (a bit of "devil's advocate" there but there's not a single mention of any exception in the documentation,
2/ They do, but only for IOException, which is not enough,
3/ They do and go safe and unfortunately swallow CancellationException by catching Exception without re-throwing, now they possible leak coroutines,
4/ Same but arguably worse by catching Throwable instead, still without re-throwing.

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 CancellationExceptions to communicate between each other and any developper can interrupt this communication without any warning (how come there's not even a lint warning when we try/catch (Exception/Throwable) a coroutine?).

I still reckon those extensions should catch just what you need to catch - the infamous "exceptions that aren't" as you put it -

Once again, how can I know what I need to catch ? There's no checked exception in Kotlin (and there's sealed classes a reason for that but that's just my opinion). If I go throught every line of Ktor code, read every line of documentation page of guide, I may come to a nice and exhaustive 12 lines long try / catch block that will only catch IOException, ResponseException and whatever else, but not CancellationException because it needs to bubble up. Once again, there's no static check that I didn't miss any exception but yolo. Now Ktor can introduce a new Exception that doesn't belong to those I previously catched and my application may crash even if the code compiled just fine. I'm really sorry but that's just a terrible idea. The only alternative - catching(Exception) - is better in my opinion but it's still very boilerplaty for Kotlin and error-prone with Coroutines.

@NorbertSandor
Copy link

NorbertSandor commented Feb 19, 2024

There's no checked exception in Kotlin

Take a look at: Working with typed errors

Although it is built on the experimental feature context receivers (which will be replaced by context parameters eventually), I use arrow-kt's raise() in my own code with success. And although officially it works only on JVM, with Kotlin 1.9.22 it seems to work in multiplatform, too.

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).

@NorbertSandor
Copy link

that's a terrible surface API in my eyes. That's what I call "not type-safe"

There are several options to wrap existing APIs according to your taste, like the Raise construct I mentioned before, xxxOrNull(), kotlin-result, the built-in Result class if a suboptimal solution is acceptable, returning sealed classes, etc.

why do every single developer using Ktor need to know every single Exceptions that can possibly arise from a HttpClient.get, otherwise they face a crash at runtime (or face a rogue coroutine by swallowing CancellationException)

It is a legacy of Java, and arises from the non-pure-functional paradigm of Kotlin. We simply cannot do anything with it.
What Jetbrains could do is to write much better documentation on exception/error handling, and to take the related issues more seriously - like this one, filed 4 years ago, and lots of others also related to coroutines.

But this discussion has gone slightly offtopic, maybe we should focus to runCatching() again :)

@mattshoe
Copy link

mattshoe commented Mar 8, 2024

It seems that there really should be a "coroutine-purposed" version of runCatching available for devs

It's not immediately obvious to newcomers to coroutines that runCatching is not safe to use with coroutines, and it is not unreasonable for them to expect a language-provided tool behaves cooperatively with coroutines (a language provided tool)

It seems like implementing a cooperativeCatch function that behaves the same as runCatching but is implicitly designed to work with Coroutines would be great way to help developers preserve their current patterns and ease transitions to coroutines.

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)
    }
}

@dkhalanskyjb
Copy link
Collaborator

@mattshoe, #1814 (comment)
I feel like we should open fewer GitHub issues and more GitHub discussions. With a single linear history of comments without any structure, it quickly becomes unmanageable to keep track of all that's been said.

@mattshoe
Copy link

mattshoe commented Mar 8, 2024

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests