CODEWITHSUNDEEP
kotlinandroid-jetpack-composekotlin-coroutines
c_idle
c_idle

Reputation: 1584

Kotlin: ConcurrentModificationException when searching a compose snapshot state list in a coroutine

I have (what I thought) a pretty straightforward concept where I can refresh the details of a list of todo items. What I've found is that if there are enough TODO items (a few thousand) and the refresh button is pressed (therefore calling fetchFreshTodoItemDetails repeatedly) then I crash with this exception:

java.util.ConcurrentModificationException
at androidx.compose.runtime.snapshots.StateListIterator.validateModification(SnapshotStateList.kt:278)
at androidx.compose.runtime.snapshots.StateListIterator.next(SnapshotStateList.kt:257)
at com.rollertoaster.app.ui.screens.todo.TodoScreenViewModel$fetchFreshTodoItemDetails$1$1$1.invokeSuspend(TodoScreenViewModel.kt:332)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:570)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:750)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:677)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:664)
Suppressed: kotlinx.coroutines.DiagnosticCoroutineContextException: [StandaloneCoroutine{Cancelling}@97be452, Dispatchers.Main.immediate]

MyViewModel:

  var fetchJob: Job? = null

 fun fetchFreshTodoItemDetails(idsToRefresh: List<Long>) {
    fetchJob?.cancel()
    fetchJob =
        viewModelScope.launch {
          when (val todosResponse = apiService.getTodos(ids)) {
            is ApiResult.Success -> {
              if (todosResponse.value.isEmpty()) return@launch
              todosResponse.items.forEach { todo ->
                val currentTodo: TodoModel
                val indexOfTodo: Int
                val updatedTodo: TodoModel

                //this search for index could take a long time, so move to CPU bound Dispatcher
                withContext(Dispatchers.Default) {
                

                  // The crash/exception happens on this line VVV
                  indexOfTodo =
                    appState.fullListOfTodos.indexOfFirst { it.id == todo.id }
                  place = appState.fullListOfTodos[indexOfTodo]

                  updatedTodo = TodoModel(//update a few fields)

                }
                // If I remove this line, the crash/exception does not happen VV
                appState.fullListOfTodos[indexOfTodo] = updatedTodo
}}}}}

Although I can think of a few ways to work around this issue... I still "think" that the above should work and the fact that it doesn't is driving me a bit insane. Appreciate any help. Thank you

EDIT: fullListOfTodos is defined in my appStateHolder as such

var fullListOfTodos = mutableStateListOf<TodoModel>()

Upvotes: 2

Views: 3748

Answers (2)

Horațiu Udrea
Horațiu Udrea

Reputation: 1897

The problem

As you mentioned, the search in the list done by indexOfFirst takes a long time. The implementation of the iterator forbids its concurrent modification, meaning that you are not allowed to change the list items while iterating over it for the search. The crash happens here because the list IS modified while it is looped over, yielding a ConcurrentModificationException.

You may think now that there is no way that the list is modified while being iterated, since your code executes sequentially, therefore the modification is done after the search. The issue appears when you call fetchFreshTodoItemDetails multiple times. It creates another coroutine that executes (posibly at the same time) the code that searches and modifies the list. So basically, the function calls race each other and interrupt each other if unlucky.

The reason it was not obvious for you

At this point you would argue that you implemented a mechanism that does not allow two coroutines to run in parallel, using the fetchJob variable, that holds a single job that gets cancelled when the function is called, and then starts a new job. The issue here is that your assumption is false.

When you call fetchJob?.cancel(), you basically send a cancellation signal to the coroutine that is currently executing. Since cancellation is cooperative, the coroutine has to reach a suspension point to be cancelled, which does not happen in the indexOfFirst call. Since fetchJob?.cancel() does not wait for the Job to finish, viewModelScope.launch may execute BEFORE the previous Job finishes, hence executing in parallel with the last call. Spamming the button results in many of these coroutines being created and breaking each other when modifying the list items.

Solutions

Now that you know the problem, you can most likely come up with your own solution after some more documentation (which I highly advise, Kotlin guides for coroutines are great).

There are some quick fixes that you may think of (which I do NOT recommend):

  • You use yield() or coroutineContext.isActive in the long-running search to make it cooperative with the cancellation mechanism. Here you still run the risk of concurrent execution, but it is lower.
  • You use fetchJob?.cancelAndJoin() in order to really wait for the last coroutine to finish, but this just piles up requests and requires the function to be marked as suspend or you need to launch two nested coroutines.

My recommendation is inspired by the actor pattern. You will essentially have a Channel where you can send a signal, the intent of updating your data. This intent will be consumed by a coroutine that runs indefinitely and just starts the searching process whenever a new intent is present, but only if the last operation finished.

class MyViewModel : ViewModel() {
    private val updateChannel = Channel<List<Long>>(capacity = 1, onBufferOverflow = BufferOverflow.DROP_OLDEST)

    init {
        viewModelScope.launch(Dispatchers.Default) {
            updateChannel.consumeEach { ids ->
                searchAndReplace(ids)
            }
        }
    }

    fun fetchFreshTodoItemDetails(idsToRefresh: List<Long>) {
        updateChannel.trySend(idsToRefresh)
    }

    private suspend fun searchAndReplace(ids: List<Long>) {
        TODO("The logic you had previously")
    }
}

You can of course tweak this according to your use case. Hope this was useful and gave you a new perspective on the issue at hand.

Upvotes: 9

Thracian
Thracian

Reputation: 67293

When i build similar set up it works fine. ConcurrentModification issue comes from list when you try to remove or add items in a for or foreach loop. Solution is to use iterator.

val myCollection = remember {
    mutableStateListOf<Int>().apply {
        repeat(10){
            add(it)
        }
    }
}

// Throws exception
myCollection.forEachIndexed { index, item ->
    if (index == 2) {
        myCollection.remove(item)
    }
}

// Works fine
val iterator = myCollection.iterator()
while(iterator.hasNext()){
    val item = iterator.next()
    if(item == 2){
        iterator.remove()
    }
}

Upvotes: 2

Related Questions