https://kotlinlang.org logo
Join Slack
Powered by
Coroutines are incredibly complex, as I have learn...
# codereview
p
Coroutines are incredibly complex, as I have learned over the past week or so. As part of a movie/tv app I am writing, I faced the need to fetch millions of lines of JSON data for a half dozen or so lists. This is my second take at a daily task to update these records in the app. Critique’s much appreciated:
The actual code is on Github at https://github.com/pajato/tmdb-lib fwiw
g
1. I think you shouldn’t use 
runBlocking
, but instead make function suspendable 2. It’s not clear for me, why do you need channel in this case 3. Instead of passing scope as argument, I would just make function suspend (as in item 1) and use 
coroutineScope {}
block, because this function shouldn’t return until it finish job 4. I would convert fetchLines to suspend function + 
withContext(IO)
, so function become non blocking and you can just can call it, without worry about dispatcher 5. Using MutableMap to return value doesn’t seems correct, better just return value
p
Cool, thanks for offering up a critique.
1) Making dailyExportTask() suspend causes an error: the invoking code must be a coroutine, so this suggestion just forces the runBlocking (or equivalent) to be moved.
2) Channel is for safe transfer of list back to the main thread.
g
2) Channel is for safe transfer of list back to the main thread.
This is strange reason, suspend function is also completely trade safe for that (of course if you don’t touch global state)
1) Making dailyExportTask() suspend causes an error: the invoking code must be a coroutine,
Yes, because you use launch it outside of coroutine, I added a couple TODO with suggestion just expost this API as asyncronous suspend function rather than do blocking network call on singleton access, which is really smells
6. I see that you use a lot of nested functions, this fine, but for my eyes just adds a lot of noise and overcomplicated closures and harder to follow. I refactor everything to private top level functions, so you have this same encapsulation as before but imo it improved readability
p
It’ll take me a while to grok your changes, which I appreciate you suggesting, but it is very clear we don’t care for each other’s preferred style. 🙂
g
So, in general, to your original comment “Coroutines are incredibly complex”, I see that your code was quite complex, you can see that in my version I have only 1 suspend nd function that used only to wrap blocking code and use of 
map + awaitAll
patter to run multiple coroutines concurrently and await all the results It doesn’t look complicated for me Approach with Channel was much more complicated, it true, but there is no reason to use it
Yes, sure, i agree about style, but because I changed how functions are interact each other (no more global state, no more global closures, just pure functions that return result), It looked reasonable also move functions to top level
p
Was it clear that dailyExportTask() will be executed in a main (UI) thread so it needs to just kick off the downloads and update the list data later? The downloads (without coroutines) take ~12 seconds, unacceptable at startup. I haven’t figured out yet of your changes preserve this behavior.
g
I see what you mean, and this is problem with 
runBlocking
If you want async code, just use async request
p
Ok, I’ll keep studying your solution.
g
Yes, my solution doesn’t handle issue with main thread, I can show how you can handle it if make your API suspendable
p
Go for it.
g
Updated commit, added info about related issues with suspend functions testing
4 Views
Open in Slack
PreviousNext
Powered by