From fead5a87547039bbec1323fcd61ad00b1a989a94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matej=20Drobni=C4=8D?= Date: Tue, 5 May 2026 09:49:03 +0200 Subject: [PATCH 1/3] fix SideEffects.shouldBeEmpty() crashing --- .../com/adidas/mvi/sideeffects/SideEffects.kt | 21 ++++++++++++------- .../adidas/mvi/sideeffects/SideEffectsTest.kt | 13 ++++++++++++ 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/mvi/src/commonMain/kotlin/com/adidas/mvi/sideeffects/SideEffects.kt b/mvi/src/commonMain/kotlin/com/adidas/mvi/sideeffects/SideEffects.kt index 8d83727..cd818f5 100644 --- a/mvi/src/commonMain/kotlin/com/adidas/mvi/sideeffects/SideEffects.kt +++ b/mvi/src/commonMain/kotlin/com/adidas/mvi/sideeffects/SideEffects.kt @@ -27,13 +27,18 @@ public class SideEffects() : Iterable { return SideEffects() } - override fun iterator(): Iterator = - iterator { - while (true) { - val currentList = sideEffects.value - if (currentList.isEmpty()) break - val nextSideEffect = currentList.removeFirstOrNull() - nextSideEffect?.let { yield(it) } - } + override fun iterator(): Iterator { + return SideEffectsIterator() + } + + private inner class SideEffectsIterator : Iterator { + override fun next(): T { + return sideEffects.value.removeFirst() + } + + override fun hasNext(): Boolean { + return sideEffects.value.isNotEmpty() } + + } } diff --git a/mvi/src/jvmTest/kotlin/com/adidas/mvi/sideeffects/SideEffectsTest.kt b/mvi/src/jvmTest/kotlin/com/adidas/mvi/sideeffects/SideEffectsTest.kt index 3b5f813..0dd132a 100644 --- a/mvi/src/jvmTest/kotlin/com/adidas/mvi/sideeffects/SideEffectsTest.kt +++ b/mvi/src/jvmTest/kotlin/com/adidas/mvi/sideeffects/SideEffectsTest.kt @@ -1,10 +1,12 @@ package com.adidas.mvi.sideeffects +import io.kotest.assertions.throwables.shouldThrow import io.kotest.core.spec.style.BehaviorSpec import io.kotest.matchers.booleans.shouldBeFalse import io.kotest.matchers.collections.shouldBeEmpty import io.kotest.matchers.collections.shouldContain import io.kotest.matchers.collections.shouldContainInOrder +import io.kotest.matchers.collections.shouldNotBeEmpty import kotlin.time.DurationUnit import kotlin.time.ExperimentalTime import kotlin.time.toDuration @@ -36,6 +38,17 @@ internal class SideEffectsTest : BehaviorSpec({ } } + `when`("I add one SideEffect with empty check") { + val addedSideEffect = TestSideEffect() + val returnedSideEffectContainer = sideEffects.add(addedSideEffect) + + then("The SideEffect empty check should not crash") { + shouldThrow { + returnedSideEffectContainer.shouldBeEmpty() + } + } + } + `when`("I add two SideEffects") { val firstSideEffect = TestSideEffect() val secondSideEffect = TestSideEffect() From f432962eccb69e72cc005d88d5a24d42f3811e92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matej=20Drobni=C4=8D?= Date: Tue, 5 May 2026 10:08:14 +0200 Subject: [PATCH 2/3] reformat --- .../commonMain/kotlin/com/adidas/mvi/sideeffects/SideEffects.kt | 1 - .../jvmTest/kotlin/com/adidas/mvi/sideeffects/SideEffectsTest.kt | 1 - 2 files changed, 2 deletions(-) diff --git a/mvi/src/commonMain/kotlin/com/adidas/mvi/sideeffects/SideEffects.kt b/mvi/src/commonMain/kotlin/com/adidas/mvi/sideeffects/SideEffects.kt index cd818f5..fa57037 100644 --- a/mvi/src/commonMain/kotlin/com/adidas/mvi/sideeffects/SideEffects.kt +++ b/mvi/src/commonMain/kotlin/com/adidas/mvi/sideeffects/SideEffects.kt @@ -39,6 +39,5 @@ public class SideEffects() : Iterable { override fun hasNext(): Boolean { return sideEffects.value.isNotEmpty() } - } } diff --git a/mvi/src/jvmTest/kotlin/com/adidas/mvi/sideeffects/SideEffectsTest.kt b/mvi/src/jvmTest/kotlin/com/adidas/mvi/sideeffects/SideEffectsTest.kt index 0dd132a..d999f17 100644 --- a/mvi/src/jvmTest/kotlin/com/adidas/mvi/sideeffects/SideEffectsTest.kt +++ b/mvi/src/jvmTest/kotlin/com/adidas/mvi/sideeffects/SideEffectsTest.kt @@ -6,7 +6,6 @@ import io.kotest.matchers.booleans.shouldBeFalse import io.kotest.matchers.collections.shouldBeEmpty import io.kotest.matchers.collections.shouldContain import io.kotest.matchers.collections.shouldContainInOrder -import io.kotest.matchers.collections.shouldNotBeEmpty import kotlin.time.DurationUnit import kotlin.time.ExperimentalTime import kotlin.time.toDuration From 45f49862faba9464fd7a601c1f9051d2b0ca7d14 Mon Sep 17 00:00:00 2001 From: Matej Drobnic Date: Thu, 7 May 2026 08:40:42 +0200 Subject: [PATCH 3/3] migrate to a better thread-safe implementation Co-authored-by: jzeferino --- .../com/adidas/mvi/sideeffects/SideEffects.kt | 25 ++------- .../adidas/mvi/sideeffects/SideEffectsTest.kt | 55 ------------------- 2 files changed, 5 insertions(+), 75 deletions(-) diff --git a/mvi/src/commonMain/kotlin/com/adidas/mvi/sideeffects/SideEffects.kt b/mvi/src/commonMain/kotlin/com/adidas/mvi/sideeffects/SideEffects.kt index fa57037..3f936c3 100644 --- a/mvi/src/commonMain/kotlin/com/adidas/mvi/sideeffects/SideEffects.kt +++ b/mvi/src/commonMain/kotlin/com/adidas/mvi/sideeffects/SideEffects.kt @@ -9,18 +9,13 @@ import kotlinx.atomicfu.atomic * It locks itself, so you can't add and read at the same time, also it's not possible to read it at the same time from different threads, being completely thread-safe. */ -public class SideEffects() : Iterable { - private val sideEffects: AtomicRef> = atomic(ArrayList()) +public class SideEffectsprivate constructor(sideEffects: List) : Iterable { + private val sideEffects: AtomicRef> = atomic(sideEffects) - // Private constructor to initialize from an Iterable - private constructor(sideEffects: Iterable) : this() { - this.sideEffects.value.addAll(sideEffects) - } + public constructor() : this(emptyList()) public fun add(vararg sideEffectsToAdd: T): SideEffects { - val newList = sideEffects.value.toMutableList() - newList.addAll(sideEffectsToAdd) - return SideEffects(newList) + return SideEffects(sideEffects.value + sideEffectsToAdd) } public fun clear(): SideEffects { @@ -28,16 +23,6 @@ public class SideEffects() : Iterable { } override fun iterator(): Iterator { - return SideEffectsIterator() - } - - private inner class SideEffectsIterator : Iterator { - override fun next(): T { - return sideEffects.value.removeFirst() - } - - override fun hasNext(): Boolean { - return sideEffects.value.isNotEmpty() - } + return sideEffects.getAndSet(emptyList()).iterator() } } diff --git a/mvi/src/jvmTest/kotlin/com/adidas/mvi/sideeffects/SideEffectsTest.kt b/mvi/src/jvmTest/kotlin/com/adidas/mvi/sideeffects/SideEffectsTest.kt index d999f17..e92613f 100644 --- a/mvi/src/jvmTest/kotlin/com/adidas/mvi/sideeffects/SideEffectsTest.kt +++ b/mvi/src/jvmTest/kotlin/com/adidas/mvi/sideeffects/SideEffectsTest.kt @@ -1,17 +1,10 @@ package com.adidas.mvi.sideeffects -import io.kotest.assertions.throwables.shouldThrow import io.kotest.core.spec.style.BehaviorSpec -import io.kotest.matchers.booleans.shouldBeFalse import io.kotest.matchers.collections.shouldBeEmpty import io.kotest.matchers.collections.shouldContain import io.kotest.matchers.collections.shouldContainInOrder -import kotlin.time.DurationUnit import kotlin.time.ExperimentalTime -import kotlin.time.toDuration -import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.launch -import kotlinx.coroutines.sync.Semaphore @ExperimentalTime internal class SideEffectsTest : BehaviorSpec({ @@ -37,17 +30,6 @@ internal class SideEffectsTest : BehaviorSpec({ } } - `when`("I add one SideEffect with empty check") { - val addedSideEffect = TestSideEffect() - val returnedSideEffectContainer = sideEffects.add(addedSideEffect) - - then("The SideEffect empty check should not crash") { - shouldThrow { - returnedSideEffectContainer.shouldBeEmpty() - } - } - } - `when`("I add two SideEffects") { val firstSideEffect = TestSideEffect() val secondSideEffect = TestSideEffect() @@ -72,42 +54,5 @@ internal class SideEffectsTest : BehaviorSpec({ clearedSideEffects.shouldBeEmpty() } } - - `when`("I try to read SideEffects and it takes time, simulated by a semaphore") { - val firstSideEffect = TestSideEffect() - val secondSideEffectToBeAddedLater = TestSideEffect() - - var returnedSideEffects = sideEffects.add(firstSideEffect) - - val semaphore = Semaphore(2) - - val readJob = - launch(Dispatchers.Default) { - returnedSideEffects.forEach { _ -> - semaphore.acquire() // Wait for the signal - } - } - - val addJob = - launch(Dispatchers.Default) { - returnedSideEffects = sideEffects.add(secondSideEffectToBeAddedLater) - } - - semaphore.release() - - then("It should be released only by the semaphore").config( - timeout = - 5.toDuration( - DurationUnit.SECONDS, - ), - ) { - readJob.join() - addJob.join() - - readJob.isActive.shouldBeFalse() - addJob.isActive.shouldBeFalse() - returnedSideEffects.shouldContain(secondSideEffectToBeAddedLater) - } - } } })