-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Async builder and cancellation in structured concurrency #763
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
@qwwdfsad I agree with your concerns. Unfortunately I don't fully understand your consideration about My proposal is to consider My second consideration is to avoid any suspend fun <T : Comparable<T>> List<T>.quickSort(): List<T> =
if(size <= 1) this
else coroutineScope {
val pivot = first()
val (smaller, greater) = drop(1).partition { it <= pivot}
val smallerSorted by fork { smaller.quickSort() }
val greaterSorted by fork { greater.quickSort() }
smallerSorted.quickSort() + pivot + greaterSorted.quickSort()
} |
@fvasco This would need to allow |
Yes @LouisCAD, I like the idea.
Edit: suspending delegates does not requires |
My proposal is to consider Erlang style spawn / link / monitor instead of Also could consider go but for safety better Go style go ? |
@fvasco Wow, I didn't know about this trick! private suspend operator fun <T> Deferred<T>.getValue(
thisRef: Any?,
property: KProperty<*>
): T = await()
val a by async { // IDE shows suspend call here
delay(5000)
1
}
val b by async { // IDE shows suspend call here too
delay(5000)
2
}
val c = a + b // Suspends, but IDE doesn't show it. However, according to play.kotl.in, this no longer compiles in Kotlin 1.3, as |
Hi @LouisCAD, In my opinion A different consideration is for The last operator is As a quick recap of my idea:
|
@fvasco I don't agree with making For an |
@qwwdfsad Maybe |
Can you propose an example so it is not possible to use val something: Deferred<*> = async { }
something.join() // job join
something.await() // check error
fun Deferred<*>.asJob() = object : Job by this However I am not an Android developer, so I don't have any strong opinion against |
@fvasco There are two reasons:
Finally, there's a lot of code that uses The discussion is not about |
I don't understand, fun main(args: Array<String>) = runBlocking<Unit> {
async { error("Scope crashed") }
} @qwwdfsad proposed fun main(args: Array<String>) = runBlocking<Unit> {
async(SupervisorJob(coroutineContext[Job])) { error("Scope crashed") }
} |
@fvasco Did you read the docs for It is already clear with the docs, and clear if you run it, but I'll repeat it once for all: Again, this issue is not about |
This is probably a horrible misuse of completion handlers, but what if async did something like this:
This allows async to work as expected in the happy case and when the async body wants to handle exceptions itself, but still ensures that it's impossible to forget to do so and lose an exception, as long as there's a parent job. |
I wish to recover this issueto reevaluate some previous considerations. @qwwdfsad focus this issue around the Therefore, though we introduce some new fancy builder like |
Wild idea: a function named |
Linking here a related question from Kotlin dicussions https://discuss.kotlinlang.org/t/kotlin-coroutines-are-super-confusing-and-hard-to-use/10628 |
|
Please note that this workaround that is often mentioned above does not work: I have fixed it like this:
This is subtly different too, though, in that failure of a 2nd level subordinate job can still cancel the parent. |
The non-standard |
Can someone add some color to this? What are the implications for those of us following this issue? |
It stays the way it is now. |
I got that, but a lot of different approaches have been discussed in this issue. Your comment seemed to indicate the idiomatic approach now is:
however, your use of the term "non-standard" gave me pause. What did you mean by that? |
It simply means that people coming from other |
This is so confusing. I am reading and testing Kotlin async exception handling for days now. And I am still not getting it. This is even worse than Java with it's Future-API from hell (can't believe, I'm writing this). Wish we could just have simple async/await equivalents like in C# or Javascript. They just do their stuff like expected without having to deal with (global) scopes, coroutine builders, dispatchers, suspend functions, supervisors, catched exceptions bubbling up and crashing my app etc. The current state is just - awful. Everyone is just confused how to use all those stuff correctly. Sorry. In C# all those works with async/await like expected, done. In Kotlin it's rocket science. |
@spyro2000 In Kotlin, it's done the correct way: Also, you need |
For me, it looks like a design flaw.
I hardly imagine cases without a double indentation level due to |
@neworld Can, you please, clarify, what are you trying to achieve? Why your use-case is not handled by a much simpler code like shown below?
|
Your code has no error handling at all. And the app would end itself right away (AFAIK) as launch() does not wait until anything has finished (maybe In C# this all would just works fine and in parallel like this:
What's the Kotlin equivalent to this? |
Let's expand this example with a bit of context. I suppose that C# snippet is part of some async method (because it is using async Task<int> SumTwoThingsConcurrentlyAsync() {
try {
var a = someAsyncMethod();
var b = someOtherAsyncMethod();
var c = await a + await b;
return c;
} catch (e: Exception) {
// handle
}
} In Kotlin the similar function looks like this: suspend fun sumTwoThingsConcurrently() {
try {
coroutineScope {
val a = async { someMethod() }
val b = async { someOtherMethod() }
val c = a.await() + b.await()
return@coroutineScope c
}
} catch (e: Exception) {
// handle
}
} I don't see how that's much different from C# code, but it is clearly safer than C# code. The problem with C# solution is that if the call to No such problem with the code in Kotlin, even though the code looks quite similar. When |
Thank you @elizarov So the magic part to prevent the catched exception bubbeling up is basically |
My original answer above had a bug (I flipped the nesting of |
@specherkin It depends on what are you doing in your main method and why you need to wrap it. |
@elizarov, tricky happens for complex business logic, which involved many parallel IO organized in a tree. For example content feed with multiple data sources. Doing that properly is not hard, and my point is not that coroutines have bad API. It just looks like design flow because code fills with many |
@neworld I still don't understand why would ever need to write |
I think this happens when you write a suspending function that actually produces or returns a The case that comes up fairly often for me is memoizing asynchronous results. When someone wants the value, I would return or await a previously cached |
It's great that a failure of the enclosing scope cancels the async tasks. The problem is that exceptions thrown by the async tasks cancel the enclosing scope. |
@elizarov Thank you again. I tried to adapt your example in my simple test setup: fun main() {
System.getProperties().setProperty("Dkotlinx.coroutines.debug", "true");
runBlocking {
val value1Def = async { getValueThrowsExceptionAsync() }
val value2Def = async { getValueAsync() }
val sum = try {
println("Awaiting results...")
value1Def.await() + value2Def.await()
} catch (e: Exception) {
println("Catched exception")
0
}
println("Our result: $sum")
}
}
suspend fun getValueAsync() = coroutineScope {
println("Calling without exception...")
delay(2000)
println("Calling without exception - DONE...")
return@coroutineScope 1
}
suspend fun getValueThrowsExceptionAsync() = coroutineScope {
println("Calling with exception...")
delay(3000)
println("Throwing exception...")
throw RuntimeException("Argh!")
return@coroutineScope 1 // this is actually dead code but is enforced by compiler
} But even that results in the following output:
So the exception is still not catched :( Also tried the following (out of sheer desperation): suspend fun main() = coroutineScope {
System.getProperties().setProperty("Dkotlinx.coroutines.debug", "true");
val sum = try {
val a = async { getValue() }
val b = async { getValueThrowsException() }
a.await() + b.await()
} catch (e: Exception) {
println("Catched")
}
println("Sum: $sum")
} Still the same. Exception crashes the app. This, however, seems to work as excpected: suspend fun main() {
System.getProperties().setProperty("Dkotlinx.coroutines.debug", "true");
supervisorScope {
val a = async { getValueThrowsException() }
val b = async { getValue() }
// do something else
try {
println(a.await() + b.await())
} catch (e:Exception) {
println("catched")
}
}
} Output:
So, is this the preferred pattern to avoid exceptions breaking out from coroutines? |
That's not a problem, but a feature, and I like and use that feature, personally. For the cases where I don't want it, I catch the errors inside the |
@spyro2000 Here's the correct version of your last snippet: suspend fun main() {
System.getProperties().setProperty("Dkotlinx.coroutines.debug", "true");
try {
val result = coroutineScope {
val a = async { getValueThrowsException() }
val b = async { getValue() }
// do something else
a.await() + b.await() // Last expression of the lambda is its return type, put in result
}
println(result)
} catch (e:Exception) {
println("catched")
}
} |
@LouisCAD Thank you. Are there any disadvantages / pitfalls to use make the main method suspending? In other words - can I always do that? And why does this actually work at all (coroutineScope() seems to behave like runBlocking()) and waiting for an result before terminating the app? |
Getting OT here, but like |
@spyro2000 Now, it's best to look at the KDoc of both to understand the differences (that reply above summarizes a little). In the case of the main function, there's a few things to know as of Kotlin 1.3 (or gotchas if you prefer):
|
@rocketraman @LouisCAD Many thanks to both of you. |
@elizarov Could you please answer the following questions?
|
@taras-i-fedyk We used to have a different As for your other question and comments, I cannot quite grasp the context of what you are trying to achieve here. Can you please elaborate with example snippet of code which you'd like to improve the behavior of? |
@elizarov Here's my response. It'll be a bit long because I have to explain what exactly I meant within my initial post.
|
Since my posts above haven’t been fully addressed here, I’ve created two dedicated issues for those topics. I hope the discussion will be continued there. |
If we don't do this, catching the exception isn't enough; it'll still bubble to its parent and kill everything. See Kotlin/kotlinx.coroutines#763 for an extended discussion of Kotlin misdesigns. Signed-off-by: Jason A. Donenfeld <[email protected]>
Background
After
async
integration with structured concurrency (#552), any failure inasync
cancels its parent.For example, following code
cancels outer scope and it usually leads to undesired consequences.
The rationale behind this change was simple, it was previously hard or impossible to have proper parallel decomposition to fail when one of the decomposed computations failed.
While there is nothing wrong with that reasoning (=> we won't revert this change), we now have a more serious problem with user's intuition of this behaviour.
async
is a way too polluted keyword which neither reflects its purpose inkotlinx.coroutines
nor matches similar concepts in other paradigms or programming languages, so any newcomer will be confused with this behaviour and thus has no or incorrect mental model about it.Moreover, if someone already understands concept of
kotlinx.coroutines
async, (s)he still may want to have future-like behaviour (and according to our slack, demand for that is quite high).And there is no simple answer to that.
To have a proper semantic (one-way cancellation), one should write something like
async(SupervisorJob(coroutineContext[Job])) { ... }
(really?!) and it completely defies the goal of having clear and easily understandable API.coroutineScope
is not applicable for that purpose, because it awaits all its children andGlobalScope
is just unsafe.We should address this issue and educating users with "this is intended 'async' behaviour" without providing an alternative is just wrong. I can't imagine the situation where someone asks about this behaviour in Slack and community responds with "yes, this is how it works, you actually need
async(SupervisorJob(coroutineContext[Job])) { ... }
"Possible solutions
In my opinion, it is necessary to provide future-like builder (aka "old" async) and it would be nice if its name won't clash with anything we already have.
For example,
deferred
builder. It is not something newcomers will start to use immediately (whileasync
is kinda red flag "please use me") due to its name, but it is a simple concept, it is clean, short and easy to explain (see my "can't imagine the situation" rant).Another possible solution (please take it with a grain of salt, it requires a lot more design/discussing with other users) is to deprecate
async
at all. As I have mentioned, this name is useless, polluted and does not reflect its semantics. Even withdeferred
builder, we still should make a tremendous effort with educating users that this is notasync
you are familiar with, this is completely differentasync
.But if we will deprecate it and introduce another primitive with a more intuitive name, for example,
decompose {}
(naming here is a crucial part,decompose
is just the first thing that popped in my mind), then we will have no problems withasync
. Newcomers won't see their familiarasync
, butdeferred
anddecompose
and then will choose the right primitive consciously.Reported:
https://discuss.kotlinlang.org/t/caught-exceptions-are-still-propagated-to-the-thread-uncaughtexceptionhandler/10170
#753
#787
#691
The text was updated successfully, but these errors were encountered: