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

Async builder and cancellation in structured concurrency #763

Open
qwwdfsad opened this Issue Oct 25, 2018 · 16 comments

Comments

Projects
None yet
6 participants
@qwwdfsad
Member

qwwdfsad commented Oct 25, 2018

Background

After async integration with structured concurrency (#552), any failure in async cancels its parent.
For example, following code

try {
  async { error("") }.await()
} catch (e: Throwable) {
  ...
}

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 in kotlinx.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 and GlobalScope 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 (while async 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 with deferred builder, we still should make a tremendous effort with educating users that this is not async you are familiar with, this is completely different async.
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 with async. Newcomers won't see their familiar async, but deferred and decompose 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

@qwwdfsad qwwdfsad added the design label Oct 25, 2018

@fvasco

This comment has been minimized.

Contributor

fvasco commented Oct 25, 2018

@qwwdfsad I agree with your concerns.

Unfortunately I don't fully understand your consideration about async, personally I like the old pattern async ... await.

My proposal is to consider fork instead of decompose.
The decompose word suggests me the data structure decomposition, instead fork remembers me the fork join pattern, so we fork the current scope for a future join.

My second consideration is to avoid any join or await phases, this avoids any try { await() } catch { ... } block. In my example below is not clear how handle error inside the coroutine scope.

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()
    }
@LouisCAD

This comment has been minimized.

Contributor

LouisCAD commented Oct 25, 2018

@fvasco This would need to allow suspend val for delegated properties.

@fvasco

This comment has been minimized.

Contributor

fvasco commented Oct 26, 2018

Yes @LouisCAD, I like the idea.
Do you have any consideration against the suspending delegates?

suspend operator fun <T> Fork<T>.getValue(thisRef: Any?, property: KProperty<*>): T

Edit: suspending delegates does not requires suspend val

@SolomonSun2010

This comment has been minimized.

SolomonSun2010 commented Oct 26, 2018

@qwwdfsad I agree with your concerns.

Unfortunately I don't fully understand your consideration about async, personally I like the old pattern async ... await.

My proposal is to consider Erlang style spawn / link / monitor instead of decompose.
spawn(modulename, myfuncname, []) % Erlang
please see also:
http://erlang.org/doc/reference_manual/processes.html

Also could consider go but for safety better Go style go ?
go(CTX) { myfunc() }

@LouisCAD

This comment has been minimized.

Contributor

LouisCAD commented Oct 26, 2018

@fvasco Wow, I didn't know about this trick!
In Kotlin 1.2.71, this compiles, and works as you would expect, apart from the IDE not showing the suspension point for delegated property/local access, but on declaration instead:

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 suspend operator getValue seems to have been marked as unsupported.

@fvasco

This comment has been minimized.

Contributor

fvasco commented Oct 27, 2018

Hi @LouisCAD,
the above example is the my preferred way for building a suspending lazy property (#706):
The fork function is heavily inspired by the lazy one, so I consider it as single solution for launch, async and lazy in a coroutine scope (for decomposition use case).

In my opinion async is a good operator to start a deferred computation and handle its result later (i.e. the awaitFirst problem #424). async live in the scope but it should not fail the scope.

A different consideration is for launch, for my use case it is very uncommon to use a scoped launch, it exposes only a boolean exit status (isCancelled) and it is pretty unclear how to handle it, the most common solution is to steal the error using a CompletionHandler (#345).

The last operator is produce, a producer should live in the scope (like async) without cancelling it, instead both produce and async should be cancelled when the scope exit (#645).

As a quick recap of my idea:

  • launch is a top level function and it is not scoped, like a Thread
  • async is always scoped (GlobalScope is OK), it does not influence the scope but it is cancelled at the scope termination (it cannot more be awaited), it is similar to RX Single
  • produce is always scoped (GlobalScope is OK), it does not influence the scope but it is cancelled at the scope termination (it cannot more be consumed), it is similar to RX Observable
  • fork is always scoped (I don't see a use case for GlobalScope), if a fork fails then the whole scope fails, if the scope fails then all forks fails. The scope terminates successfully if all forks complete the task without errors (or lazy forks are not started at all). It implements the fork-join model.
@LouisCAD

This comment has been minimized.

Contributor

LouisCAD commented Oct 27, 2018

@fvasco I don't agree with making launch top level again without scope, there's a reason it moved to structured concurrency with mandatory scopes. You can still write a globalLaunch or alike method if you want, but scopes are really useful for launch, think Android apps, child coroutines that should not be forgotten, etc…

For an async alternative though, like fork or something, it would be great. Then, we could have an async or alike that uses a SupervisorJobso it doesn't cancel parent on failure, but only throws whenawait()` is called.

@LouisCAD

This comment has been minimized.

Contributor

LouisCAD commented Oct 27, 2018

@qwwdfsad Maybe async should be marked as experimental before 1.0.0 to allow changes regarding this?
Not so much code is currently using it anyway I think.

@fvasco

This comment has been minimized.

Contributor

fvasco commented Oct 27, 2018

@LouisCAD

child coroutines that should not be forgotten, etc…

async can do this job with no downside.

Can you propose an example so it is not possible to use async instead of launch?
Please consider that any Deferred is a Job, and consider the follow code:

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 GlobalScope.launch.

@LouisCAD

This comment has been minimized.

Contributor

LouisCAD commented Oct 27, 2018

@fvasco There are two reasons:

  • async doesn't behave as launch. An uncaught exception in async doesn't crash the app (silent fail), while in launch it does. I don't want the app to continue working on uncaught exceptions.
  • launch has the benefit of not having async connotation that people may have because of async/await in C#, Dart, JS or other languages.

Finally, there's a lot of code that uses launch in Android apps, and moving to async exception handling could introduce many problems because of muted throwables.

The discussion is not about launch vs async which are completely different because of their exception/throwable handling, but about behavior of async when a Throwable occurs, and possible solutions to this.

@fvasco

This comment has been minimized.

Contributor

fvasco commented Oct 27, 2018

An uncaught exception in async doesn't crash the app (silent fail), while in launch it does.

I don't understand, launch and async look similar on 1.0.0-RC1

fun main(args: Array<String>) = runBlocking<Unit> {
    async { error("Scope crashed") }
}

@qwwdfsad proposed async(SupervisorJob(coroutineContext[Job])) { ... }, but the follow code does not solve the issue

fun main(args: Array<String>) = runBlocking<Unit> {
    async(SupervisorJob(coroutineContext[Job]))  { error("Scope crashed") }
}
@LouisCAD

This comment has been minimized.

Contributor

LouisCAD commented Oct 27, 2018

@fvasco Did you read the docs for launch and async? Did you notice the IDE warning when calling async without using its resulting Deferred? Did you compare the results with async and launch? Did you try to understand the differences, beyond the fact that they "look similar"?

It is already clear with the docs, and clear if you run it, but I'll repeat it once for all:
current async since structured concurrency cancels the scope, but the exception is still hidden, and only thrown when calling await(). launch on the other hand crashes the app with the original exception in addition to having the scope cancelled.

Again, this issue is not about launch vs async but about the fact that async cancels the scope even if exception is caught at await() call, and what solutions we have to get the old async behavior safely, and the naming for all of this.

@zach-klippenstein

This comment has been minimized.

zach-klippenstein commented Oct 27, 2018

This is probably a horrible misuse of completion handlers, but what if async did something like this:

  1. When the async body throws an exception, hang onto it. If there is a job in the context, immediately register a completion handler on the job that will rethrow the exception. Hang onto the handler's disposable.
  2. If await is called before the job completes, throw the exception there and consider the exception reported. Dispose the completion handler so it won't propagate to the parent.
  3. If the parent is cancelled without await having been called, the completion handler from 1 will ensure the exception is not lost (it will get wrapped with a CompletionHandlerException).
  4. If there's no parent job, and await is not called, the exception is dropped, but if you're opting out of using structured concurrency you're already using hard mode.

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.

@fvasco

This comment has been minimized.

Contributor

fvasco commented Oct 31, 2018

I wish to recover this issueto reevaluate some previous considerations.

@qwwdfsad focus this issue around the async builder, but I wish to extend the issue to launch and produce.

Therefore, though we introduce some new fancy builder like job,deferred and channel, we need to customize the behavior for each one (really similar to CoroutineStart attribute).
I.e. I can start a supervisor job, but I want to build some child tasks as strictly related to this scope.

@LouisCAD

This comment has been minimized.

Contributor

LouisCAD commented Nov 9, 2018

Wild idea: a function named asyncCatching { ... } that calls async(SupervisorJob(coroutineContext[Job])) { ... } under the hood.
There may be a confusion with Result and xxxxCatching { … } methods though… Maybe a similar convention could be introduced with Deferred?

@elizarov

This comment has been minimized.

Collaborator

elizarov commented Dec 6, 2018

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