From c9d8ecc5996cf4409dd9d5404f857e683ce96b49 Mon Sep 17 00:00:00 2001 From: Abduqodiri Qurbonzoda Date: Fri, 12 Jan 2024 18:46:10 +0500 Subject: [PATCH] Detect concurrent modifications in ArrayDeque #KT-63355 Remove the ArrayDeque.subList().subList() test for comodification detection. For types that extend AbstractList and to not override subList function, the behavior varies depending on JDK version. With JKD8, the test fails for ArrayDeque. While with JDK9+, it passes. --- .../kotlin/collections/AbstractMutableList.kt | 10 +++ .../src/kotlin/collections/ArrayDeque.kt | 23 ++++- .../collections/ConcurrentModificationTest.kt | 89 +++++++++++++++++-- 3 files changed, 113 insertions(+), 9 deletions(-) diff --git a/libraries/stdlib/native-wasm/src/kotlin/collections/AbstractMutableList.kt b/libraries/stdlib/native-wasm/src/kotlin/collections/AbstractMutableList.kt index 2b41ce2fbcc..7e868192ffe 100644 --- a/libraries/stdlib/native-wasm/src/kotlin/collections/AbstractMutableList.kt +++ b/libraries/stdlib/native-wasm/src/kotlin/collections/AbstractMutableList.kt @@ -234,6 +234,16 @@ public actual abstract class AbstractMutableList protected actual constructor return _size } + override fun iterator(): MutableIterator { + checkForComodification() + return super.iterator() + } + + override fun listIterator(index: Int): MutableListIterator { + checkForComodification() + return super.listIterator(index) + } + private fun checkForComodification() { if (list.modCount != modCount) throw ConcurrentModificationException() diff --git a/libraries/stdlib/src/kotlin/collections/ArrayDeque.kt b/libraries/stdlib/src/kotlin/collections/ArrayDeque.kt index 887e6f5ab27..fcb30b8fbae 100644 --- a/libraries/stdlib/src/kotlin/collections/ArrayDeque.kt +++ b/libraries/stdlib/src/kotlin/collections/ArrayDeque.kt @@ -120,6 +120,7 @@ public class ArrayDeque : AbstractMutableList { * Prepends the specified [element] to this deque. */ public fun addFirst(element: E) { + registerModification() ensureCapacity(size + 1) head = decremented(head) @@ -131,6 +132,7 @@ public class ArrayDeque : AbstractMutableList { * Appends the specified [element] to this deque. */ public fun addLast(element: E) { + registerModification() ensureCapacity(size + 1) elementData[internalIndex(size)] = element @@ -142,6 +144,7 @@ public class ArrayDeque : AbstractMutableList { */ public fun removeFirst(): E { if (isEmpty()) throw NoSuchElementException("ArrayDeque is empty.") + registerModification() val element = internalGet(head) elementData[head] = null @@ -160,6 +163,7 @@ public class ArrayDeque : AbstractMutableList { */ public fun removeLast(): E { if (isEmpty()) throw NoSuchElementException("ArrayDeque is empty.") + registerModification() val internalLastIndex = internalIndex(lastIndex) val element = internalGet(internalLastIndex) @@ -190,6 +194,7 @@ public class ArrayDeque : AbstractMutableList { return } + registerModification() ensureCapacity(size + 1) // Elements in circular array lay in 2 ways: @@ -269,6 +274,8 @@ public class ArrayDeque : AbstractMutableList { public override fun addAll(elements: Collection): Boolean { if (elements.isEmpty()) return false + + registerModification() ensureCapacity(this.size + elements.size) copyCollectionElements(internalIndex(size), elements) return true @@ -283,6 +290,7 @@ public class ArrayDeque : AbstractMutableList { return addAll(elements) } + registerModification() ensureCapacity(this.size + elements.size) val tail = internalIndex(size) @@ -424,6 +432,8 @@ public class ArrayDeque : AbstractMutableList { return removeFirst() } + registerModification() + val internalIndex = internalIndex(index) val element = internalGet(internalIndex) @@ -510,14 +520,18 @@ public class ArrayDeque : AbstractMutableList { } } } - if (modified) + if (modified) { + registerModification() size = negativeMod(newTail - head) + } return modified } public override fun clear() { if (isNotEmpty()) { + registerModification() + val tail = internalIndex(size) nullifyNonEmpty(head, tail) } @@ -549,7 +563,6 @@ public class ArrayDeque : AbstractMutableList { override fun removeRange(fromIndex: Int, toIndex: Int) { AbstractList.checkRangeIndexes(fromIndex, toIndex, size) - // TODO: registerModification() val length = toIndex - fromIndex when (length) { @@ -564,6 +577,8 @@ public class ArrayDeque : AbstractMutableList { } } + registerModification() + if (fromIndex < size - toIndex) { // closer to the first element -> shift preceding elements removeRangeShiftPreceding(fromIndex, toIndex) @@ -622,6 +637,10 @@ public class ArrayDeque : AbstractMutableList { } } + private fun registerModification() { + modCount += 1 + } + // for testing internal fun testToArray(array: Array): Array = toArray(array) internal fun testToArray(): Array = toArray() diff --git a/libraries/stdlib/test/collections/ConcurrentModificationTest.kt b/libraries/stdlib/test/collections/ConcurrentModificationTest.kt index 0bc6eacab44..240947852a2 100644 --- a/libraries/stdlib/test/collections/ConcurrentModificationTest.kt +++ b/libraries/stdlib/test/collections/ConcurrentModificationTest.kt @@ -95,7 +95,12 @@ class ConcurrentModificationTest { fun mutableList() { if (TestPlatform.current == TestPlatform.Js) return - val operations = listOf>>( + /** + * Some operations should register a modification by contract, but java ArrayList, + * whose implementation we can't change, do not register. + * @param isJavaArrayListBehavior specifies whether to test java ArrayList behavior or the behavior by contract. + */ + fun operations(isJavaArrayListBehavior: Boolean) = listOf>>( CollectionOperation("set()", throwsCME = false) { set(2, "e") }, CollectionOperation("add()") { add("e") }, @@ -106,9 +111,9 @@ class ConcurrentModificationTest { CollectionOperation("removeAt()") { removeAt(2) }, CollectionOperation("addAll()") { addAll(listOf("e", "f")) }, - CollectionOperation("addAll(emptyList())") { addAll(emptyList()) }, + CollectionOperation("addAll(emptyList())", throwsCME = isJavaArrayListBehavior) { addAll(emptyList()) }, CollectionOperation("addAll(index)") { addAll(2, listOf("e", "f")) }, - CollectionOperation("addAll(index, emptyList())") { addAll(2, emptyList()) }, + CollectionOperation("addAll(index, emptyList())", throwsCME = isJavaArrayListBehavior) { addAll(2, emptyList()) }, CollectionOperation("removeAll(non-existing)", throwsCME = false) { removeAll(listOf("e", "f")) }, CollectionOperation("removeAll(some exist)") { removeAll(listOf("d", "e")) }, @@ -127,8 +132,8 @@ class ConcurrentModificationTest { } } - fun testThrowsCME(withMutableList: WithCollection>) { - testIteratorThrowsCME(withMutableList, operations) + fun testThrowsCME(isJavaArrayListBehavior: Boolean = true, withMutableList: WithCollection>) { + testIteratorThrowsCME(withMutableList, operations(isJavaArrayListBehavior)) } // size == capacity @@ -146,6 +151,16 @@ class ConcurrentModificationTest { } } + testThrowsCME(isJavaArrayListBehavior = false) { action -> + ArrayDeque(4).apply { + addAll(listOf("a", "b", "c", "d")) + action(this) + } + } + testThrowsCME(isJavaArrayListBehavior = false) { action -> + ArrayDeque(listOf("a", "b", "c", "d")).also(action) + } + // size < capacity testThrowsCME { action -> ArrayList(10).apply { @@ -160,6 +175,13 @@ class ConcurrentModificationTest { action(this) } } + + testThrowsCME(isJavaArrayListBehavior = false) { action -> + ArrayDeque(10).apply { + addAll(listOf("a", "b", "c", "d")) + action(this) + } + } } @Test @@ -195,6 +217,45 @@ class ConcurrentModificationTest { } } + @Test + fun arrayDeque() { + if (TestPlatform.current == TestPlatform.Js) return + + val operations = listOf>>( + CollectionOperation("addFirst()") { addFirst("e") }, + CollectionOperation("addLast()") { addLast("e") }, + + CollectionOperation("removeFirst()") { removeFirst() }, + CollectionOperation("removeLast()") { removeLast() }, + + CollectionOperation("removeFirstOrNull()") { removeFirstOrNull() }, + CollectionOperation("removeLastOrNull()") { removeLastOrNull() }, + ) + + fun testThrowsCME(withArrayDeque: WithCollection>) { + testIteratorThrowsCME(withArrayDeque, operations) + } + + // size == capacity + testThrowsCME { action -> + ArrayDeque(4).apply { + addAll(listOf("a", "b", "c", "d")) + action(this) + } + } + testThrowsCME { action -> + ArrayDeque(listOf("a", "b", "c", "d")).also(action) + } + + // size < capacity + testThrowsCME { action -> + ArrayDeque(10).apply { + addAll(listOf("a", "b", "c", "d")) + action(this) + } + } + } + @Test fun subList() { if (TestPlatform.current == TestPlatform.Js) return @@ -230,8 +291,6 @@ class ConcurrentModificationTest { CollectionOperation("clear()") { clear() }, CollectionOperation("iterator()") { iterator() }, CollectionOperation("listIterator()") { listIterator() }, - - CollectionOperation("subList()", throwsCME = false) { subList(0, 1) }, ) fun testThrowsCME(withMutableList: WithCollection>) { @@ -244,6 +303,11 @@ class ConcurrentModificationTest { arrayList.add("e") action(subList) } + assertFailsWith { + val arrayList = arrayListOf("a", "b", "c", "d") + for (e in arrayList.subList(1, 3)) arrayList.remove(e) + } + testThrowsCME { action -> buildList { addAll(listOf("a", "b", "c", "d")) @@ -258,6 +322,17 @@ class ConcurrentModificationTest { for (e in subList(1, 3)) remove(e) } } + + testThrowsCME { action -> + val arrayDeque = ArrayDeque(listOf("a", "b", "c", "d")) + val subList = arrayDeque.subList(0, arrayDeque.size) + arrayDeque.add("e") + action(subList) + } + assertFailsWith { + val arrayDeque = ArrayDeque(listOf("a", "b", "c", "d")) + for (e in arrayDeque.subList(1, 3)) arrayDeque.remove(e) + } } @Test