[K/N] Catch concurrent modifications of ArrayList

As a part of efforts to stabilize Native stdlib.

libraries/stdlib/jvm/src/kotlin/collections/builders/ListBuilder.kt

@@ -179,6 +179,10 @@ internal class ListBuilder<E> private constructor(
 
     // ---------------------------- private ----------------------------
 
+    private fun registerModification() {
+        modCount += 1
+    }
+
     private fun checkIsMutable() {
         if (isEffectivelyReadOnly) throw UnsupportedOperationException()
     }
@@ -209,6 +213,7 @@ internal class ListBuilder<E> private constructor(
     }
 
     private fun addAtInternal(i: Int, element: E) {
+        registerModification()
         if (backing != null) {
             backing.addAtInternal(i, element)
             array = backing.array
@@ -220,6 +225,7 @@ internal class ListBuilder<E> private constructor(
     }
 
     private fun addAllInternal(i: Int, elements: Collection<E>, n: Int) {
+        registerModification()
         if (backing != null) {
             backing.addAllInternal(i, elements, n)
             array = backing.array
@@ -236,6 +242,7 @@ internal class ListBuilder<E> private constructor(
     }
 
     private fun removeAtInternal(i: Int): E {
+        registerModification()
         if (backing != null) {
             val old = backing.removeAtInternal(i)
             length--
@@ -250,6 +257,7 @@ internal class ListBuilder<E> private constructor(
     }
 
     private fun removeRangeInternal(rangeOffset: Int, rangeLength: Int) {
+        if (rangeLength > 0) registerModification()
         if (backing != null) {
             backing.removeRangeInternal(rangeOffset, rangeLength)
         } else {
@@ -261,10 +269,8 @@ internal class ListBuilder<E> private constructor(
 
     /** Retains elements if [retain] == true and removes them it [retain] == false. */
     private fun retainOrRemoveAllInternal(rangeOffset: Int, rangeLength: Int, elements: Collection<E>, retain: Boolean): Int {
-        if (backing != null) {
+        val removed = if (backing != null) {
-            val removed = backing.retainOrRemoveAllInternal(rangeOffset, rangeLength, elements, retain)
+            backing.retainOrRemoveAllInternal(rangeOffset, rangeLength, elements, retain)
-            length -= removed
-            return removed
         } else {
             var i = 0
             var j = 0
@@ -278,20 +284,24 @@ internal class ListBuilder<E> private constructor(
             val removed = rangeLength - j
             array.copyInto(array, startIndex = rangeOffset + rangeLength, endIndex = length, destinationOffset = rangeOffset + j)
             array.resetRange(fromIndex = length - removed, toIndex = length)
-            length -= removed
+            removed
-            return removed
         }
+        if (removed > 0) registerModification()
+        length -= removed
+        return removed
     }
 
     private class Itr<E> : MutableListIterator<E> {
         private val list: ListBuilder<E>
         private var index: Int
         private var lastIndex: Int
+        private var expectedModCount: Int
 
         constructor(list: ListBuilder<E>, index: Int) {
             this.list = list
             this.index = index
             this.lastIndex = -1
+            this.expectedModCount = list.modCount
         }
 
         override fun hasPrevious(): Boolean = index > 0
@@ -301,32 +311,44 @@ internal class ListBuilder<E> private constructor(
         override fun nextIndex(): Int = index
 
         override fun previous(): E {
+            checkForComodification()
             if (index <= 0) throw NoSuchElementException()
             lastIndex = --index
             return list.array[list.offset + lastIndex]
         }
 
         override fun next(): E {
+            checkForComodification()
             if (index >= list.length) throw NoSuchElementException()
             lastIndex = index++
             return list.array[list.offset + lastIndex]
         }
 
         override fun set(element: E) {
+            checkForComodification()
             check(lastIndex != -1) { "Call next() or previous() before replacing element from the iterator." }
             list.set(lastIndex, element)
         }
 
         override fun add(element: E) {
+            checkForComodification()
             list.add(index++, element)
             lastIndex = -1
+            expectedModCount = list.modCount
         }
 
         override fun remove() {
+            checkForComodification()
             check(lastIndex != -1) { "Call next() or previous() before removing element from the iterator." }
             list.removeAt(lastIndex)
             index = lastIndex
             lastIndex = -1
+            expectedModCount = list.modCount
+        }
+
+        private fun checkForComodification() {
+            if (list.modCount != expectedModCount)
+                throw ConcurrentModificationException()
         }
     }
 }

libraries/stdlib/native-wasm/src/kotlin/collections/AbstractMutableList.kt

@@ -18,6 +18,16 @@ package kotlin.collections
  * @param E the type of elements contained in the list. The list is invariant in its element type.
  */
 public actual abstract class AbstractMutableList<E> protected actual constructor() : AbstractMutableCollection<E>(), MutableList<E> {
+    /**
+     * The number of times this list is structurally modified.
+     *
+     * A modification is considered to be structural if it changes the list size,
+     * or otherwise changes it in a way that iterations in progress may return incorrect results.
+     *
+     * This value can be used by iterators returned by [iterator] and [listIterator]
+     * to provide fail-fast behavoir when a concurrent modification is detected during iteration.
+     * [ConcurrentModificationException] will be thrown in this case.
+     */
     protected var modCount: Int = 0
 
     abstract override fun add(index: Int, element: E): Unit
@@ -91,26 +101,41 @@ public actual abstract class AbstractMutableList<E> protected actual constructor
     private open inner class IteratorImpl : MutableIterator<E> {
         /** the index of the item that will be returned on the next call to [next]`()` */
         protected var index = 0
+
         /** the index of the item that was returned on the previous call to [next]`()`
          * or [ListIterator.previous]`()` (for `ListIterator`),
          * -1 if no such item exists
          */
         protected var last = -1
 
+        /**
+         * The [modCount] value that the backing list is expected to have.
+         * Otherwise, the iterator has detected concurrent modification.
+         */
+        protected var expectedModCount = modCount
+
         override fun hasNext(): Boolean = index < size
 
         override fun next(): E {
+            checkForComodification()
             if (!hasNext()) throw NoSuchElementException()
             last = index++
             return get(last)
         }
 
         override fun remove() {
+            checkForComodification()
             check(last != -1) { "Call next() or previous() before removing element from the iterator."}
 
             removeAt(last)
             index = last
             last = -1
+            expectedModCount = modCount
+        }
+
+        protected fun checkForComodification() {
+            if (modCount != expectedModCount)
+                throw ConcurrentModificationException()
         }
     }
 
@@ -129,6 +154,7 @@ public actual abstract class AbstractMutableList<E> protected actual constructor
         override fun nextIndex(): Int = index
 
         override fun previous(): E {
+            checkForComodification()
             if (!hasPrevious()) throw NoSuchElementException()
 
             last = --index
@@ -138,14 +164,18 @@ public actual abstract class AbstractMutableList<E> protected actual constructor
         override fun previousIndex(): Int = index - 1
 
         override fun add(element: E) {
+            checkForComodification()
             add(index, element)
             index++
             last = -1
+            expectedModCount = modCount
         }
 
         override fun set(element: E) {
+            checkForComodification()
             check(last != -1) { "Call next() or previous() before updating element value with the iterator."}
             this@AbstractMutableList[last] = element
+            expectedModCount = modCount
         }
     }
 
@@ -162,6 +192,7 @@ public actual abstract class AbstractMutableList<E> protected actual constructor
 
             list.add(fromIndex + index, element)
             _size++
+            modCount = list.modCount
         }
 
         override fun get(index: Int): E {
@@ -175,6 +206,7 @@ public actual abstract class AbstractMutableList<E> protected actual constructor
 
             val result = list.removeAt(fromIndex + index)
             _size--
+            modCount = list.modCount
             return result
         }
 

libraries/stdlib/native-wasm/src/kotlin/collections/ArrayList.kt

@@ -160,12 +160,14 @@ actual class ArrayList<E> private constructor(
 
     actual fun trimToSize() {
         if (backingList != null) throw IllegalStateException() // just in case somebody casts subList to ArrayList
+        registerModification()
         if (length < backingArray.size)
             backingArray = backingArray.copyOfUninitializedElements(length)
     }
 
     final actual fun ensureCapacity(minCapacity: Int) {
         if (backingList != null) throw IllegalStateException() // just in case somebody casts subList to ArrayList
+        registerModification()
         if (minCapacity <= backingArray.size) return
         ensureCapacityInternal(minCapacity)
     }
@@ -205,6 +207,10 @@ actual class ArrayList<E> private constructor(
 
     // ---------------------------- private ----------------------------
 
+    private fun registerModification() {
+        modCount += 1
+    }
+
     private fun checkIsMutable() {
         if (isReadOnly || root != null && root.isReadOnly) throw UnsupportedOperationException()
     }
@@ -232,6 +238,7 @@ actual class ArrayList<E> private constructor(
     }
 
     private fun addAtInternal(i: Int, element: E) {
+        registerModification()
         if (backingList != null) {
             backingList.addAtInternal(i, element)
             backingArray = backingList.backingArray
@@ -243,6 +250,7 @@ actual class ArrayList<E> private constructor(
     }
 
     private fun addAllInternal(i: Int, elements: Collection<E>, n: Int) {
+        registerModification()
         if (backingList != null) {
             backingList.addAllInternal(i, elements, n)
             backingArray = backingList.backingArray
@@ -259,6 +267,7 @@ actual class ArrayList<E> private constructor(
     }
 
     private fun removeAtInternal(i: Int): E {
+        registerModification()
         if (backingList != null) {
             val old = backingList.removeAtInternal(i)
             length--
@@ -273,6 +282,7 @@ actual class ArrayList<E> private constructor(
     }
 
     private fun removeRangeInternal(rangeOffset: Int, rangeLength: Int) {
+        if (rangeLength > 0) registerModification()
         if (backingList != null) {
             backingList.removeRangeInternal(rangeOffset, rangeLength)
         } else {
@@ -284,10 +294,8 @@ actual class ArrayList<E> private constructor(
 
     /** Retains elements if [retain] == true and removes them it [retain] == false. */
     private fun retainOrRemoveAllInternal(rangeOffset: Int, rangeLength: Int, elements: Collection<E>, retain: Boolean): Int {
-        if (backingList != null) {
+        val removed = if (backingList != null) {
-            val removed = backingList.retainOrRemoveAllInternal(rangeOffset, rangeLength, elements, retain)
+            backingList.retainOrRemoveAllInternal(rangeOffset, rangeLength, elements, retain)
-            length -= removed
-            return removed
         } else {
             var i = 0
             var j = 0
@@ -301,20 +309,24 @@ actual class ArrayList<E> private constructor(
             val removed = rangeLength - j
             backingArray.copyInto(backingArray, startIndex = rangeOffset + rangeLength, endIndex = length, destinationOffset = rangeOffset + j)
             backingArray.resetRange(fromIndex = length - removed, toIndex = length)
-            length -= removed
+            removed
-            return removed
         }
+        if (removed > 0) registerModification()
+        length -= removed
+        return removed
     }
 
     private class Itr<E> : MutableListIterator<E> {
         private val list: ArrayList<E>
         private var index: Int
         private var lastIndex: Int
+        private var expectedModCount: Int
 
         constructor(list: ArrayList<E>, index: Int) {
             this.list = list
             this.index = index
             this.lastIndex = -1
+            this.expectedModCount = list.modCount
         }
 
         override fun hasPrevious(): Boolean = index > 0
@@ -324,32 +336,44 @@ actual class ArrayList<E> private constructor(
         override fun nextIndex(): Int = index
 
         override fun previous(): E {
+            checkForComodification()
             if (index <= 0) throw NoSuchElementException()
             lastIndex = --index
             return list.backingArray[list.offset + lastIndex]
         }
 
         override fun next(): E {
+            checkForComodification()
             if (index >= list.length) throw NoSuchElementException()
             lastIndex = index++
             return list.backingArray[list.offset + lastIndex]
         }
 
         override fun set(element: E) {
+            checkForComodification()
             check(lastIndex != -1) { "Call next() or previous() before replacing element from the iterator." }
             list.set(lastIndex, element)
         }
 
         override fun add(element: E) {
+            checkForComodification()
             list.add(index++, element)
             lastIndex = -1
+            expectedModCount = list.modCount
         }
 
         override fun remove() {
+            checkForComodification()
             check(lastIndex != -1) { "Call next() or previous() before removing element from the iterator." }
             list.removeAt(lastIndex)
             index = lastIndex
             lastIndex = -1
+            expectedModCount = list.modCount
+        }
+
+        private fun checkForComodification() {
+            if (list.modCount != expectedModCount)
+                throw ConcurrentModificationException()
         }
     }
 }

libraries/stdlib/test/collections/ConcurrentModificationTest.kt

@@ -0,0 +1,194 @@
+/*
+ * Copyright 2010-2023 JetBrains s.r.o. and Kotlin Programming Language contributors.
+ * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file.
+ */
+
+package test.collections
+
+import test.TestPlatform
+import test.current
+import kotlin.test.Test
+import kotlin.test.assertFailsWith
+import kotlin.test.assertTrue
+import kotlin.test.fail
+
+class ConcurrentModificationTest {
+
+    private fun <C, I : Iterator<*>> testThrowsCME(
+        withCollection: WithCollection<C>,
+        createIterator: C.() -> I,
+        collectionOp: CollectionOperation<C>,
+        iteratorOp: IteratorOperation<I>
+    ) {
+        var invoked = false
+        withCollection { collection ->
+            invoked = true
+
+            val iterator = collection.createIterator()
+
+            iteratorOp.precedingFunction?.invoke(iterator)
+            collectionOp.function.invoke(collection)
+
+            val message = "listOp: ${collectionOp.description}, iteratorOp: ${iteratorOp.description}"
+            if (collectionOp.throwsCME) {
+                assertFailsWith<ConcurrentModificationException>(message) {
+                    iteratorOp.function.invoke(iterator)
+                }
+            } else {
+                try {
+                    iteratorOp.function.invoke(iterator)
+                } catch (e: Throwable) {
+                    fail("$message. Expected no exception, but was $e")
+                }
+            }
+        }
+
+        assertTrue(invoked)
+    }
+
+    private fun <C : MutableList<String>> testThrowsCME(
+        withMutableList: WithCollection<C>,
+        listOps: List<CollectionOperation<C>>
+    ) {
+        for (listOp in listOps) {
+            for (iteratorOp in iteratorOperations<String>()) {
+                testThrowsCME(withMutableList, { iterator() }, listOp, iteratorOp)
+            }
+            for (iteratorOp in listIteratorOperations) {
+                testThrowsCME(withMutableList, { listIterator() }, listOp, iteratorOp)
+                testThrowsCME(withMutableList, { listIterator(2) }, listOp, iteratorOp)
+            }
+        }
+    }
+
+    @Test
+    fun mutableList() {
+        if (TestPlatform.current == TestPlatform.Js) return
+
+        val operations = listOf<CollectionOperation<MutableList<String>>>(
+            CollectionOperation("set()", throwsCME = false) { set(2, "e") },
+
+            CollectionOperation("add()") { add("e") },
+            CollectionOperation("add(index)") { add(2, "e") },
+
+            CollectionOperation("remove(non-existing)", throwsCME = false) { remove("e") },
+            CollectionOperation("remove(existing)") { remove("d") },
+            CollectionOperation("removeAt()") { removeAt(2) },
+
+            CollectionOperation("addAll()") { addAll(listOf("e", "f")) },
+            CollectionOperation("addAll(emptyList())") { addAll(emptyList()) },
+            CollectionOperation("addAll(index)") { addAll(2, listOf("e", "f")) },
+            CollectionOperation("addAll(index, emptyList())") { addAll(2, emptyList()) },
+
+            CollectionOperation("removeAll(non-existing)", throwsCME = false) { removeAll(listOf("e", "f")) },
+            CollectionOperation("removeAll(some exist)") { removeAll(listOf("d", "e")) },
+            CollectionOperation("removeAll(emptyList())", throwsCME = false) { removeAll(emptyList()) },
+
+            CollectionOperation("retainAll(this.toMutableList())", throwsCME = false) { retainAll(this.toMutableList()) },
+            CollectionOperation("retainAll(non-existing)") { retainAll(listOf("e", "f")) },
+            CollectionOperation("retainAll(some exist)") { retainAll(listOf("d", "e")) },
+            CollectionOperation("retainAll(emptyList())") { retainAll(emptyList()) },
+
+            CollectionOperation("clear()") { clear() },
+            CollectionOperation("iterator.remove()") { iterator().apply { next(); remove() } },
+        ).also { ops ->
+            ops + ops.map {
+                CollectionOperation("subList(1, size)." + it.description, it.throwsCME) { it.function.invoke(subList(1, size)) }
+            }
+        }
+
+        fun testThrowsCME(withMutableList: WithCollection<MutableList<String>>) {
+            testThrowsCME(withMutableList, operations)
+        }
+
+        // size == capacity
+        testThrowsCME { action ->
+            MutableList(4) { ('a' + it).toString() }.also(action)
+        }
+        testThrowsCME { action ->
+            mutableListOf("a", "b", "c", "d").also(action)
+        }
+
+        testThrowsCME { action ->
+            buildList(4) {
+                addAll(listOf("a", "b", "c", "d"))
+                action(this)
+            }
+        }
+
+        // size < capacity
+        testThrowsCME { action ->
+            ArrayList<String>(10).apply {
+                addAll(listOf("a", "b", "c", "d"))
+                action(this)
+            }
+        }
+
+        testThrowsCME { action ->
+            buildList(10) {
+                addAll(listOf("a", "b", "c", "d"))
+                action(this)
+            }
+        }
+    }
+
+    @Test
+    fun arrayList() {
+        if (TestPlatform.current == TestPlatform.Js) return
+
+        val operations = listOf<CollectionOperation<ArrayList<String>>>(
+            CollectionOperation("trimToSize()") { trimToSize() },
+            CollectionOperation("ensureCapacity(minCapacity > capacity)") { ensureCapacity(100) },
+            CollectionOperation("ensureCapacity(minCapacity < capacity)") { ensureCapacity(1) },
+            CollectionOperation("ensureCapacity(minCapacity == size)") { ensureCapacity(size) },
+        )
+
+        fun testThrowsCME(withArrayList: WithCollection<ArrayList<String>>) {
+            testThrowsCME(withArrayList, operations)
+        }
+
+        // size == capacity
+        testThrowsCME { action ->
+            ArrayList<String>(4).apply {
+                addAll(listOf("a", "b", "c", "d"))
+                action(this)
+            }
+        }
+        testThrowsCME { action ->
+            arrayListOf("a", "b", "c", "d").also(action)
+        }
+
+        // size < capacity
+        testThrowsCME { action ->
+            ArrayList<String>(10).apply {
+                addAll(listOf("a", "b", "c", "d"))
+                action(this)
+            }
+        }
+    }
+}
+
+private typealias WithCollection<C> = (action: (C) -> Unit) -> Unit
+
+private class CollectionOperation<C>(
+    val description: String,
+    val throwsCME: Boolean = true,
+    val function: C.() -> Unit
+)
+
+private class IteratorOperation<I>(
+    val description: String,
+    val precedingFunction: (I.() -> Unit)? = null,
+    val function: I.() -> Unit
+)
+
+private fun <E> iteratorOperations() = listOf<IteratorOperation<MutableIterator<E>>>(
+    IteratorOperation("next()") { next() },
+    IteratorOperation("remove()", { next() }) { remove() }
+)
+private val listIteratorOperations = listOf<IteratorOperation<MutableListIterator<String>>>(
+    IteratorOperation("next()") { next() },
+    IteratorOperation("remove()", { next() }) { remove() },
+    IteratorOperation("previous()", { next() }) { previous() },
+    IteratorOperation("add(\"e\")") { add("e") }
+)