We started migration of our Android apps to Struct...

# coroutines

[gildor]

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

[itnoles]

async should be multiple background calls. If it just one, you can use withContext()

[gildor]

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

[gildor]

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

[louiscad]

I don't think failing async { … } coroutine cancels the scope, but I think uncaught exceptions from the await() call do.

[gildor]

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")
        }
    }
}

[gildor]

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

[gildor]

And then try it with kotlinx.coroutines 1.0.0-RC1 which changed default scope behaviour

[gildor]

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)

[gildor]

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

[gildor]

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

[louiscad]

@elizarov Why would async { ... } cancel its scope and not just rely on await() call to throw? That sounds unnatural to me...

[gildor]

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

[gildor]

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

[elizarov]

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.

[elizarov]

2. Can you please clarify what is the use-case?

[gildor]

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()

[gildor]

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

[gildor]

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

[dekans]

Can we achieve 2. with launch(Job())?

[gildor]

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

[louiscad]

I think async cancelling scope is risky. Does this happen also on normal cancellation (CancellationException or subclass)?

[gildor]

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

[dave08]

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?

[dekans]

@gildor stated the contrary in the beginning of this thread, coroutineScope ignores parent job type

[gildor]

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

[dave08]

So what does this mean?

maybe make sense to change coroutineScope default implementation and create new Job instance if outer context contains SupervisorJob

[dave08]

@gildor, supervisorScope does ignore th coroutine context's Job... that was my point, why is coroutineScope behavior different?

[gildor]

It means something like:

val existingJob = coroutineContext[Job].takeIf { it !is SupervisorJob }
CoroutineScope(existingJob ?: Job())

[gildor]

Oh, I see your point

[gildor]

It's good question actually

[dave08]

Until you raised this issue, I assumed coroutineScope ignored the outer context too 🙈

[Vsevolod Tolstopyatov [JB]]

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")
    }

[gildor]

Yes, sure, You right

[gildor]

A right solution should also listen competition of parent job, probably add invokeOnComplete

[gildor]

I mean that Job in this case not only way to handle cancellation but also controls child coroutines cancellation strategy

[gildor]

I just would like to see some idiomatic way to switch between those 2 behaviours

[Vsevolod Tolstopyatov [JB]]

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.

[gildor]

Now yes

[gildor]

Because it uses parent Job

[gildor]

Including SupervisorJob

[gildor]

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

[dave08]

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

[dave08]

Should be:

val existingJob = coroutineContext[Job].takeIf { it !is SupervisorJob }
CoroutineScope(existingJob ?: Job(existingJob))

, no?

[dave08]

At least that's how I expected coroutineScope { } to work, and that would conform with the current working of supervisorScope { }

[Vsevolod Tolstopyatov [JB]]

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?

[Vsevolod Tolstopyatov [JB]]

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 🙂

[Vsevolod Tolstopyatov [JB]]

@gildor you might be interested in https://github.com/Kotlin/kotlinx.coroutines/issues/763