Improve SideEffects thread safety

[matejdro]

PR improves the thread safety of the SideEffects

With the current version, if you call SideEffects.shouldBeEmpty(), it will crash with a NoSuchElementException.

It turns out that the implementation of the SideEffect's iterator did not conform to the Iterator's contract: Iterator.hasNext() should be a read-only idempotent operation, but in our implementation, we remove the found item immediately after it. kotest's shouldBeEmpty() relies on the proper behavior of this iterator to print proper error messages.

PR fixes this.

P.S.: there is also an AtomicRef there that seems to serve no purpose? It's essentially
a more expensive val?

[jzeferino]

the new iterator implementation has a thread-safety issue. removeFirst() and isNotEmpty() are called on a plain ArrayList without synchronization. two threads iterating concurrently can corrupt the list.

the AtomicRef doesn't help here because we never atomically swap the reference we just read .value and mutate the underlying mutable list directly.

the fix is to pair AtomicRef with immutable data. thread-safe without locks. my proposal:

public class SideEffects<T>() : Iterable<T> {
    private val sideEffects: AtomicRef<List<T>> = atomic(emptyList())

    private constructor(sideEffects: List<T>) : this() {
        this.sideEffects.value = sideEffects
    }

    public fun add(vararg sideEffectsToAdd: T): SideEffects<T> {
        return SideEffects(sideEffects.value + sideEffectsToAdd)
    }

    public fun clear(): SideEffects<T> {
        return SideEffects()
    }

    override fun iterator(): Iterator<T> {
        return sideEffects.getAndSet(emptyList()).iterator()
    }
}

[matejdro]

This proposal has a slight drawback: it does not support partial consumption e.g. when a view starts iterating on the side effects, it will wipe them out, even if it does not consume anything. But I think the better thread safety is worth it, I think this is a really edge edge case that should never happen. So I have updated the implementation with your proposal and removed a test for this behavior.

However, it also does not fix the issue in the title of this PR. shouldBeEmpty() works like this:

1. Checks emptiness via iterator.hasNext()
2. Attempts to get the element for the error message via iterable.first() which fails, because the list is empty.

However, I think this is a bug in the kotest that we shouldn't hack around to fix: kotest/kotest#6005

I will rename this PR to only update this implementation and leave the kotest issue.