Hi everyone :slightly_smiling_face: Kinda noobish ...

# getting-started

[Jérémy CROS]

Hi everyone 🙂
Kinda noobish question regarding concurrent list access...
Basically, we have a mutable list that can be updated from multiple threads so we've added a mutex to protect against issues, something like that:

private val list = mutableListOf<T>()
private val mutex = Mutex()

override suspend fun addData(data: List<T>) {
    mutex.withLock {
        list.add(data)
    }
}

Every once in a while, we're saving this data to a file in a string format so we have:

private fun getLog(data: List<T>): String = data.mapNotNull { it.format() }.joinToString(" ")

(here, we're passing the above mutable list as parameter data)
And this particular function sometimes (rarely) generates crashlytics reports with the exception ConcurrentModificationException

So, 2 questions really 😅 How can this function throw this particular exception when, as far as I can tell, it's not modifying anything?
And, what's the best way to make this more robust?
Thanks! 😊

[Sergey Dmitriev]

mutableListOf uses ArrayList under the hood and its Iterator.next() can throw ConcurrentModificationException — and you are using the iterator when mapNotNull

Make a copy of the list before writing it to file

val newList = ArrayList<String>(originalList)

[Jérémy CROS]

hmmm, ok, makes sense (i mean, not really, the exception kinda is misleading I feel but I now understand what the issue is 😁)
is the creation of the copy list "safe"? Or do I also need to wrap that in the mutex?

[Sergey Dmitriev]

public ArrayList(Collection<? extends E> c) {
        Object[] a = c.toArray();
        if ((size = a.length) != 0) {
            if (c.getClass() == ArrayList.class) {
                elementData = a;
            } else {
                elementData = Arrays.copyOf(a, size, Object[].class);
            }
        } else {
            // replace with empty array.
            elementData = EMPTY_ELEMENTDATA;
        }
    }

It looks safe 🙂

[ephemient]

that's not guaranteed to be safe either, you might read a changed array before the element itself has been published to other threads

[ephemient]

read the list under the same mutex lock, or switch to a concurrent collection like CopyOnWriteArrayList

[Sergey Dmitriev]

Yeah, I meant safe from ConcurrentModificationException

[Jérémy CROS]

Just to make sure I get this right 🙂 Either of those are equally fine, correct ?

val safeList = mutex.withLock { unsafeList.toList() }
val safeList = CopyOnWriteArrayList(unsafeList)

[ephemient]

no, 1 is good but 2 is the same as ArrayList(unsafeList). if you change the declaration itself to

private val list = CopyOnWriteArrayList<T>()

you will not need mutex

[Sergey Dmitriev]

Also for 1 you can simply guard your getLog with mutex instead

[Sergey Dmitriev]

private suspend fun getLog(data: List<T>): String {
  return mutex.withLock {
    data.mapNotNull { it.format() }.joinToString(" ")
  }
}

[Sergey Dmitriev]

So you can keep using non-concurrent list

[Jérémy CROS]

oh ok, makes sense 👍
hmmmm, dropping the mutex and the need for suspend looks "convenient" in that particular case 🤔
I guess using CopyOnWriteArrayList comes with a performance cost?

[ephemient]

yes, every write to the list will sync and copy the whole underlying array

[Jérémy CROS]

no surprise I guess 😅
ok, I'll weigh my options then but there's a fix at least.
thanks both of you for the comprehensive answers! 🙏

[Joffrey]

Just a note about the exception name: it's about the fact that a modification happened concurrently with whatever you were doing (writing or reading). So it's ok to get it when reading

[ephemient]

well, it's not unreasonable to get it while reading. but it does mean you're doing something unsafe
(also the lack of CME does not mean what you're doing is safe)

[Arjan van Wieringen]

Does kotlinx peristent collections help here? Place that one behind a mutex and update it? No full copy

[ephemient]

persistent collections have other trade-offs, typically amortized O(log n) access instead of O(1). performance will depend on how it's being used

[Ronny Bräunlich]

There's also java.util.Collections#synchronizedList(java.util.List<T>) you could use.

[ephemient]

synchronizedList doesn't synchronize the iterator so it doesn't help with CME while mapping

[ephemient]

you could wrap it in

synchronized(list) {
    list.mapNotNull { ... }
}

but you can't mix that and `suspend`ing functions