ConcurrentModificationException v4.1.1

[shirozatou]

Sorry for submit an issue again instead of PR.

Well, forEach over a non-local MutableCollection probably not a good idea.

Stack trace

Caused by: java.util.ConcurrentModificationException
    at java.util.LinkedHashMap$LinkedHashIterator.nextNode(LinkedHashMap.java:1061)
    at java.util.LinkedHashMap$LinkedKeyIterator.next(LinkedHashMap.java:1084)
    at com.sebaslogen.resaca.ScopedViewModelContainer.scheduleToDisposeAfterReturningFromBackground(ScopedViewModelContainer.kt:453)
    at com.sebaslogen.resaca.ScopedViewModelContainer.onStateChanged(ScopedViewModelContainer.kt:370)
    at androidx.lifecycle.LifecycleRegistry$ObserverWithState.dispatchEvent(LifecycleRegistry.jvm.kt:320)
        ......

Related to https://github.com/sebaslogen/resaca/issues/129#issuecomment-2310893529

[sebaslogen]

No problem about the PR, thanks for reporting, that's already helpful 👍

The ConcurrentModificationException makes sense, that's why the code always used a ConcurrentSkipListSet in previous resaca versions, but unfortunately that's not possible anymore in KMP, so I'm trying to find a solution that works consistently across platforms. Do you know any good multiplatform alternative to ConcurrentSkipListSet ?

The KISS and hopefully safe solution I can think of for the forEach loop is to just grab a local copy of the keys in the collection and then iterate over it. If the key is not in the collection by the time scheduleToDispose is called, it can be safely ignored.

Something like

    fun scheduleToDisposeAfterReturningFromBackground() {
        val keys = safeConcurrentMarkForDisposalMutation { markedForDisposal.toList() }
        keys.forEach { key ->
            scheduleToDispose(key)
        }
    }

Let me know if you have suggestions.

[shirozatou]

So the problem is Composable function not thread safe? How about use viewModelScope + Dispatchers.Main ?

Blocking is heavy, so I would like to wrap all dispose operations on a single thread (UI Thread). If order of execution matter, probably create a collection snapshot.

BTW, I'm not sure it's a risk or not, In function getOrBuildViewModel, ViewModel#onCleared invoked directly from Composable function, it can be non-UI thread.

[sebaslogen]

So the problem is Composable function not thread safe? How about use viewModelScope + Dispatchers.Main ?

In theory the Compose team could run (re)compositions in a separate thread to improve performance but AFAIK nowadays everything still runs on MainThread.

Blocking is heavy, so I would like to wrap all dispose operations on a single thread (UI Thread). If order of execution matter, probably create a collection snapshot.

I like the suggestion, it should be faster, specially if the launch doesn't switch threads, although it's quite more work.

BTW, I'm not sure it's a risk or not, In function getOrBuildViewModel, ViewModel#onCleared invoked directly from Composable function, it can be non-UI thread.

You're right, there is no protection to check if Compose is calling for a MainThread or a background thread. In the parent abstract ViewModel there is no check for MainThread on onCleared, but the clear function that is internally used to call onCleared does enforce MainThread, so even if only for consistency it's a good idea to call onCleared also from MainThread. Good catch!

Thanks for the feedback, I appreciate it.

[shirozatou]

The deadlock(blocking Main thread) might have been solved by get rid of locks, but forEach over MutableSet still a bug that need to be fixed.

[sebaslogen]

The deadlock(blocking Main thread) might have been solved by get rid of locks, but forEach over MutableSet still a bug that need to be fixed.

Thanks, I totally forgot that with the concurrency thing.

Fixed in https://github.com/sebaslogen/resaca/commit/64aa3ce18cb8254115c38174e8e57f860eafb5ae