Coroutines are incredibly complex as I have learned over the

Question

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

pajatopmr · Answer

The actual code is on Github at <https github com pajato tmdb lib> fwiw

gildor · Answer

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

pajatopmr · Answer

Cool thanks for offering up a critique

pajatopmr · Answer

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

pajatopmr · Answer

2 Channel is for safe transfer of list back to the main thread

gildor · Answer

See pull request <https github com pajato tmdb lib pull 2 files>

gildor · Answer

gt 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

gildor · Answer

gt 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

gildor · Answer

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

pajatopmr · Answer

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 slightly smiling face

gildor · Answer

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

gildor · Answer

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

pajatopmr · Answer

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

gildor · Answer

I see what you mean and this is problem with `runBlocking`

gildor · Answer

If you want async code just use async request

pajatopmr · Answer

Ok I ll keep studying your solution

gildor · Answer

Yes my solution doesn t handle issue with main thread I can show how you can handle it if make your API suspendable

pajatopmr · Answer

Go for it

gildor · Answer

Done see my last commit <https github com pajato tmdb lib pull 2 commits 26db30b15bc9c037d0cba47191b5043c436b6adc>

gildor · Answer

Updated commit added info about related issues with suspend functions testing