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

safeAsync #4253

Open
Nodrex opened this issue Oct 20, 2024 · 4 comments
Open

safeAsync #4253

Nodrex opened this issue Oct 20, 2024 · 4 comments

Comments

@Nodrex
Copy link

Nodrex commented Oct 20, 2024

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
Copy link
Collaborator

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
Copy link
Author

Nodrex commented Oct 21, 2024

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
Copy link
Collaborator

dkhalanskyjb commented Oct 21, 2024

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
Copy link
Author

Nodrex commented Oct 21, 2024

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

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

No branches or pull requests

2 participants