gildor
10/24/2018, 3:26 AMasync
builder, because you handle result only when call await
and don’t want to cancel all other coroutines and especially CoroutineScope of this UI component if one of our async jobs is failed, we can actually handle error when call await()
And you need non-supervisor Job and scope only in case if you want to run a few operations in parallel and want to cancel them if one of them failed, but it’s specific and explicit case, at least in usual UI development
So I have 2 points here:
1. Should coroutine guide recommend to use SupervisorJob for all UI components and not Job (what is now standard in documentation, for example check CoroutineScope
docs which use Job in Android Activity). Otherwise it can cause a lot of very subtle bugs when you use CoroutineScope for UI development.
2. We need some easy way to start non-supervisor scope. Now the only way to do that is CoroutineScope(coroutineContext + Job())
which is quite wordy way. maybe make sense to change coroutineScope
default implementation and create new Job instance if outer context contains SupervisorJob
If it make sense for someone, I would happy to create an issue and contribute, but maybe I missed some important use cases thereitnoles
10/24/2018, 3:56 AMgildor
10/24/2018, 4:58 AMlouiscad
10/24/2018, 5:50 AMasync { … }
coroutine cancels the scope, but I think uncaught exceptions from the await()
call do.gildor
10/24/2018, 5:58 AMimport kotlinx.coroutines.*
suspend fun main(args: Array<String>) {
coroutineScope {
val res1 = async {
delay(100)
throw IllegalStateException()
}
val res2 = async {
delay(200)
"job 2 result"
}
try {
res1.await()
} catch (e: Exception) {
println("Job 1: failed $e")
}
try {
val result = res2.await()
println("Job 2: $result")
} catch (e: Exception) {
println("Job 2: failed $e")
}
}
}
Job 1: failed java.lang.IllegalStateException
Job 2: job 2 result
Job 1: failed java.lang.IllegalStateException
Job 2: failed kotlinx.coroutines.JobCancellationException: Job is cancelling; job=ScopeCoroutine{Cancelling}@53e8f4a3
And also coroutineScope will throw an exception, so you have to handle it (wrap coroutine scope invocation to try/catch)Job 1: failed java.lang.IllegalStateException
Job 2: job 2 result
and no exception from supervisorScopelouiscad
10/24/2018, 6:34 AMasync { ... }
cancel its scope and not just rely on await()
call to throw? That sounds unnatural to me...gildor
10/24/2018, 6:35 AMasync
to do something in parallel and if one of them cancelled you don’t want to leak those coroutines, you just want to get error message and cancel other
see this for reference
https://github.com/Kotlin/kotlinx.coroutines/issues/552elizarov
10/24/2018, 7:23 AMSupervisorJob
. I’m not sure what would be most common error handling practice in android development, though.gildor
10/24/2018, 7:29 AMclass SomeActivity : Activity, CoroutineScope {
override val coroutineContext = Dispatchers.Main + SupervisorJob()
suspend fun doSomeTaskInParallel() = coroutineScope {
(0..10).map {
// I want Job() semantics here, cancel all tasks in case of error, but will get SupervisorJob behaviour
launch/async { }
}
}
}
}
But now I see that probably for a real case I would just use awaitAll()
I’m not sure what would be most common error handling practice in android development, though.This is not even about Android. If I have uncaught exception I want to crash the app. But in case of
async
I even don’t have uncaught exception, because I expect to handle it only when I need result and I call .await()
, but Job()
will cancel scope in this case
In case of uncaught exception in launch
it’s actually fine, because this will crash the app and it’s responsibility of developer to handle it, but async encapsulates exceptions2. Can you please clarify what is the use-case?One more point about this, I just think that it make sense to allow switch between cancellation strategies easily, same as between contexts
dekans
10/24/2018, 9:02 AMlaunch(Job())
?gildor
10/24/2018, 9:06 AMcoroutineScope
is more idiomatic for this case. But It can be done using another approaches, such as coroutines builders or just using awaitAlllouiscad
10/24/2018, 9:11 AMasync
cancelling scope is risky. Does this happen also on normal cancellation (CancellationException
or subclass)?gildor
10/24/2018, 9:12 AMdave08
10/24/2018, 11:16 AMsupervisorScope
has a SupervisorJob
behavior, and ignores its parent's type, whereas coroutineScope
depends on its parent to decide if it's a Supervisor
or regular Job
... or am I not understanding something?dekans
10/24/2018, 12:39 PMcoroutineScope
ignores parent job typegildor
10/24/2018, 12:42 PMdave08
10/24/2018, 12:42 PMmaybe make sense to changedefault implementation and create new Job instance if outer context contains SupervisorJobcoroutineScope
supervisorScope
does ignore th coroutine context's Job
... that was my point, why is coroutineScope
behavior different?gildor
10/24/2018, 12:44 PMval existingJob = coroutineContext[Job].takeIf { it !is SupervisorJob }
CoroutineScope(existingJob ?: Job())
dave08
10/24/2018, 12:46 PMcoroutineScope
ignored the outer context too 🙈Vsevolod Tolstopyatov [JB]
10/24/2018, 12:52 PMIt means something like:But it doesn’t. E.g.
GlobalScope.async {
try {
coroutineScope {
async {
try {
delay(Long.MAX_VALUE)
} catch (e: Exception) {
println("Scoped async is cancelled")
}
}
throw Exception()
}
} catch (e: Exception) {
println("Caught exception from coroutineScope")
}
delay(1000)
println("Outer async is not cancelled")
}
gildor
10/24/2018, 12:53 PMVsevolod Tolstopyatov [JB]
10/24/2018, 12:59 PMA right solution should also listen competition of parent job, probably add invokeOnCompleteNot sure what do you mean.
coroutineScope
will be cancelled, if outer job is cancelled.gildor
10/24/2018, 12:59 PMdave08
10/24/2018, 1:06 PMval existingJob = coroutineContext[Job].takeIf { it !is SupervisorJob }
CoroutineScope(existingJob ?: Job(existingJob))
, no?coroutineScope { }
to work, and that would conform with the current working of supervisorScope { }
Vsevolod Tolstopyatov [JB]
10/24/2018, 1:14 PMcoroutineScope
creates isolated coroutine scope, which is cancelled either when its body throws an exception, its child throws an exception or caller (if caller has a job) is cancelled. Cancellation is not propagated upwards to the caller, instead coroutineScope
throws its exception. For example, it’s safe to write:
coroutineScope {
async() { throw E() }.await()
}
Caller will not be cancelled, coroutineScope
will throw E
.
supervisorScope
has all the properties coroutineScope
has except one: it is not cancelled if its child throws an exception.
Having that in mind, could you please clarify which mechanism you are missing right now?launch
. But in that case it’s probably a good idea to fail fast rather than relying on asynchronous side-effect that will never happen?
2) Using async
+ await
. This is a big topic worth separate thread, but for me await
in the activity scope is a signal of an incorrect primitive choice. Because either is should have been withContext
, or parallel decomposition from coroutineScope {}
builder.
3) Produce/actor failure. Don’t know, should think about it 🙂