https://kotlinlang.org logo
Title
m

marcinmoskala

04/03/2023, 11:48 AM
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
j

Joffrey

04/03/2023, 12:01 PM
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
r

Robert Williams

04/03/2023, 12:07 PM
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
f

franztesca

04/03/2023, 12:09 PM
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?
j

Joffrey

04/03/2023, 12:10 PM
@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
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.
r

Robert Williams

04/03/2023, 12:27 PM
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"
j

Joffrey

04/03/2023, 12:28 PM
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
s

Sam

04/03/2023, 3:21 PM
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.
j

Joffrey

04/03/2023, 3:22 PM
@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
And when it is, using
awaitAll()
over
map { it.await() }
still reads better and doesn't have drawbacks that I know of
s

Sam

04/03/2023, 3:25 PM
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.
j

Joffrey

04/03/2023, 3:29 PM
Replace with
supervisorScope
, and tada! 😄 Though, that would be pretty weird... But I have seen worse
m

marcinmoskala

04/04/2023, 10:33 AM
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.
I think I managed to address all the suggestions, let me know if there is anything else 🙂
j

Joffrey

04/04/2023, 12:19 PM
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.
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.
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).
r

Robert Williams

04/04/2023, 2:59 PM
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?
m

marcinmoskala

04/04/2023, 3:43 PM
@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
.
j

Joffrey

04/04/2023, 5:36 PM
@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
@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
f

franztesca

04/04/2023, 11:09 PM
@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?