Request for comments: May I ask for feedback on my...

# coroutines

[marcinmoskala]

Request for comments: May I ask for feedback on my collection of Kotlin Coroutines Best Practices? Would you add something? Or do you disagree with some points? I want it to represent the voice of community, not only my experiences.
https://kt.academy/article/cc-best-practices

[Joffrey]

Cool! I agree with almost everything here. Just a couple comments on 2 of those points:

• Suspend functions should be safe to call from any thread - while it is generally good advice, and <http://Dispatchers.IO|Dispatchers.IO> should be used around blocking calls to avoid blocking coroutines, I would not generally advise hardcoding a dispatcher in most places. This reduces testability significantly when using the kotlinx-coroutines-test project (the test dispatcher can no longer easily be injected, or stops propagating high in the call hierarchy)
• Use SupervisorJob when creating CoroutineScope - I wouldn't phrase it this way. You're saying we can freely assume that we don't want a coroutine failure to cancel the others. It's usually the case indeed (I don't have an example right now where this isn't the case). But, rather than making the assumption for them, I'd just advise readers to be careful about this specific property of the scope when they create a custom one. And if the assumption holds for them, then yes use SupervisorJob

[Robert Williams]

I don't think you properly explained why this code is wrong/ how to fix it:

suspend fun main(): Unit = coroutineScope {
    val job = Job()
    launch(job) {
        delay(1000)
        println("Text 1")
    }
    job.join() // Here we will await forever
    println("Will not be printed")
}

The problem is the same problem as "Job is not inherited" but the correct fix is to use the Job returned by launch instead of making a new Job

[franztesca]

Consider cancelling scope children

Once a scope is cancelled, it cannot be used anymore in the future. If you want to cancel all the tasks started on the scope, but you might want to keep this scope active, cancel its children. Keeping scope active costs us nothing.

I think this is not a great practice. In general each scope should be linked to some actual lifecycle and have an owner that takes care of cancelling it when the corresponding lifecycle finishes AFAIK.
In which use case did you find yourself preferring cancelling the children instead of the underlying scope for reusability?

[Joffrey]

@Robert Williams I guess it is mentioned as However, in general, I would avoid using Job builder, except for constructing a scope., but I agree this sentence should be given more importance and placed earlier maybe. Actually maybe a more useful title for this section should be Avoid using the Job() factory function outside custom scope creations

[Joffrey]

I agree with @franztesca, I also never ran into a situation where I created my custom job and needed to reuse it. But I also never work in Android applications. I would assume even for Android you usually want to tie the coroutine scope to a component's lifecycle and cancel the whole scope when throwing the component away. If you need to launch new coroutines, you probably re-create such component with a new scope anyway.

[Robert Williams]

That's exactly my point though: it's confusing to give advice like "don't do X" without giving an alternative. The only suggested fix here is to complete the job manually or use job.children.forEach and it doesn't mention that the API directly returns the Job you should be using which makes the original Job() not needed. Kind of makes the whole thing read like "don't do X but sometimes you need to" rather than "you probably never need to"

[Joffrey]

Fair point, I agree. This is why I would suggest a section like Avoid using the Job() factory function outside custom scope creations, which describes why you should never (or rather, rarely) need to do so

[Sam]

Great stuff! The parts about jobs and structured concurrency are particularly important and it’s always great to see diagrams of relationships between jobs 👌👍.
One comment on “use awaitAll”: I believe that is potentially misleading advice. If you’re using structured concurrency properly, awaitAll is unnecessary. async will always fail its parent job immediately on failure, regardless of if you use awaitAll or not. In my experience people often don’t realise this, which can lead to confusion.

[Joffrey]

@Sam the coroutines (and job) backing the Deferred instances are not necessarily part of the same job hierarchy as the one doing the awaitAll, though

[Joffrey]

And when it is, using awaitAll() over map { it.await() } still reads better and doesn't have drawbacks that I know of

[Sam]

True 👍, and I think I agree that there’s no real disadvantage to using awaitAll everywhere just in case. But in the typical pattern of

val results = coroutineScope {
    items.map { async { ... } }.map { it.await() }
}

I think it’s very useful and important for people to understand that awaitAll will not change the behaviour.

[Joffrey]

Replace with supervisorScope, and tada! 😄 Though, that would be pretty weird... But I have seen worse

[marcinmoskala]

Thank you all for the suggestions.

@Joffrey
Regarding "Suspending functions should be safe to call from any thread", I considered your doubt, but I think it is a minor problem, because if you follow this suggestion, you will use dispatchers mainly in data layer, which is faked/mocked anyway in unit tests. I mentioned the necessity to inject dispatcher for testing and I think it should be enough. I added the following paragraph "It is a controversial topic if Dispatchers.Main.immediate should be used explicitly in suspending functions updating Android views. This should depend on the project policy. We do not need to use it in the layers where Dispatchers.Main is considered the default dispatcher, like in the presentation layer in many Android projects."
Regarding "Use SupervisorJob when creating CoroutineScope", I never had a situation where I would find a different job preferred. If anyone shows me such case, I will mention it.

I renamed suggestion to "Avoid using Job builder except for constructing a scope"

@Robert Williams I condered your comment, and I added the following text to "Avoid awaiting on standalone Job"

In most cases, the simplest solution is to join on the job returned by coroutine builders. Most common cases include storing active task job in a variable, or collecting jobs of all started coroutines.


```kotlin
class SomeService {
   private var job: Job? = null
   private val scope = CoroutineScope(SupervisorJob())


   // Every time we start a new task,
   // we cancel the previous one.
   fun startTask() {
       cancel()
       job = scope.launch {
           // ...
       }
   }
  
   fun cancel() {
       job?.cancel()
   }
}

kotlin
class SomeService {
   private var jobs: List<Job> = emptyList()
   private val scope = CoroutineScope(SupervisorJob())
  
   fun startTask() {
       jobs += scope.launch {
           // ...
       }
   }


   fun cancel() {
       jobs.forEach { it.cancel() }
   }
}
```

I hope you like this alternative, do you?

@franztesca Your suggestion is very Android-specific, but I will add it, as it is important for Android devs. This is what I added:

On Android, instead of defining and cancelling custom scopes, you should prefer using `viewModelScope`, `lifecycleScope` and lifecycle-aware coroutine scopes from ktx libraries, that are cancelled automatically.

Is it enough?

@Sam I do agree it might not be necessery in most cases, still it will be useful as I believe it is more efficient and better accumulate exceptions. That you presented later was well explained in the book.

[marcinmoskala]

I think I managed to address all the suggestions, let me know if there is anything else 🙂

[Joffrey]

Regarding "Use SupervisorJob when creating CoroutineScope", I never had a situation where I would find a different job preferred. If anyone shows me such case, I will mention it.

@marcinmoskala I have a couple classes in the Krossbow project that use a CoroutineScope() with a regular Job(). These are launching coroutines upon initialization, and those coroutines are supposed to live for the lifetime of the class. No other coroutine is launched in the scope through user interaction with this class, they are more like housekeeping background coroutines. If one of them fails, the whole class is in a weird state and everything should be cancelled, so a regular Job is more appropriate. Here is the code if you're interested (there are launch calls in the init block, and a startIn call):
https://github.com/joffrey-bion/krossbow/blob/main/krossbow-stomp-core/src/commonMain/kotlin/org/hildan/krossbow/stomp/BaseStompSession.kt#L29

As a rule of thum, I'd say when the launched coroutines are housekeeping long-lived coroutines, they probably need to all be running or all be cancelled, and regular jobs are therefore better. When coroutines can be launched on demand through the API of the class, then maybe SupervisorJob is better because some of those "requests" may fail and we probably don't want to fail others in this case.

[Joffrey]

Actually it turns out my example is slightly more complicated and has a mix of both, but the coroutines that are launched "on-demand" have all the same "reason to fail" - the websocket failure. So in that case it's still ok (and probably desirable) to kill all launched coroutines when one fails.

Maybe that was not my best example after all 🙂 and there are plenty of things that should be reworked in this class. But I hope you get the idea.

[Joffrey]

Btw, this also breaks the "avoid `suspend fun(...): Flow<...>`" advice, because the current design didn't allow any other way to properly wait for the subscription to happen before doing anything (onStart is not enough!), apart from wrapping the type of the flow elements to materialize the subscription (which I really didn't want to do).

[Robert Williams]

You can always do

fun nonSuspendingFlowCall() = flow {
  emitAll(suspendingFlowCall())
}

Is that what you meant by "wrapping the type of the flow elements"? Any particular reason you don't want to do it?

[marcinmoskala]

@Joffrey I see your example, but I do not feel it. As you mention, the consequances of using Job is a wired class state where everything is broken. Also, such classes must be designed for a single process - to me it is waiting for someone who start something else on the scope, and we have a serious occassion for problems that are hard to debug. Thank you for your opinion, but I do not want people writing code this way, at least not without documenting that properly, or naming this scope purpose-specific (like sharedStompEventsScope), so I will stick to my suggestion.

Best practices sometimes needs to be broken, they are supposed to be guidelines for standard situations.

I actually like this sentence. I decided to add in the end of this chapter "Before we close this chapter, I want you to remember this sentence: Best practices sometimes needs to be broken, they are guidelines for standard situations, not rules for every situation."

@Robert Williams AFAIK emitAll expects Flow , it does not accept List.

[Joffrey]

@Robert Williams the problem is on the call site. The point is to have a guarantee about the subscription to be able to do something between the subscribe call and the flow collection, so having just a cold flow is not enough

[Joffrey]

@marcinmoskala well this is an implementation detail of the class, so nothing can randomly change. But I do get the point about the name, which makes sense. I still believe this is a valid and not-so-rare case among the custom scopes cases. I find there is rarely a need for custom scopes in the first place anyway

[franztesca]

@franztesca Your suggestion is very Android-specific, but I will add it, as it is important for Android devs

I used the words "lifecycle" and "owner" like in Android but my point was more broad, it should be applicable everywhere.
Every CoroutineScope must have an owner (the one entilted to cancel it). If there is no owner then you are using CoroutineScope in an unstructured way, if there is more than one, then nasty things are going to happen (like cancelling jobs that weren't supposed to be cancelled).
Now, in which circumstances would the owner cancel the children rather than cancelling the whole scope? I don't see this use case happening often: most of the times you'd be better off by just cancelling the whole scope when the owner becomes "unused".

For example, let's imagine we write an audio player, we have an AudioPlayer class that owns a CoroutineScope on which player-related coroutines are launched (for example to buffer some audio data asynchronously). Now, at some point, that audio player instance will not be used anymore, so the app will call AudioPlayer.release and the CoroutineScope will be cancelled. This is good, right?
We could say "but what if I want to reuse the audio player? Let's just cancel the children instead". We can do that, but that means that we are not really done with the player instance (aka we don't call AudioPlayer.release, but AudioPlayer.reset, for example). We add it, but that doesn't remove the need to have also a release method to close (cancel) completely the scope. The scope owner (AudioPlayer) should always close (cancel) the scope, eventually.
That's why cancelling the scope is much more common use case than cancelling its children in my experience, and the point in your article seems to promote leaving CoroutineScopes active indefinitely by cancelling its children. While cancelling the children often works in practice (scopes are cheap and as long as you're sure that there are no children running it should be fine), I don't think that is the best way of modelling concurrency in a structured way.
WDYT?