From af1630e270d630a04942e7799fd58f69c71297b1 Mon Sep 17 00:00:00 2001 From: Abduqodiri Qurbonzoda Date: Thu, 25 May 2023 03:59:49 +0300 Subject: [PATCH] [K/N] Catch concurrent modifications of HashMap As a part of efforts to stabilize Native stdlib. --- .../kotlin/collections/builders/MapBuilder.kt | 31 +++++ .../src/kotlin/collections/HashMap.kt | 50 ++++++- .../collections/ConcurrentModificationTest.kt | 122 ++++++++++++++++++ 3 files changed, 197 insertions(+), 6 deletions(-) diff --git a/libraries/stdlib/jvm/src/kotlin/collections/builders/MapBuilder.kt b/libraries/stdlib/jvm/src/kotlin/collections/builders/MapBuilder.kt index ab91032a273..921a6219a5b 100644 --- a/libraries/stdlib/jvm/src/kotlin/collections/builders/MapBuilder.kt +++ b/libraries/stdlib/jvm/src/kotlin/collections/builders/MapBuilder.kt @@ -25,6 +25,18 @@ internal class MapBuilder private constructor( ) : MutableMap, Serializable { private var hashShift: Int = computeShift(hashSize) + /** + * The number of times this map is structurally modified. + * + * A modification is considered to be structural if it changes the map size, + * or otherwise changes it in a way that iterations in progress may return incorrect results. + * + * This value can be used by iterators of the [keys], [values] and [entries] views + * to provide fail-fast behavoir when a concurrent modification is detected during iteration. + * [ConcurrentModificationException] will be thrown in this case. + */ + private var modCount: Int = 0 + override var size: Int = 0 private set @@ -112,6 +124,7 @@ internal class MapBuilder private constructor( valuesArray?.resetRange(0, length) size = 0 length = 0 + registerModification() } override val keys: MutableSet get() { @@ -176,6 +189,10 @@ internal class MapBuilder private constructor( internal val capacity: Int get() = keysArray.size private val hashSize: Int get() = hashArray.size + private fun registerModification() { + modCount += 1 + } + internal fun checkIsMutable() { if (isReadOnly) throw UnsupportedOperationException() } @@ -237,6 +254,7 @@ internal class MapBuilder private constructor( } private fun rehash(newHashSize: Int) { + registerModification() if (length > size) compact() if (newHashSize != hashSize) { hashArray = IntArray(newHashSize) @@ -308,6 +326,7 @@ internal class MapBuilder private constructor( presenceArray[putIndex] = hash hashArray[hash] = putIndex + 1 size++ + registerModification() if (probeDistance > maxProbeDistance) maxProbeDistance = probeDistance return putIndex } @@ -336,6 +355,7 @@ internal class MapBuilder private constructor( removeHashAt(presenceArray[index]) presenceArray[index] = TOMBSTONE size-- + registerModification() } private fun removeHashAt(removedHash: Int) { @@ -477,6 +497,7 @@ internal class MapBuilder private constructor( ) { internal var index = 0 internal var lastIndex: Int = -1 + private var expectedModCount: Int = map.modCount init { initNext() @@ -490,15 +511,23 @@ internal class MapBuilder private constructor( fun hasNext(): Boolean = index < map.length fun remove() { + checkForComodification() check(lastIndex != -1) { "Call next() before removing element from the iterator." } map.checkIsMutable() map.removeKeyAt(lastIndex) lastIndex = -1 + expectedModCount = map.modCount + } + + internal fun checkForComodification() { + if (map.modCount != expectedModCount) + throw ConcurrentModificationException() } } internal class KeysItr(map: MapBuilder) : Itr(map), MutableIterator { override fun next(): K { + checkForComodification() if (index >= map.length) throw NoSuchElementException() lastIndex = index++ val result = map.keysArray[lastIndex] @@ -510,6 +539,7 @@ internal class MapBuilder private constructor( internal class ValuesItr(map: MapBuilder) : Itr(map), MutableIterator { override fun next(): V { + checkForComodification() if (index >= map.length) throw NoSuchElementException() lastIndex = index++ val result = map.valuesArray!![lastIndex] @@ -521,6 +551,7 @@ internal class MapBuilder private constructor( internal class EntriesItr(map: MapBuilder) : Itr(map), MutableIterator> { override fun next(): EntryRef { + checkForComodification() if (index >= map.length) throw NoSuchElementException() lastIndex = index++ val result = EntryRef(map, lastIndex) diff --git a/libraries/stdlib/native-wasm/src/kotlin/collections/HashMap.kt b/libraries/stdlib/native-wasm/src/kotlin/collections/HashMap.kt index a1956b25f29..0862edd6855 100644 --- a/libraries/stdlib/native-wasm/src/kotlin/collections/HashMap.kt +++ b/libraries/stdlib/native-wasm/src/kotlin/collections/HashMap.kt @@ -10,15 +10,33 @@ import kotlin.native.FreezingIsDeprecated @OptIn(FreezingIsDeprecated::class) actual class HashMap private constructor( - private var keysArray: Array, - private var valuesArray: Array?, // allocated only when actually used, always null in pure HashSet - private var presenceArray: IntArray, - private var hashArray: IntArray, - private var maxProbeDistance: Int, - private var length: Int + // keys in insert order + private var keysArray: Array, + // values in insert order, allocated only when actually used, always null in pure HashSet + private var valuesArray: Array?, + // hash of a key by its index, -1 if a key at that index was removed + private var presenceArray: IntArray, + // (index + 1) of a key by its hash, 0 if there is no key with that hash, -1 if collision chain continues to the hash-1 + private var hashArray: IntArray, + // max length of a collision chain + private var maxProbeDistance: Int, + // index of the next key to be inserted + private var length: Int ) : MutableMap { private var hashShift: Int = computeShift(hashSize) + /** + * The number of times this map is structurally modified. + * + * A modification is considered to be structural if it changes the map size, + * or otherwise changes it in a way that iterations in progress may return incorrect results. + * + * This value can be used by iterators of the [keys], [values] and [entries] views + * to provide fail-fast behavoir when a concurrent modification is detected during iteration. + * [ConcurrentModificationException] will be thrown in this case. + */ + private var modCount: Int = 0 + private var _size: Int = 0 override actual val size: Int get() = _size @@ -140,6 +158,7 @@ actual class HashMap private constructor( valuesArray?.resetRange(0, length) _size = 0 length = 0 + registerModification() } override actual val keys: MutableSet get() { @@ -206,6 +225,10 @@ actual class HashMap private constructor( private val capacity: Int get() = keysArray.size private val hashSize: Int get() = hashArray.size + private fun registerModification() { + modCount += 1 + } + internal fun checkIsMutable() { if (isReadOnly) throw UnsupportedOperationException() } @@ -268,6 +291,7 @@ actual class HashMap private constructor( } private fun rehash(newHashSize: Int) { + registerModification() if (length > _size) compact() if (newHashSize != hashSize) { hashArray = IntArray(newHashSize) @@ -339,6 +363,7 @@ actual class HashMap private constructor( presenceArray[putIndex] = hash hashArray[hash] = putIndex + 1 _size++ + registerModification() if (probeDistance > maxProbeDistance) maxProbeDistance = probeDistance return putIndex } @@ -367,6 +392,7 @@ actual class HashMap private constructor( removeHashAt(presenceArray[index]) presenceArray[index] = TOMBSTONE _size-- + registerModification() } private fun removeHashAt(removedHash: Int) { @@ -534,6 +560,7 @@ actual class HashMap private constructor( ) { internal var index = 0 internal var lastIndex: Int = -1 + private var expectedModCount: Int = map.modCount init { initNext() @@ -547,14 +574,23 @@ actual class HashMap private constructor( fun hasNext(): Boolean = index < map.length fun remove() { + checkForComodification() + check(lastIndex != -1) { "Call next() before removing element from the iterator." } map.checkIsMutable() map.removeKeyAt(lastIndex) lastIndex = -1 + expectedModCount = map.modCount + } + + internal fun checkForComodification() { + if (map.modCount != expectedModCount) + throw ConcurrentModificationException() } } internal class KeysItr(map: HashMap) : Itr(map), MutableIterator { override fun next(): K { + checkForComodification() if (index >= map.length) throw NoSuchElementException() lastIndex = index++ val result = map.keysArray[lastIndex] @@ -566,6 +602,7 @@ actual class HashMap private constructor( internal class ValuesItr(map: HashMap) : Itr(map), MutableIterator { override fun next(): V { + checkForComodification() if (index >= map.length) throw NoSuchElementException() lastIndex = index++ val result = map.valuesArray!![lastIndex] @@ -577,6 +614,7 @@ actual class HashMap private constructor( internal class EntriesItr(map: HashMap) : Itr(map), MutableIterator> { override fun next(): EntryRef { + checkForComodification() if (index >= map.length) throw NoSuchElementException() lastIndex = index++ val result = EntryRef(map, lastIndex) diff --git a/libraries/stdlib/test/collections/ConcurrentModificationTest.kt b/libraries/stdlib/test/collections/ConcurrentModificationTest.kt index 1dfc6e50dd7..ec33bfeafa0 100644 --- a/libraries/stdlib/test/collections/ConcurrentModificationTest.kt +++ b/libraries/stdlib/test/collections/ConcurrentModificationTest.kt @@ -166,6 +166,128 @@ class ConcurrentModificationTest { } } } + + @Test + fun mutableSet() { + if (TestPlatform.current == TestPlatform.Js) return + + val operations = listOf>>( + CollectionOperation("add(non-existing)") { add("e") }, + CollectionOperation("add(existing)", throwsCME = false) { add("d") }, + + CollectionOperation("remove(non-existing)", throwsCME = false) { remove("e") }, + CollectionOperation("remove(existing)") { remove("d") }, + + CollectionOperation("addAll(emptyList())", throwsCME = false) { addAll(emptyList()) }, + CollectionOperation("addAll(existing)", throwsCME = false) { addAll(listOf("d", "b")) }, + CollectionOperation("addAll(some exist)") { addAll(listOf("d", "e")) }, + CollectionOperation("addAll(non-existing)") { addAll(listOf("e", "f")) }, + + 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.toMutableSet())", throwsCME = false) { retainAll(this.toMutableSet()) }, + 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() } }, + ) + + fun testThrowsCME(withMutableSet: WithCollection>) { + for (setOp in operations) { + for (iteratorOp in iteratorOperations()) { + testThrowsCME(withMutableSet, { iterator() }, setOp, iteratorOp) + } + } + } + + // Because platform implementations may have different load factor and rehash strategy, + // make sure there is enough capacity to avoid rehash. + + val elements = listOf("a", "b", "c", "d") + + testThrowsCME { action -> + LinkedHashSet(10).apply { + addAll(elements) + action(this) + } + } + testThrowsCME { action -> + HashSet(10).apply { + addAll(elements) + action(this) + } + } + + testThrowsCME { action -> + buildSet(10) { + addAll(elements) + action(this) + } + } + } + + @Test + fun mutableMap() { + if (TestPlatform.current == TestPlatform.Js) return + + val operations = listOf>>( + CollectionOperation("put(non-existing)") { put("e", "e") }, + CollectionOperation("put(existing)", throwsCME = false) { put("d", "d") }, + CollectionOperation("put(update value)", throwsCME = false) { put("d", "D") }, + + CollectionOperation("remove(non-existing)", throwsCME = false) { remove("e") }, + CollectionOperation("remove(existing)") { remove("d") }, + + CollectionOperation("putAll(emptyList())", throwsCME = false) { putAll(emptyMap()) }, + CollectionOperation("putAll(existing)", throwsCME = false) { putAll(mapOf("d" to "d", "b" to "b")) }, + CollectionOperation("putAll(update values)", throwsCME = false) { putAll(mapOf("d" to "D", "b" to "B")) }, + CollectionOperation("putAll(some exist)") { putAll(mapOf("d" to "d", "e" to "e")) }, + CollectionOperation("putAll(non-existing)") { putAll(mapOf("e" to "e", "f" to "f")) }, + + CollectionOperation("clear()") { clear() }, + CollectionOperation("iterator.remove()") { iterator().apply { next(); remove() } }, + ) + + fun testThrowsCME(withMutableMap: WithCollection>) { + for (mapOp in operations) { + for (iteratorOp in iteratorOperations()) { + testThrowsCME(withMutableMap, { keys.iterator() }, mapOp, iteratorOp) + testThrowsCME(withMutableMap, { values.iterator() }, mapOp, iteratorOp) + } + for (iteratorOp in iteratorOperations>()) { + testThrowsCME(withMutableMap, { entries.iterator() }, mapOp, iteratorOp) + } + } + } + + // Because platform implementations may have different load factor and rehash strategy, + // make sure there is enough capacity to avoid rehash. + + val entries = mapOf("a" to "a", "b" to "b", "c" to "c", "d" to "d") + + testThrowsCME { action -> + LinkedHashMap(10).apply { + putAll(entries) + action(this) + } + } + testThrowsCME { action -> + HashMap(10).apply { + putAll(entries) + action(this) + } + } + testThrowsCME { action -> + buildMap(10) { + putAll(entries) + action(this) + } + } + } } private typealias WithCollection = (action: (C) -> Unit) -> Unit