Mutex for Kotlin/Common #508

bbrockbernd · 2025-02-07T09:50:59Z

A FIFO reentrant mutex for Kotlin/Common.
Depends on Thread parking for Kotlin/Common #498 (Parking logic is under review there)

dkhalanskyjb · 2025-02-07T10:42:50Z

It would be better to set native-thread-parking as the base branch of this PR. It's more difficult to review just the mutex when the changes related to parking are also included.

atomicfu/src/commonMain/kotlin/kotlinx/atomicfu/locks/Mutex.kt

dkhalanskyjb · 2025-02-10T12:46:30Z

atomicfu/src/commonMain/kotlin/kotlinx/atomicfu/locks/Mutex.kt

+ * Multiplatform mutex.
+ * On native based on pthread system calls.
+ * On JVM delegates to ReentrantLock.
+ */


This is user-visible documentation, and so it must explain the contracts of the data structure. What is a mutex? What would an example of its usage look like? Is it reentrant? Is it fair? It should mention that it's unsuitable for async (suspend) code, blocks the thread, and is only suitable for low-level technical work.

The same goes for every function in the file: all API entries need to be documented. When does each function return? When can it throw an exception? What does it do?

I have adjusted the docs, wdyt?

Great, thanks!

dkhalanskyjb · 2025-02-10T12:53:50Z

atomicfu/src/commonMain/kotlin/kotlinx/atomicfu/locks/Mutex.kt

+ * On JVM delegates to ReentrantLock.
+ */
+expect class Mutex() {
+    fun isLocked(): Boolean


Even if it turns out someone does need this function, they can do fun Mutex.isLocked() = if (tryLock()) { unlock(); true } else false (right?), so including this function is purely a performance optimization for a performance problem that may not even exist. So, unless we know of some specific use cases for this function, let's not take on the extra commitment of including it.

That is correct. I have tried to keep the api similar to the coroutines mutex's api. But will remove it for now.

P.S. true and false should be swapped right?

On a second thought, this won't work if I am holding the lock myself since it is reentrant.

atomicfu/src/concurrentMain/kotlin/kotlinx/atomicfu/locks/Mutex.kt

atomicfu/src/jsAndWasmSharedMain/kotlin/kotlinx/atomicfu/locks/Mutex.jsAndWasmShared.kt

atomicfu/src/concurrentMain/kotlin/kotlinx/atomicfu/locks/Mutex.kt

dkhalanskyjb · 2025-02-10T13:22:07Z

atomicfu/src/commonMain/kotlin/kotlinx/atomicfu/locks/Mutex.kt

+/**
+ * Multiplatform mutex.
+ * On native based on pthread system calls.
+ * On JVM delegates to ReentrantLock.


This doesn't seem accurate, though? ReentrantLock does get used in synchronized and kotlinx.atomicfu.ReentrantLock, but not in Mutex, right?

If so, there seems to be some inconsistency: on the one hand, atomicfu.ReentrantLock delegates to Java's ReentrantLock, on the other, atomicfu.Mutex doesn't, whereas on Native, atomicfu.ReentrantLock and atomcifu.Mutex are basically the same thing.
If we believe that our mutex is better, we should use it throughout; if we don't, we should use ReentrantLock throughout. Are there measurements showing which one performs better?

atomicfu/api/atomicfu.api

dkhalanskyjb · 2025-02-11T11:42:30Z

atomicfu/src/commonMain/kotlin/kotlinx/atomicfu/locks/Mutex.kt

+ * Multiplatform mutex.
+ * On native based on pthread system calls.
+ * On JVM delegates to ReentrantLock.
+ */


Great, thanks!

atomicfu/src/commonMain/kotlin/kotlinx/atomicfu/locks/Mutex.kt

atomicfu/src/jsAndWasmSharedMain/kotlin/kotlinx/atomicfu/locks/Mutex.jsAndWasmShared.kt

dkhalanskyjb · 2025-06-20T10:56:49Z

atomicfu/api/atomicfu.klib.api

-
-    final fun lock() // kotlinx.atomicfu.locks/NativeMutexNode.lock|lock(){}[0]
-    final fun unlock() // kotlinx.atomicfu.locks/NativeMutexNode.unlock|unlock(){}[0]
-}


I assume removing this implementation detail is safe, but cc @fzhinkin: he probably knows if this was already used by someone else.

atomicfu/build.gradle.kts

atomicfu/src/commonMain/kotlin/kotlinx/atomicfu/locks/SynchronousMutex.kt

atomicfu/src/concurrentMain/kotlin/kotlinx/atomicfu/locks/NativeMutex.kt

dkhalanskyjb · 2025-06-25T13:27:06Z

atomicfu/src/concurrentMain/kotlin/kotlinx/atomicfu/locks/NativeMutex.kt

+                // Dequeueing woken nodes can lead to issues when pre-unparked.
+                parkingQueue.dequeue()
+                nextParker = parkingQueue.getHead()
+            }


I don't fully get it. Take a lock in one thread, then await the lock with a small timeout in another thread, time out. state == 2. unlock is called, currentState == 1. nodeWake fails, and decrementAndGet() returns 0, after which we exit without dequeueing anything at all. Repeat that a hundred times, and you get a hundred useless nodes in the queue. Is this a memory leak, or do I misunderstand some logic?

Good catch! It indeed needs to be either getAndDecrement or >= 0. I have added some validation logic to the lincheck tests to check that the queue is indeed always empty when all operations are finished.

Looks like this change broke correctness (tests fail). I'd expect getAndDecrement >= 0 to be a reason to dequeue the caller, but not a reason to attempt to wake another one up.

…tion logic to lincheck tests to check that queue is empty after each execution.

…ggestions

dkhalanskyjb

Well done! All I have left are stylistic nitpicks.

dkhalanskyjb · 2025-07-11T13:33:15Z

atomicfu/src/jvmTest/kotlin/kotlinx/atomicfu/locks/NativeMutexTimeoutLincheckTest.kt

+
+class NativeMutexTimeoutLincheckTest {
+    class Counter {
+        @Volatile


I wonder what this does. Since Counter is guarded by a lock, what purpose does Volatile serve?

dkhalanskyjb · 2025-07-11T13:34:56Z

atomicfu/src/jvmTest/kotlin/kotlinx/atomicfu/locks/NativeMutexReentrantLincheckTest.kt

+
+class NativeMutexReentrantLincheckTest {
+    class Counter {
+        @Volatile


dkhalanskyjb · 2025-07-11T13:35:16Z

atomicfu/src/jvmTest/kotlin/kotlinx/atomicfu/locks/NativeMutexLincheckTest.kt

+
+class NativeMutexLincheckTest {
+    class Counter {
+        @Volatile


dkhalanskyjb · 2025-07-11T13:37:41Z

atomicfu/src/jsAndWasmSharedMain/kotlin/kotlinx/atomicfu/locks/Mutex.jsAndWasmShared.kt

+    public actual fun tryLock(timeout: Duration): Boolean = true
+    public actual fun lock(): Unit { state++ }
+    public actual fun unlock(): Unit { 
+        if (state == 0) throw IllegalStateException("Mutex already unlocked") 


Style: here and everywhere else where we have if (cond) throw IllegalStateException(str), it can be replaced with check(!cond) { str }, which looks nicer.

dkhalanskyjb · 2025-07-11T13:42:39Z

atomicfu/src/concurrentTest/kotlin/kotlinx/atomicfu/locks/VaryingContentionTest.kt

+                t.lockInt.lock()
+                if (t.lockInt.n % t.mod == t.id) t.lockInt.n++
+                if (t.lockInt.n >= t.max) {
+                    t.lockInt.unlock()
+                    break
+                }
+                t.lockInt.unlock()


Suggested change

t.lockInt.lock()

if (t.lockInt.n % t.mod == t.id) t.lockInt.n++

if (t.lockInt.n >= t.max) {

t.lockInt.unlock()

break

}

t.lockInt.unlock()

t.lockInt.lock()

try {

if (t.lockInt.n % t.mod == t.id) t.lockInt.n++

if (t.lockInt.n >= t.max) break

} finally {

t.lockInt.unlock()

}

is a somewhat more idiomatic way to express the same thing.

dkhalanskyjb · 2025-07-11T13:46:41Z

atomicfu/src/concurrentTest/kotlin/kotlinx/atomicfu/locks/VaryingContentionTest.kt

+    data class LockIntTest(
+        val lockInt: LockInt,
+        val max: Int,
+        val mod: Int,
+        val id: Int,
+    )


These can just be function parameters, I think, so there's no need for this whole class, right?

dkhalanskyjb · 2025-07-11T13:46:49Z

atomicfu/src/concurrentTest/kotlin/kotlinx/atomicfu/locks/VaryingContentionTest.kt

+        val futureList = mutableListOf<Fut>()
+        repeat(nThreads) { i ->
+            val test = LockIntTest(lockInt, countTo, nThreads, i)
+            futureList.add(testWithThread(test))
+        }


Suggested change

val futureList = mutableListOf<Fut>()

repeat(nThreads) { i ->

val test = LockIntTest(lockInt, countTo, nThreads, i)

futureList.add(testWithThread(test))

}

val futureList = List(nThreads) { i ->

testWithThread(LockIntTest(lockInt, countTo, nThreads, i))

}

dkhalanskyjb · 2025-07-11T13:50:51Z

atomicfu/src/concurrentTest/kotlin/kotlinx/atomicfu/locks/NativeMutexTest.kt

+private const val N_REPEATS_SLOW = 100
+private const val N_REPEATS_FAST = 30_000 
+
+class NativeMutexTest {


These two tests look basically the same, the only two parameters that make them different are the number of repetitions (100 vs 30_000) and the length of the sleep between adding two values (10 vs 0 milliseconds). Seems worth it to abstract this logic into one function that gets called from two tests.

dkhalanskyjb · 2025-07-11T13:54:35Z

atomicfu/src/concurrentTest/kotlin/kotlinx/atomicfu/locks/LockWithTimoutTests.kt

+        val threads = mutableListOf<TestThread>()
+
+        repeat(N_THREADS) { threadId ->
+            val thread = testThread {
+                while (counter.value < TARGET_COUNT) {
+                    // Try to acquire the lock with a timeout
+                    if (mutex.tryLock(getRandomTimeout().milliseconds)) {
+                        try {
+                            // Increment the counter if lock was acquired
+                            if (counter.value < TARGET_COUNT) {
+                                counter.incrementAndGet()
+                            }
+                            // Random sleep after increment to increase variation
+                            sleepMillis(getRandomWait())
+                        } finally {
+                            mutex.unlock()
+                        }
+                    }
+
+                    // Random sleep between increment attempts to increase variation
+                    sleepMillis(getRandomWait())
+                }
+            }
+            threads.add(thread)
+        }


Suggested change

val threads = mutableListOf<TestThread>()

repeat(N_THREADS) { threadId ->

val thread = testThread {

while (counter.value < TARGET_COUNT) {

// Try to acquire the lock with a timeout

if (mutex.tryLock(getRandomTimeout().milliseconds)) {

try {

// Increment the counter if lock was acquired

if (counter.value < TARGET_COUNT) {

counter.incrementAndGet()

}

// Random sleep after increment to increase variation

sleepMillis(getRandomWait())

} finally {

mutex.unlock()

}

}

// Random sleep between increment attempts to increase variation

sleepMillis(getRandomWait())

}

}

threads.add(thread)

}

val threads = List(N_THREADS) { threadId ->

testThread {

while (counter.value < TARGET_COUNT) {

// Try to acquire the lock with a timeout

if (mutex.tryLock(getRandomTimeout().milliseconds)) {

try {

// Increment the counter if lock was acquired

if (counter.value < TARGET_COUNT) {

counter.incrementAndGet()

}

// Random sleep after increment to increase variation

sleepMillis(getRandomWait())

} finally {

mutex.unlock()

}

}

// Random sleep between increment attempts to increase variation

sleepMillis(getRandomWait())

}

}

}

bbrockbernd requested review from ndkoval and dkhalanskyjb February 7, 2025 09:50

bbrockbernd mentioned this pull request Feb 7, 2025

Mutex for Kotlin Common #494

Closed

bbrockbernd changed the base branch from master to native-thread-parking February 7, 2025 10:49

pjBooms mentioned this pull request Feb 7, 2025

Potentially harfmul SynchronizedObject usages on iOS platform Kotlin/kotlinx.coroutines#4282

Open

dkhalanskyjb requested changes Feb 10, 2025

View reviewed changes

atomicfu/src/concurrentMain/kotlin/kotlinx/atomicfu/locks/Mutex.kt Outdated Show resolved Hide resolved

dkhalanskyjb requested changes Feb 10, 2025

View reviewed changes

bbrockbernd requested a review from dkhalanskyjb February 10, 2025 16:07

dkhalanskyjb reviewed Feb 11, 2025

View reviewed changes

bbrockbernd force-pushed the bbrockbernd/common-mutex branch from 71dfd4b to 28f0d71 Compare March 13, 2025 14:42

bbrockbernd force-pushed the bbrockbernd/common-mutex branch 2 times, most recently from bfa21fb to cdc1df9 Compare April 25, 2025 14:18

bbrockbernd force-pushed the native-thread-parking branch from 519e59d to 0d9ea07 Compare April 29, 2025 11:59

bbrockbernd force-pushed the bbrockbernd/common-mutex branch from cdc1df9 to f9c6167 Compare May 1, 2025 08:57

bbrockbernd added 6 commits June 3, 2025 10:27

Introduce mutex

e1bd019

Minor improvements

a4d1a3b

Revert lincheck version

00187e5

Fix inconsistent package defs

2c11bc5

Lazy init native mutex in lincheck tests

464a124

Add comment

7a35ef7

bbrockbernd force-pushed the bbrockbernd/common-mutex branch from f9c6167 to 7a35ef7 Compare June 3, 2025 13:21

Exclude lincheck tests from the transformed test jobs

2ace4c7

bbrockbernd changed the base branch from native-thread-parking to develop June 4, 2025 12:44

bbrockbernd requested a review from dkhalanskyjb June 4, 2025 12:45

dkhalanskyjb requested changes Jun 25, 2025

View reviewed changes

bbrockbernd added 3 commits July 2, 2025 08:38

Fixed node not being dequed properly after timeout. Also added valida…

536b81c

…tion logic to lincheck tests to check that queue is empty after each execution.

Fix jsAndWasm comments, remove redundant return, apply terminology su…

6c745aa

…ggestions

Turn redundant ifs into checks, assert for native is still experimental.

9301433

bbrockbernd added 2 commits July 2, 2025 10:56

Exclude all tests from transformation that end with "LincheckTest"

43d0892

Bugfix mutex

5768a08

bbrockbernd requested a review from dkhalanskyjb July 2, 2025 17:57

dkhalanskyjb requested a review from fzhinkin July 3, 2025 06:40

dkhalanskyjb approved these changes Jul 11, 2025

View reviewed changes

fzhinkin self-assigned this Jul 11, 2025

Mutex for Kotlin/Common #508

Are you sure you want to change the base?

Mutex for Kotlin/Common #508

Uh oh!

Conversation

bbrockbernd commented Feb 7, 2025

Uh oh!

dkhalanskyjb commented Feb 7, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbrockbernd Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkhalanskyjb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bbrockbernd Feb 10, 2025 •

edited

Loading