https://kotlinlang.org logo
#coroutines
Title
# coroutines
d

Dmytro Danylyk

12/03/2018, 5:08 AM
Decided to write about several things which in my opinion you should and shouldn’t do (or at least try to avoid) when using Kotlin coroutines. Would appreciate if someone can check it and give a feedback. 😉 https://medium.com/@dmytrodanylyk/kotlin-coroutines-patterns-anti-patterns-f9d12984c68e
👍 5
o

oshai

12/03/2018, 6:58 AM
I have 2 suggestions: say in the title it is related to androind, and make the example more clear what is wrong and right
b

bdawg.io

12/03/2018, 7:03 AM
In code examples and the medium post preview image,
withContext
instead of
witchContext
(unless that's a new method I haven't come across yet)
Any particular reason you're utilizing your
networkClient
on
Dispatchers.Default
instead of
<http://Dispatchers.IO|Dispatchers.IO>
?
t

Tolriq

12/03/2018, 7:42 AM
I would have the classWorkManager extend
CoroutineScope
directly. I would use
SupervisorJob
to avoid parent Job cancellation. And to avoid recreating a job use
coroutineScope.cancelChildren
b

bdawg.io

12/03/2018, 7:49 AM
I was just coming back on to suggest
cancelChildren
@Tolriq haha
d

Dmytro Danylyk

12/03/2018, 7:51 AM
Doh the typo.. I will fix that.
@bdawg.io regarding networkClient, no reason just to switch context to any dispatcher which operates on background thread but I will be fixing type anyway, so IO it is.
👍 1
d

dave08

12/03/2018, 8:36 AM
At first, you recommend using Main as a scope, and then you put withContext(Main) in the login fun... I think it should be the opposite, the network call should have a withContext(IO) and be called from the scoped to Main presenter, and there win't be any crashes... what do you think @Dmytro Danylyk?
d

Dmytro Danylyk

12/03/2018, 8:44 AM
@dave08 yes, you are right, ideally it should be done that way. But in the example of “Avoid writing suspend function with an implicit dispatcher” I wanted to show that if you do have the case where you need to write suspend function which touch UI and does some background work you need to do that correctly.
@dave08 think of it as when you have a library which handles all the logic and UI and expose for you suspend function
login
.
d

dave08

12/03/2018, 8:47 AM
Even then, any network calls should be called in the right context in their implementation, no?
Especially if they're
suspend
and blocking...
So that lib would also have its context set to Main...
It might be nice to put this extra note for good practices.
d

Dmytro Danylyk

12/03/2018, 8:49 AM
@dave08 yes, so the correct implementation is shown later on:
Copy code
suspend fun login(): Result = withContext(Dispatcher.Main) {
    view.showLoading()
    val result = withContext(<http://Dispatcher.IO|Dispatcher.IO>) {  
        networkClient.login(...) 
    }
    view.hideLoading()
    return result
}
d

dave08

12/03/2018, 8:51 AM
Well, I'd move the network call's withContext into the function declaration and make it suspending
Then the code would be much cleaner
And safer (you won't forget the context)
I liked your point about recreating the job after cancelling though 😉
👍 1
t

Tolriq

12/03/2018, 8:58 AM
Should not do that 😉 Multithreading can lead to a job start between the cancel and the new creation. coroutineScope.cancelChildren is the proper way to avoid issues, cancel all child jobs without the parent. (And for a workerpool with job that can fail, you should really really use SupervisorJob as any job that fails, will cancel the parent otherwise.
d

Dmytro Danylyk

12/03/2018, 9:03 AM
@Tolriq if your job fail and you didn’t handle that, it will crash your app. So what the point of always using SupervisorJob?
t

Tolriq

12/03/2018, 9:04 AM
Not necessarily, you can have CoroutineExceptionHandler, you can have async.await that even if exception is catched will cancel the parent too.
So not always use SupervisorJob but for Generic stuff like a WorkManager it's certainly a good idea. And since you want users to learn, it's better to explain the main concepts at that time as they will fall into this at some point.
Coroutine docs are not yet fully end user ready and details information are a little spread 😞
d

Dmytro Danylyk

12/03/2018, 9:08 AM
@Tolriq I didn’t know that when you attach CoroutineExceptionHandler it will cancel parent job as well. I need to check that.
t

Tolriq

12/03/2018, 9:09 AM
Not sure it happens in all cases, but the main pain point is async / await that does that 100%
d

Dmytro Danylyk

12/03/2018, 9:10 AM
Is it when you catch exception inside async function or when you wrap await with try/catch?
Copy code
try { work.await() }
catch(e: Exception { }
t

Tolriq

12/03/2018, 9:11 AM
when you wrap it
And it's how async is meant to be used for exception handling.
d

Dmytro Danylyk

12/03/2018, 9:12 AM
So the code above will cancel parent job?
t

Tolriq

12/03/2018, 9:12 AM
yep
see linked isssue for details
well it does cancel the job that by default cancel parent unless using supervisor
d

Dmytro Danylyk

12/03/2018, 10:10 AM
@Tolriq thanks, that’s interesting, not sure how I missed that, probably because I don’t use async too often. I will add this a separate advice.
t

Tolriq

12/03/2018, 10:12 AM
There's many subtle things that are hard to spot 😞 Like https://github.com/Kotlin/kotlinx.coroutines/issues/830 many many people show that as example and it's wrong. With unwrapped exception rethrown meaning very very hard debugging.
Docs will improve with time, but it's better if all tutorial that are currently being written take those in account already.
d

Dmytro Danylyk

12/03/2018, 11:09 AM
Thanks everyone for help. Send me your twitter or medium account name if you want me to add link to your account in the “Special thanks” section of the article.
l

louiscad

12/03/2018, 2:11 PM
@Dmytro Danylyk I'd add that for async/await and catching errors properly, you should do
async
and
await
calls inside a
coroutineScope { … }
lambda, and catch any exception/throwable outside of this very scope:
Copy code
try {
    coroutineScope {
        val mayFailAsync1 = async {
            mayFail1()
        }
        val mayFailAsync2 = async {
            mayFail2()
        }
        useResult(mayFailAsync1.await(), mayFailAsync2.await())
    }
} catch (e: IOException) {
    // handle this
} catch (e: AnotherException) {
    // handle this too
}
👍 1
g

gildor

12/03/2018, 2:28 PM
Copy code
withContext(<http://Dispatcher.IO|Dispatcher.IO>) { networkClient.login(...) }
Not sure that this is a good example even with IO dispatcher. Usage of blocking API of networking library is already a bad practice itself (blocking additional thread, no cancellation) and such example does encourage this. If you replace networkClient.login() to something like
someBlockibgCall()
it would be more explicit
Agree with point of Dave about bad style to wrap blocking calls right in coroutine block instead extracting them to own suspend functions, this is bad style imo that couse pollution of business logic with thread switching
p

pakoito

12/03/2018, 2:35 PM
@louiscad how is that better than a
handleErrorWith
operator is beyond me.
☝️ 1
for a low level API it's okay, I wouldn't expect to write that code multiple times through the codebase when it could be abstracted
1
g

gildor

12/03/2018, 2:37 PM
What is
handleErrorWith
?
l

louiscad

12/03/2018, 2:37 PM
@pakoito Extracting error handling to a domain specific function to not have try catch blocks all over the place is a good practice, but that doesn't free you from using structured concurrency properly
p

pakoito

12/03/2018, 2:39 PM
Copy code
useResult(mayFailAsync1.await(), mayFailAsync2.await())
  .handleErrorWith { when (it) is { ... }  }
@gildor wrapping fail but you get the point
because suspended functions have no types there's no general extfun you can create to abstract this pattern, so you have to resort to per-case solutions
l

louiscad

12/03/2018, 2:40 PM
@pakoito That doesn't follow structured concurrency because not wrapped into a local
coroutineScope { … }
, so it is plain unsafe.
p

pakoito

12/03/2018, 2:40 PM
okay, move the handleErrorWith extfun to coroutineScope then
same
except you cannot handle and recover from individual errors for each of them, or can you?
that's not entirely clear
and how does it interact with cancellation
l

louiscad

12/03/2018, 2:52 PM
You can do everything, but you should do it outside of the
coroutineScope { … }
call.
p

pakoito

12/03/2018, 2:57 PM
so you have to create a sub-scope per async call to recover from errors in it
and make that subscope inherit from the parent scope
is that correct?
l

louiscad

12/03/2018, 3:10 PM
coroutineScope { … }
, what you call the "subscope", is automatically cancelled if the
CoroutineContext
it was called from is cancelled. And, yes, each time you want to do parallelization with
async
(and
await
), you have to make sure you have a local scope where you can recover from the outside, but in practice, you do not do parallelization so often as calling suspend functions sequentially.
p

pakoito

12/03/2018, 3:26 PM
thank you for the rundown @louiscad. I'll apply those to our Deferred wrapper
j

Jorge Castillo

12/03/2018, 8:38 PM
I guess that's why our wrapper is not supporting cancelation properly now?
l

louiscad

12/03/2018, 8:42 PM
Probably half of the reason. The other is that cancellation is not built into Kotlin itself, but depends on kotlinx.coroutines. Making something without kotlinx.coroutines means making your own system that is incompatible with the one in kotlinx.coroutines, unless you achieve implementing a two way hack (I think it is possible one way on JVM only using reflection, but not possible two way IMHO)
👍 1
d

Dmytro Danylyk

12/04/2018, 12:37 AM
If you replace networkClient.login() to something like
someBlockibgCall()
it would be more explicit
👍
p

pakoito

12/04/2018, 12:51 AM
@louiscad we have our own implementation working end to end for our datatype
IO
already. Now we have to comply with the same behavior bridging Deferred. It's possible, just a moving target at the moment :)
☝️ 1
p

pakoito

12/04/2018, 2:43 PM
that resource is outdated because it doesn't take scopes into account, from what I could gather in this conversation
l

louiscad

12/04/2018, 10:46 PM
@pakoito Cancellation is still cooperative, that didn't change with structured concurrency.
a

Alexey Pushkarev

12/06/2018, 6:57 PM
@Dmytro Danylyk why launch here is without any scope passing ?
d

Dmytro Danylyk

12/06/2018, 10:30 PM
@Alexey Pushkarev because activity implements
CoroutineScope
so launch will be executed using that scope.