13

I am new to Kotlin/Coroutines, so hopefully I am just missing something/don't fully understand how to structure my code for the problem I am trying to solve.

Essentially, I am taking a list of strings, and for each item in the list I want to send it to another method to do work (make a network call and return data based on the response). (Edit:) I want all calls to launch concurrently, and block until all calls are done/the response is acted on, and then return a new list with the info of each response.

I probably don't yet fully understand when to use launch/async, but I've tried to following with both launch (with joinAll), and async (with await).

fun processData(lstInputs: List<String>): List<response> {

    val lstOfReturnData = mutableListOf<response>()

    runBlocking {
        withContext(Dispatchers.IO) {
            val jobs = List(lstInputs.size) {
                launch {
                    lstOfReturnData.add(networkCallToGetData(lstInputs[it]))
                }
            }
            jobs.joinAll()
        }
    }

    return lstofReturnData

What I am expecting to happen, is if my lstInputs is a size of 120, when all jobs are joined, my lstOfReturnData should also have a size of 120.

What actually is happening is inconsitent results. I'll run it once, and I get 118 in my final list, run it again, it's 120, run it again, it's 117, etc. In the networkCallToGetData() method, I am handling any exceptions, to at least return something for every request, regardless if the network call fails.

Can anybody help explain why I am getting inconsistent results, and what I need to do to ensure I am blocking appropriately and all jobs are being joined before moving on?

Sergio
  • 27,326
  • 8
  • 128
  • 149
SteveNikks
  • 193
  • 1
  • 1
  • 8

3 Answers3

17

mutableListOf() creates an ArrayList, which is not thread-safe.
Try using ConcurrentLinkedQueue instead.

Also, do you use the stable version of Kotlin/Kotlinx.coroutine (not the old experimental one)? In the stable version, with the introduction of structured concurrency, there is no need to write jobs.joinAll anymore. launch is an extesion function of runBlocking which will launch new coroutines in the scope of the runBlocking and the runBlocking scope will automatically wait for all the launched jobs to finsish. So the code above can be shorten to

val lstOfReturnData = ConcurrentLinkedQueue<response>()
runBlocking {
        lstInputs.forEach {
            launch(Dispatches.IO) {
                lstOfReturnData.add(networkCallToGetData(it))
            }
        }
}
return lstOfReturnData
Willi Mentzel
  • 27,862
  • 20
  • 113
  • 121
Ryan Kenvin
  • 186
  • 1
  • 3
  • Thank you so much for the reply and pointer on using ConcurrentLinkedQueue, and the updates on the stable API. Yes, I am using the new version, but a lot of my team's code is on the old one so I see I mixed stuff up a bit. I just did some initial testing where I am introducing a `delay(2000)` and it looks to be working as I want. All items fire off (and don't wait for each one to get past the delay), all join back when done, and the list size afterwards is what I am expecting. I will do some more in depth testing this weekend, and accept your answer if it still looks good. – SteveNikks Feb 16 '19 at 15:25
  • Thank you so much for this solution! Please, fix the typo in `Dispatches` ('r' is missed). – Igor Tulmentyev Feb 28 '21 at 08:38
4

runBlocking blocks current thread interruptibly until its completion. I guess it's not what you want. If I think wrong and you want to block the current thread than you can get rid of coroutine and just make network call in the current thread:

val lstOfReturnData = mutableListOf<response>()
lstInputs.forEach {
    lstOfReturnData.add(networkCallToGetData(it))
} 

But if it is not your intent you can do the following:

class Presenter(private val uiContext: CoroutineContext = Dispatchers.Main) 
    : CoroutineScope {

    // creating local scope for coroutines
    private var job: Job = Job()
    override val coroutineContext: CoroutineContext
        get() = uiContext + job

    // call this to cancel job when you don't need it anymore
    fun detach() {
        job.cancel()
    }

    fun processData(lstInputs: List<String>) {

        launch {
            val deferredList = lstInputs.map { 
                async(Dispatchers.IO) { networkCallToGetData(it) } // runs in parallel in background thread
            }
            val lstOfReturnData = deferredList.awaitAll() // waiting while all requests are finished without blocking the current thread

            // use lstOfReturnData in Main Thread, e.g. update UI
        }
    }
}
Sergio
  • 27,326
  • 8
  • 128
  • 149
  • This approach seems more effective, you can correct me if I'm wrong but since all jobs will be run simultaneously the execution time will be less compared to the accepted answer. – Ishaan May 23 '20 at 11:41
1

Runblocking should mean you don't have to call join. Launching a coroutine from inside a runblocking scope should do this for you. Have you tried just:

fun processData(lstInputs: List<String>): List<response> {

val lstOfReturnData = mutableListOf<response>()

runBlocking {
    lstInputs.forEach {
            launch(Dispatchers.IO) {
                lstOfReturnData.add(networkCallToGetData(it))
            }
   } 
}

return lstofReturnData
TomH
  • 2,581
  • 1
  • 15
  • 30