Hi! I've been thinking about whether or not this c...

# codereview

[Ali Khaleqi Yekta]

Hi! I've been thinking about whether or not this code is thread-safe in practice (in a sense that the output of get is always random).

import kotlin.concurrent.Volatile

object RandomPool {
  private val writeLock = ReentrantLock()
  private val random = kotlin.random.Random.Default

  @Volatile private var array = FloatArray(0)
  private var _array = array

  operator fun get(index: Int): Float {
    // Available in the thread-local cached array
    val local = _array
    if (index < local.size) {
      return local[index]
    }

    // Available in general
    val latest = array
    if (index < latest.size) {
      _array = latest
      return latest[index]
    }

    // Not available, need to grow
    ensureCapacity(index + 1)
    return array[index]
  }

  fun ensureCapacity(capacity: Int) =
    writeLock.withLock {
      // Double-Checked Locking: After acquiring the lock, check the condition again.
      // Another thread might have already grown the array while this thread was waiting.
      val currentArray = array
      if (capacity <= currentArray.size) return@withLock

      // This thread has the lock and has confirmed the array is still too small.
      // Proceed with the resizing operation.
      val oldSize = currentArray.size
      // Grow the array with some extra capacity to reduce the frequency of re-allocations.
      val newSize = maxOf(capacity, (1.5 * oldSize).toInt())

      // Copy-on-Write: Create and populate the new array completely before publishing it.
      val newArray = currentArray.copyOf(newSize)
      for (i in oldSize until newSize) {
        newArray[i] = random.nextFloat()
      }

      array = newArray
      _array = newArray
    }
}

I believe in this case, the _array should be an acceptable optimization and shouldn't cause any wrong output being returned, but asking AI, it keeps on insisting otherwise, and points out that there actually is a way that _array can return `0.0`(not randomly, but as an uninitialized value). The array is only ever set to completely filled arrays, and the _array is only updated afterwards, plus the values themselves never change. So, I don't seem to see any problem. Do you know if it's actually wrong, or the AI is hallucinating?

[loke]

I see some issues here. First of all, you don't have any thread-local variable there. You have a variable declared @Volatile , but that's a very different thing.

[loke]

Instead of double-checked locking, I'd probably use a CompareAndSet.

[loke]

Finally, you don't have a write barrier when updating newArray, meaning that the content may not have propagated from cache by the time another thread reads the data.

[loke]

So yes, you could actually read 0.0.

[loke]

Probably won't happen on x86, but can happen on Arm, if I understand the memory semantics correctly.

[Ali Khaleqi Yekta]

@loke Insightful, thanks!
I'm not dealing with this scenario anymore, but out of curiosity, doesn't "not being propagated from cache" mean the variable can potentially hold an older reference, meaning a reference to a different previous array? In this case, shouldn't the value be retrieved from that older array and not be 0.0?

[loke]

Yes. When you update an object, the value is written to cache (and eventually memory), but only the same thread is guaranteed to read back the same value after writing. However, the reference to the object that holds that value (i.e. the array) is immediately propagated to all other threads, since it's volatile.

[loke]

There is a special rule that says that all updates to an object during its construction will be propagated before the reference to the object is seen. But that only applies to the constructor, and not in your case, since you're updating the array after it's created.

[loke]

I would really not recommend messing around with this stuff unless you truly understand it, and even then the benefit is minimal. These kinds of tricks are only useful if you have a very high thread contention, which is very rare. It's better just to use locks to protect all accesses (reads and writes).

[loke]

Oh, and don't think you can just remove the volatile to make things work. Ordering is not guaranteed (except that I think it works on x86, because it has different guarantees than Arm).

[Ali Khaleqi Yekta]

@loke Thanks for the detailed explanation! Yes, I do agree that I've been a prematurely optimizing way too much. It's definitely not needed in a typical high-level system, and I just realized how much there is that can go wrong, or worse, only goes wrong in some architectures!

[loke]

@Ali Khaleqi Yekta Right. That's the really tricky part. You can't really test for it.