marcinmoskala
04/03/2023, 11:48 AMJoffrey
04/03/2023, 12:01 PMSuspend 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
04/03/2023, 12:07 PMsuspend 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 Jobfranztesca
04/03/2023, 12:09 PMConsider 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
04/03/2023, 12:10 PMHowever, 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
Robert Williams
04/03/2023, 12:27 PMjob.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
04/03/2023, 12:28 PMAvoid using the Job() factory function outside custom scope creations
, which describes why you should never (or rather, rarely) need to do soSam
04/03/2023, 3:21 PMawaitAll
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
04/03/2023, 3:22 PMDeferred
instances are not necessarily part of the same job hierarchy as the one doing the awaitAll
, thoughawaitAll()
over map { it.await() }
still reads better and doesn't have drawbacks that I know ofSam
04/03/2023, 3:25 PMawaitAll
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
04/03/2023, 3:29 PMsupervisorScope
, and tada! 😄 Though, that would be pretty weird... But I have seen worsemarcinmoskala
04/04/2023, 10:33 AMDispatchers.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.Joffrey
04/04/2023, 12:19 PMRegarding "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.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
04/04/2023, 2:59 PMfun 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
04/04/2023, 3:43 PMsharedStompEventsScope
), 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
04/04/2023, 5:36 PMfranztesca
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?