https://kotlinlang.org logo
Join Slack
Powered by
Decided to write about several things which in my ...
# coroutines
d
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
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
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
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
I was just coming back on to suggest 
cancelChildren
@Tolriq haha
d
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
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
@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
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
@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
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
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
@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
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
@Tolriq I didn’t know that when you attach CoroutineExceptionHandler it will cancel parent job as well. I need to check that.
t
Not sure it happens in all cases, but the main pain point is async / await that does that 100%
d
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
when you wrap it
And it's how async is meant to be used for exception handling.
d
So the code above will cancel parent job?
t
yep
see linked isssue for details
well it does cancel the job that by default cancel parent unless using supervisor
d
@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
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
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
@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
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
@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
What is 
handleErrorWith
?
l
@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
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
@pakoito That doesn't follow structured concurrency because not wrapped into a local 
coroutineScope { … }
, so it is plain unsafe.
p
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
You can do everything, but you should do it outside of the 
coroutineScope { … }
call.
p
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
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
thank you for the rundown @louiscad. I'll apply those to our Deferred wrapper
j
I guess that's why our wrapper is not supporting cancelation properly now?
l
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
If you replace networkClient.login() to something like 
someBlockibgCall()
it would be more explicit
👍
p
@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
j
p
that resource is outdated because it doesn't take scopes into account, from what I could gather in this conversation
l
@pakoito Cancellation is still cooperative, that didn't change with structured concurrency.
a
@Dmytro Danylyk why launch here is without any scope passing ?
d
@Alexey Pushkarev because activity implements 
CoroutineScope
so launch will be executed using that scope.
Open in Slack
PreviousNext
Powered by