diff --git a/libraries/stdlib/jvm/src/kotlin/collections/builders/ListBuilder.kt b/libraries/stdlib/jvm/src/kotlin/collections/builders/ListBuilder.kt index 83594d86771..4b17489f92f 100644 --- a/libraries/stdlib/jvm/src/kotlin/collections/builders/ListBuilder.kt +++ b/libraries/stdlib/jvm/src/kotlin/collections/builders/ListBuilder.kt @@ -179,6 +179,10 @@ internal class ListBuilder private constructor( // ---------------------------- private ---------------------------- + private fun registerModification() { + modCount += 1 + } + private fun checkIsMutable() { if (isEffectivelyReadOnly) throw UnsupportedOperationException() } @@ -209,6 +213,7 @@ internal class ListBuilder 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 private constructor( } private fun addAllInternal(i: Int, elements: Collection, n: Int) { + registerModification() if (backing != null) { backing.addAllInternal(i, elements, n) array = backing.array @@ -236,6 +242,7 @@ internal class ListBuilder 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 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 private constructor( /** Retains elements if [retain] == true and removes them it [retain] == false. */ private fun retainOrRemoveAllInternal(rangeOffset: Int, rangeLength: Int, elements: Collection, retain: Boolean): Int { - if (backing != null) { - val removed = backing.retainOrRemoveAllInternal(rangeOffset, rangeLength, elements, retain) - length -= removed - return removed + val removed = if (backing != null) { + backing.retainOrRemoveAllInternal(rangeOffset, rangeLength, elements, retain) } else { var i = 0 var j = 0 @@ -278,20 +284,24 @@ internal class ListBuilder 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 - return removed + removed } + if (removed > 0) registerModification() + length -= removed + return removed } private class Itr : MutableListIterator { private val list: ListBuilder private var index: Int private var lastIndex: Int + private var expectedModCount: Int constructor(list: ListBuilder, 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 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() } } } diff --git a/libraries/stdlib/native-wasm/src/kotlin/collections/AbstractMutableList.kt b/libraries/stdlib/native-wasm/src/kotlin/collections/AbstractMutableList.kt index e1d0dc2525e..30452fa8140 100644 --- a/libraries/stdlib/native-wasm/src/kotlin/collections/AbstractMutableList.kt +++ b/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 protected actual constructor() : AbstractMutableCollection(), MutableList { + /** + * 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 protected actual constructor private open inner class IteratorImpl : MutableIterator { /** 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 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 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 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 protected actual constructor val result = list.removeAt(fromIndex + index) _size-- + modCount = list.modCount return result } diff --git a/libraries/stdlib/native-wasm/src/kotlin/collections/ArrayList.kt b/libraries/stdlib/native-wasm/src/kotlin/collections/ArrayList.kt index 486a34386a1..047b72ae172 100644 --- a/libraries/stdlib/native-wasm/src/kotlin/collections/ArrayList.kt +++ b/libraries/stdlib/native-wasm/src/kotlin/collections/ArrayList.kt @@ -160,12 +160,14 @@ actual class ArrayList 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 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 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 private constructor( } private fun addAllInternal(i: Int, elements: Collection, n: Int) { + registerModification() if (backingList != null) { backingList.addAllInternal(i, elements, n) backingArray = backingList.backingArray @@ -259,6 +267,7 @@ actual class ArrayList 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 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 private constructor( /** Retains elements if [retain] == true and removes them it [retain] == false. */ private fun retainOrRemoveAllInternal(rangeOffset: Int, rangeLength: Int, elements: Collection, retain: Boolean): Int { - if (backingList != null) { - val removed = backingList.retainOrRemoveAllInternal(rangeOffset, rangeLength, elements, retain) - length -= removed - return removed + val removed = if (backingList != null) { + backingList.retainOrRemoveAllInternal(rangeOffset, rangeLength, elements, retain) } else { var i = 0 var j = 0 @@ -301,20 +309,24 @@ actual class ArrayList 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 - return removed + removed } + if (removed > 0) registerModification() + length -= removed + return removed } private class Itr : MutableListIterator { private val list: ArrayList private var index: Int private var lastIndex: Int + private var expectedModCount: Int constructor(list: ArrayList, 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 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() } } } diff --git a/libraries/stdlib/test/collections/ConcurrentModificationTest.kt b/libraries/stdlib/test/collections/ConcurrentModificationTest.kt new file mode 100644 index 00000000000..1dfc6e50dd7 --- /dev/null +++ b/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 > testThrowsCME( + withCollection: WithCollection, + createIterator: C.() -> I, + collectionOp: CollectionOperation, + iteratorOp: IteratorOperation + ) { + 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(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 > testThrowsCME( + withMutableList: WithCollection, + listOps: List> + ) { + for (listOp in listOps) { + for (iteratorOp in iteratorOperations()) { + 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("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>) { + 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(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("trimToSize()") { trimToSize() }, + CollectionOperation("ensureCapacity(minCapacity > capacity)") { ensureCapacity(100) }, + CollectionOperation("ensureCapacity(minCapacity < capacity)") { ensureCapacity(1) }, + CollectionOperation("ensureCapacity(minCapacity == size)") { ensureCapacity(size) }, + ) + + fun testThrowsCME(withArrayList: WithCollection>) { + testThrowsCME(withArrayList, operations) + } + + // size == capacity + testThrowsCME { action -> + ArrayList(4).apply { + addAll(listOf("a", "b", "c", "d")) + action(this) + } + } + testThrowsCME { action -> + arrayListOf("a", "b", "c", "d").also(action) + } + + // size < capacity + testThrowsCME { action -> + ArrayList(10).apply { + addAll(listOf("a", "b", "c", "d")) + action(this) + } + } + } +} + +private typealias WithCollection = (action: (C) -> Unit) -> Unit + +private class CollectionOperation( + val description: String, + val throwsCME: Boolean = true, + val function: C.() -> Unit +) + +private class IteratorOperation( + val description: String, + val precedingFunction: (I.() -> Unit)? = null, + val function: I.() -> Unit +) + +private fun iteratorOperations() = listOf>>( + IteratorOperation("next()") { next() }, + IteratorOperation("remove()", { next() }) { remove() } +) +private val listIteratorOperations = listOf>>( + IteratorOperation("next()") { next() }, + IteratorOperation("remove()", { next() }) { remove() }, + IteratorOperation("previous()", { next() }) { previous() }, + IteratorOperation("add(\"e\")") { add("e") } +) \ No newline at end of file