Skip to content

Commit a61c5ce

Browse files
author
Abduqodiri Qurbonzoda
committed
Fix VectorBuilder comodification bug in set operation
1 parent 280fdaf commit a61c5ce

File tree

2 files changed

+21
-1
lines changed

2 files changed

+21
-1
lines changed

kotlinx-collections-immutable/src/main/kotlin/kotlinx/collections/immutable/implementations/immutableList/PersistentVectorBuilder.kt

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -380,6 +380,10 @@ class PersistentVectorBuilder<E>(private var vector: PersistentList<E>,
380380
checkElementIndex(index, size)
381381
if (rootSize() <= index) {
382382
val mutableTail = makeMutable(tail)
383+
384+
// Creating new tail implies structural change.
385+
if (mutableTail !== tail) { modCount++ }
386+
383387
val tailIndex = index and MAX_BUFFER_SIZE_MINUS_ONE
384388
val oldElement = mutableTail[tailIndex]
385389
mutableTail[tailIndex] = element
@@ -399,6 +403,12 @@ class PersistentVectorBuilder<E>(private var vector: PersistentList<E>,
399403
val mutableRoot = makeMutable(root)
400404

401405
if (shift == 0) {
406+
// Creating new leaf implies structural change.
407+
// Actually, while descending to this leaf several nodes could be recreated.
408+
// However, this builder is exclusive owner of this leaf iff it is exclusive owner of all leaf's ancestors.
409+
// Hence, checking recreation of this leaf is enough to determine if a structural change occurred.
410+
if (mutableRoot !== root) { modCount++ }
411+
402412
oldElementCarry.value = mutableRoot[bufferIndex]
403413
mutableRoot[bufferIndex] = e
404414
return mutableRoot

kotlinx-collections-immutable/tests/src/test/kotlin/kotlinx.collections.immutable/contractTests/immutableList/ImmutableListTest.kt

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,17 @@ class ImmutableListTest {
146146
@Test fun subListOfBuilder() {
147147
val list = "abcxaxyz12".toImmutableList().toPersistentList()
148148
val builder = list.builder()
149-
val subList = builder.subList(2, 5)
149+
150+
// builder needs to recreate the inner trie to apply the modification.
151+
// So, structural changes in builder causes CME on subList iteration.
152+
var subList = builder.subList(2, 5)
153+
builder[4] = 'x'
154+
assertFailsWith<ConcurrentModificationException> { subList.joinToString("") }
155+
156+
// builder is the exclusive owner of the inner trie.
157+
// So, `set(index, value)` doesn't lead to structural changes.
158+
subList = builder.subList(2, 5)
159+
assertEquals("cxx", subList.joinToString(""))
150160
builder[4] = 'b'
151161
assertEquals("cxb", subList.joinToString(""))
152162
subList.removeAt(0)

0 commit comments

Comments
 (0)