Kotlin / kotlinx.coroutines

Library support for Kotlin coroutines
Apache License 2.0
13.08k stars 1.85k forks source link

safeAsync #4253

Open Nodrex opened 1 month ago

Nodrex commented 1 month ago

First of all, I am a huge fan of Kotlin Coroutines and I would like to contribute and add a small function that will be useful as I myself had to use it considering real world (real project requirements). So I was using async and awaitAll() but the problem was that all my async tasks crashed if at least one of them crashed (throws an exception) and as far as I understand the only way is to use good old try-catch inside async block. That's fine, but since we use catch block now we need to consider CancellationException and as you already guessed it it leads to bugs, cause in some cases someone may forget about cancelation and we have a bug So here is my little function that uses try-catch, but also handles cancelation and can be safely used in list and not crash all tasks if one of them will be crashed:

fun <T> CoroutineScope.safeAsync(
        context: CoroutineContext = EmptyCoroutineContext,
        start: CoroutineStart = CoroutineStart.DEFAULT,
        block: suspend CoroutineScope.() -> T
    ): Deferred<Result<T>> {
        return async(context, start) {
            try {
                Result.success(block())
            } catch (t: Throwable) {
                if (t is CancellationException) {
                    throw t
                }
                Result.failure(t)
            }
        }
    }

and this is how it can be used:

fun go() {
        runBlocking {
            listOf(safeAsync {
                "just String"
            }).awaitAll().forEach {
                println(it.getOrNull())
            }
        }
    }

Thanks in advance ✌️

dkhalanskyjb commented 1 month ago

as far as I understand the only way is to use good old try-catch inside async block

Failures typically shouldn't be caught, they should be propagated upwards: they are supposed to encode failures, whereas expected erroneous things (like the user sending invalid input) are to be represented by returning a null or a value describing the error. PTAL at https://elizarov.medium.com/kotlin-and-exceptions-8062f589d07 try-catch, especially wrapping something in Result.failure, should not be taken lightly. So what you're proposing looks to me like a convenient way to introduce an antipattern.

the problem was that all my async tasks crashed if at least one of them crashed

If you don't want that to happen, then this can be reflected in how you create your coroutines. By default, coroutines in the same scope are considered to all be parts of the same computations; if one part fails, the rest of them should, too, because the complete computation can not be performed in any case anymore. If you want several coroutines in the same scope to fail independently, you need a SupervisorJob.

Example:

This code will crash every coroutine:

runBlocking {
  val scope = CoroutineScope(Dispatchers.Default)
  val deferreds = List(10) { scope.async { if (it == 7) error(":(") else { delay(50); it } } }
  println(deferreds.map { runCatching { it.await() } })
  // [Failure(...), Failure(...), Failure(...), Failure(...) ...
}

But this code will not:

runBlocking {
  val scope = CoroutineScope(Dispatchers.Default + SupervisorJob()) // Note the SupervisorJob!
  val deferreds = List(10) { scope.async { if (it == 7) error(":(") else { delay(50); it } } }
  println(deferreds.map { runCatching { it.await() } })
  // [Success(0), Success(1), Success(2), Success(3), Success(4), Success(5), Success(6), Failure(java.lang.IllegalStateException: :(), Success(8), Success(9)]
}
Nodrex commented 1 month ago

Thanks for the quick replay @dkhalanskyjb As far as I remember SupervisorJob() does not help in the case of async as it will still throw an exception but I will recheck this scenario and will let you know. Also in async documentation, I found this: is not equivalent to this. map { it. await() } which fails only when it sequentially gets to wait for the failing deferred, while this awaitAll fails immediately as soon as any of the deferreds fail. So as I understand map { it. await() } is not as efficient as awaitAll() My point was to use awaitAll() for efficiency while also mimicking SupervisorScope and use the Kotlin Result class to handle exceptions that might occur in an async block

dkhalanskyjb commented 1 month ago

As far as I remember SupervisorJob() does not help in the case of async as it will still throw an exception but I will recheck this scenario

In the example, I'm showing what happens to async.

awaitAll fails immediately as soon as any of the deferreds fail

That's correct, but you don't want to fail if the deferred values fail, right?

So as I understand map { it. await() } is not as efficient as awaitAll()

Without profiling the code, making such conclusions is difficult. It may be just as efficient in your specific case, or it may be more efficient. Together with Result wrapping, it may be less efficient. Who knows?

If you see any benchmark results demonstrating that await is a performance problem for your use case, please let us know, and we'll look into implementing your use case more efficiently.

Nodrex commented 1 month ago

Ok thanks a lot, I will test it and will let you know