https://kotlinlang.org logo
Title
g

gildor

10/24/2018, 3:26 AM
We started migration of our Android apps to Structured Concurrency implementation from kotlionx.coroutines (before we used own solution for that) and now I have question about CoroutineScope. Looks that for any UI component SupervisorJob is the best choice (it’s even mentioned on Coroutines guide, see https://github.com/Kotlin/kotlinx.coroutines/blob/master/docs/exception-handling.md#supervision), because you usually don’t want to cancel all the jobs is one of them failed on your UI component, this is especially important for
async
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 there
i

itnoles

10/24/2018, 3:56 AM
async should be multiple background calls. If it just one, you can use withContext()
g

gildor

10/24/2018, 4:58 AM
What do you mean? Of course, I understand where to use async It can be used for multiple calls or as suspendable alternative for lazy But it means nothing for my concerns. You still can have multiple async operations, you just don’t want to cancel whole scope just because some of operations fail in some particular case
I’m explictly mention UI component, because it’s standard case there. Imagine that you load data to show it in UI, you run a couple requests in parallel, one of them failed and cancellation of scope is not what you want. You want to show error message and probably show some “Retry” button and if scope is cancelled you can do nothing anymore. Only somehow reset job but it doesn’t make sense because instead you can use SupervisorJob
l

louiscad

10/24/2018, 5:50 AM
I don't think failing
async { … }
coroutine cancels the scope, but I think uncaught exceptions from the
await()
call do.
g

gildor

10/24/2018, 5:58 AM
It does, check for example this code:
import 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")
        }
    }
}
You can run it on play.kotlinlang.org which uses old version of coroutines (or just use any version older than 0.30.0) before changing semantics of Job and get:
Job 1: failed java.lang.IllegalStateException
Job 2: job 2 result
And then try it with kotlinx.coroutines 1.0.0-RC1 which changed default scope behaviour
You will get
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)
but if you change coroutineScope with supervisorScope you will get
Job 1: failed java.lang.IllegalStateException
Job 2: job 2 result
and no exception from supervisorScope
Of course this makes a lot of sense in this small code snippet because rationale about default Job behaviour works in this case. But if you imagine a big UI component with multiple nested views/viewmodels/presenters that use this scope such behaviour will be much more error prone in many cases because you want to handle each fail separately and if you have uncaught exception your will just crash
l

louiscad

10/24/2018, 6:34 AM
@elizarov Why would
async { ... }
cancel its scope and not just rely on
await()
call to throw? That sounds unnatural to me...
g

gildor

10/24/2018, 6:35 AM
Because it covers important use case: you run multiple
async
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/552
Nothing wrong with this behaviour imo, this case is really important and behaviour is more safe and efficient (you never forgot to cancel coroutines which result already not needed) But this behaviour is not the best choice for any complex scopes with multiple nested scopes and decoupled tasks, such as Activity/Fragment
e

elizarov

10/24/2018, 7:23 AM
1. Maybe it does make sense to recommend
SupervisorJob
. I’m not sure what would be most common error handling practice in android development, though.
2. Can you please clarify what is the use-case?
g

gildor

10/24/2018, 7:29 AM
2. I thought about something like that:
class 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 exceptions
2. 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
d

dekans

10/24/2018, 9:02 AM
Can we achieve 2. with
launch(Job())
?
g

gildor

10/24/2018, 9:06 AM
yes, this also should work, but coroutineScope may be more efficient, because starts undispatched coroutine, but not sure that it visible in real life. I just think that
coroutineScope
is more idiomatic for this case. But It can be done using another approaches, such as coroutines builders or just using awaitAll
l

louiscad

10/24/2018, 9:11 AM
I think
async
cancelling scope is risky. Does this happen also on normal cancellation (
CancellationException
or subclass)?
g

gildor

10/24/2018, 9:12 AM
if you not cancelling scope you will not solve problem with hanging coroutines that not required anymore (upd: actually awaitAll solves this problem explicitly), so we have 2 approaches: Default and Supervisor
d

dave08

10/24/2018, 11:16 AM
Isn't it inconsistent that
supervisorScope
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?
d

dekans

10/24/2018, 12:39 PM
@gildor stated the contrary in the beginning of this thread,
coroutineScope
ignores parent job type
g

gildor

10/24/2018, 12:42 PM
coroutineScope uses any job that available in coroutine context, if no jobs are available new Job will be created. I propose to do that more explicit and create new Job if only SupervisorJob available and do not inherit supervisor behavior
d

dave08

10/24/2018, 12:42 PM
So what does this mean?
maybe make sense to change
coroutineScope
default implementation and create new Job instance if outer context contains SupervisorJob
@gildor,
supervisorScope
does ignore th coroutine context's
Job
... that was my point, why is
coroutineScope
behavior different?
g

gildor

10/24/2018, 12:44 PM
It means something like:
val existingJob = coroutineContext[Job].takeIf { it !is SupervisorJob }
CoroutineScope(existingJob ?: Job())
Oh, I see your point
It's good question actually
d

dave08

10/24/2018, 12:46 PM
Until you raised this issue, I assumed
coroutineScope
ignored the outer context too 🙈
v

Vsevolod Tolstopyatov [JB]

10/24/2018, 12:52 PM
It 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")
    }
g

gildor

10/24/2018, 12:53 PM
Yes, sure, You right
A right solution should also listen competition of parent job, probably add invokeOnComplete
I mean that Job in this case not only way to handle cancellation but also controls child coroutines cancellation strategy
I just would like to see some idiomatic way to switch between those 2 behaviours
v

Vsevolod Tolstopyatov [JB]

10/24/2018, 12:59 PM
A right solution should also listen competition of parent job, probably add invokeOnComplete
Not sure what do you mean.
coroutineScope
will be cancelled, if outer job is cancelled.
g

gildor

10/24/2018, 12:59 PM
Now yes
Because it uses parent Job
Including SupervisorJob
My (incorrect) code snippet was just an attempt to illustrate how I would like to see work of coroutineScope inside supervisorScope: switch to non-supervisor job explicitly
d

dave08

10/24/2018, 1:06 PM
But you still would want the new Job to have the outer scope as its parent, to handle if the outer scope is cancelled? That doesn't seem to be what that snippet does...
Should be:
val existingJob = coroutineContext[Job].takeIf { it !is SupervisorJob }
CoroutineScope(existingJob ?: Job(existingJob))
, no?
At least that's how I expected
coroutineScope { }
to work, and that would conform with the current working of
supervisorScope { }
v

Vsevolod Tolstopyatov [JB]

10/24/2018, 1:14 PM
Thread is too big and mixing multiple things at once, so I’m slightly lost. @gildor let’s focus on semantics of what we have and what you are missing.
coroutineScope
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?
About your point with a guide and SupervisorJob. You are right, we will revisit out guides and recommendations right after 1.0.0. We still should figure out some parts of the picture before recommending using SupervisorJob. For example: To cancel he whole activity, one of the child jobs should fail. Why can it happen? 1) Uncaught exception in
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 🙂
@gildor you might be interested in https://github.com/Kotlin/kotlinx.coroutines/issues/763
👍 2