I'm trying to build a threadsafe counter. I stumbled upon this code, but it's apparently not thread ...

# getting-started

[Colton Idle]

I'm trying to build a threadsafe counter. I stumbled upon this code, but it's apparently not thread safe unless I add @Synchornozied to the inc() method. Why is that?

class Counter {
  @Volatile
  private var value = 0

  fun inc(): Int {
    return ++value
  }
  fun get(): Int {
    return value
  }
}

[Joffrey]

++value is not an atomic operation. It first reads the value and then writes value + 1 back into the value variable. Between these 2 operations, another thread could change the value, and this change would be overwritten.

[ephemient]

++value

is basically syntactic sugar for

value = value.inc()

(well slightly more complex but https://kotlinlang.org/spec/kotlin-spec.html#prefix-increment-expressions has all the details)

[ephemient]

if you used AtomicIntegerFieldUpdater, you could atomically increment @Volatile value (or kotlinx.atomicfu which uses that under the hood), but ++ is not that

[Colton Idle]

So it seems as though adding @Synchornozied to inc the method "fixes" it. But in that case, would I be able to remove @Volatile?

[ephemient]

depends. you won't get torn reads or writes, but get() in other threads may be "out of date", unless you make that @Synchronized too

[ephemient]

class Counter {
  private var value = 0

  @Synchronized
  fun inc(): Int = ++value
  fun get(): Int = value
}

val counter = Counter()
thread {
  counter.inc()
}
while (counter.get() == 0) {} // might not terminate

[ephemient]

whereas any of these would guarantee that loop to terminate

class Counter {
  private var value = 0
  @Synchronized
  fun inc(): Int = ++value
  @Synchronized
  fun get(): Int = value
}

class Counter {
  @Volatile
  private var value = 0
  @Synchronized
  fun inc(): Int = ++value
  fun get(): Int = value
}

import java.util.concurrent.atomic.*
class Counter {
  private val value = AtomicInteger(0)
  fun inc(): Int = value.incrementAndGet()
  fun get(): Int = value.intValue()
}

import java.util.concurrent.atomic.*
class Counter {
  @Volatile
  private var value = 0
  fun inc(): Int = valueFU.incrementAndGet(this)
  fun get(): Int = value
  companion object {
    private val valueFU = AtomicIntegerFieldUpdater(Counter::class.java, "value")
  }
}

import kotlinx.atomicfu.*
class Counter {
  private val value = atomic(0)
  fun inc(): Int = value.incrementAndGet()
  fun get(): Int = value.value
}

[ephemient]

https://docs.oracle.com/javase/specs/jls/se21/html/jls-17.html#jls-17.4

[Youssef Shoaib [MOD]]

You could also use arrow.atomic to get multiplatform Atomics if you want

[Stephan Schröder]

you could also use ReentrantReadWriteLock which should be more efficient than using `synchronized`:

import java.util.concurrent.locks.ReentrantReadWriteLock
import kotlin.concurrent.read
import kotlin.concurrent.write

class Counter {
    private var value = 0
    private val lock = ReentrantReadWriteLock()

    fun inc(): Int = lock.write { ++value }
    fun get(): Int = lock.read { value }
}

assuming your code is for the JVM only.
More on locking on the JVM: here
Also notice that Kotlin has an extension functions withLock to work with Lock.
and read/write to work with ReentrantReadWriteLock (which I used in my example).

[Stephan Schröder]

I looked some more into synchronized vs ReentrantLock (though it doesn't mention ReentrantReadWriteLock which I used) and this answer from StackOverflow sounds sensible, basically stating that synchronized is a good default.

[ephemient]

the biggest issue with @Synchronized is that it is effectively synchronized(this) { ... }, which means that any outside code can easily introduce locking contention

[ephemient]

the pattern to avoid that is something like

private val lock = Any()
fun inc() = synchronized(lock) { ... }
fun get() = synchronized(lock) { ... }

but either way, monitorenter/monitorexit is fine in most cases

[ephemient]

Java's ReentrantLock has the overhead of dealing with, well, reentrancy

[CLOVIS]

@ephemient can you clarify why synchronized(this) would create contention whereas synchronized(lock) wouldn't? I don't see the difference.

[ephemient]

code you don't own could interfere with synchronized(counter)

[CLOVIS]

Ah, I see. It would be considered bad practice to do though, no?

[ephemient]

it would be, but it's a pattern used elsewhere in Java. plus separate lock objects lets you have finer-level control over locking different fields

[Colton Idle]

this was amazing. im glad i asked.

thanks all for teaching