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

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

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

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?

@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

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"

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

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.

@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

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.

Replace with

supervisorScope

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

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.

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.

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?

@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

.

@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

@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?