PR Review comments: add user friendly docs for public api, delegate to ReentrantLock on JVM, inline and private funs where possible. Additionally removed legacy NativeNode.

Author: bbrockbernd

atomicfu/api/atomicfu.api

@@ -137,6 +137,8 @@ public final class kotlinx/atomicfu/TraceKt {
 
 public final class kotlinx/atomicfu/locks/Mutex {
 	public fun <init> ()V
+	public fun <init> (Ljava/util/concurrent/locks/ReentrantLock;)V
+	public final fun getReentrantLock ()Ljava/util/concurrent/locks/ReentrantLock;
 	public final fun isLocked ()Z
 	public final fun lock ()V
 	public final fun tryLock ()Z

atomicfu/src/androidNative32BitMain/kotlin/kotlinx/atomicfu/locks/NativeMutexNode.kt

@@ -5,19 +5,70 @@ import kotlin.contracts.InvocationKind
 import kotlin.contracts.contract
 
 /**
- * Multiplatform mutex.
- * On native based on pthread system calls.
- * On JVM delegates to ReentrantLock.
+ * Mutual exclusion for Kotlin Multiplatform.
+ * 
+ * It can protect a shared resource or critical section from multiple thread accesses.
+ * Threads can acquire the lock by calling [lock] and release the lock by calling [unlock].
+ * 
+ * When a thread calls [lock] while another thread is locked, it will suspend until the lock is released.
+ * When multiple threads are waiting for the lock, they will acquire it in a fair order (first in first out).
+ * 
+ * It is reentrant, meaning the lock holding thread can call [lock] multiple times without suspending.
+ * To release the lock (after multiple [lock] calls) an equal number of [unlock] calls are required.
+ * 
+ * This Mutex should not be used in combination with coroutines and `suspend` functions
+ * as it blocks the waiting thread.
+ * Use the `Mutex` from the coroutines library instead.
+ * 
+ * ```Kotlin
+ * mutex.withLock {
+ *     // Critical section only executed by
+ *     // one thread at a time.
+ * }
+ * ```
  */
 expect class Mutex() {
+    /**
+     * Returns `true` if this mutex is locked.
+     */
     fun isLocked(): Boolean
-    fun tryLock(): Boolean 
-    fun lock() 
+
+    /**
+     * Tries to lock this mutex, returning `false` if this mutex is already locked.
+     *
+     * It is recommended to use [withLock] for safety reasons, so that the acquired lock is always
+     * released at the end of your critical section, and [unlock] is never invoked before a successful
+     * lock acquisition.     
+     */
+    fun tryLock(): Boolean
+
+    /**
+     * Locks the mutex, suspends the thread until the lock is acquired.
+     * 
+     * It is recommended to use [withLock] for safety reasons, so that the acquired lock is always
+     * released at the end of your critical section, and [unlock] is never invoked before a successful
+     * lock acquisition.
+     */
+    fun lock()
+
+    /**
+     * Releases the lock.
+     * Throws [IllegalStateException] when the current thread is not holding the lock.
+     * 
+     * It is recommended to use [withLock] for safety reasons, so that the acquired lock is always
+     * released at the end of the critical section, and [unlock] is never invoked before a successful
+     * lock acquisition.
+     */
     fun unlock() 
 }
 
+/**
+ * Executes the given code [block] under this mutex's lock.
+ * 
+ * @return result of [block]
+ */
 @OptIn(ExperimentalContracts::class)
-fun <T> Mutex.withLock(block: () -> T): T {
+inline fun <T> Mutex.withLock(block: () -> T): T {
     contract { callsInPlace(block, InvocationKind.EXACTLY_ONCE) }
     lock()
     return try {

atomicfu/src/androidNative64BitMain/kotlin/kotlinx/atomicfu/locks/NativeMutexNode.kt

@@ -5,39 +5,40 @@ import kotlinx.atomicfu.atomic
 import kotlinx.atomicfu.parking.ThreadParker
 import kotlinx.atomicfu.parking.currentThreadId
 
-/**
- * Mutex implementation for Kotlin/Native.
- *
- * The [state] variable stands for: 0 -> Lock is free
- *                                  1 -> Lock is locked but no waiters
- *                                  4 -> Lock is locked with 3 waiters
- *
- * The state.incrementAndGet() call makes my claim on the lock.
- * The returned value either means I acquired it (when it is 1).
- * Or I need to enqueue and park (when it is > 1).
- *
- * The [holdCount] variable is to enable reentrancy.
- *
- * Works by using a [parkingQueue].
- * When a thread tries to acquire the lock, but finds it is already locked it enqueues by appending to the [parkingQueue].
- * On enqueue the parking queue provides the second last node, this node is used to park on.
- * When our thread is woken up that means that the thread parked on the thrid last node called unpark on the second last node.
- * Since a woken up thread is first inline it means that it's node is the head and can therefore dequeue.
- *
- * Unlocking happens by calling state.decrementAndGet().
- * When the returned value is 0 it means the lock is free and we can simply return.
- * If the new state is > 0, then there are waiters. We wake up the first by unparking the head of the queue.
- * This even works when a thread is not parked yet,
- * since the ThreadParker can be pre-unparked resulting in the parking call to return immediately.
- */
-actual class Mutex {
+internal class NativeMutex {
+    /**
+     * Mutex implementation for Kotlin/Native.
+     * In concurrentMain sourceSet to be testable with Lincheck.
+     *
+     * The [state] variable stands for: 0 -> Lock is free
+     *                                  1 -> Lock is locked but no waiters
+     *                                  4 -> Lock is locked with 3 waiters
+     *
+     * The state.incrementAndGet() call makes my claim on the lock.
+     * The returned value either means I acquired it (when it is 1).
+     * Or I need to enqueue and park (when it is > 1).
+     *
+     * The [holdCount] variable is to enable reentrancy.
+     *
+     * Works by using a [parkingQueue].
+     * When a thread tries to acquire the lock, but finds it is already locked it enqueues by appending to the [parkingQueue].
+     * On enqueue the parking queue provides the second last node, this node is used to park on.
+     * When our thread is woken up that means that the thread parked on the thrid last node called unpark on the second last node.
+     * Since a woken up thread is first inline it means that it's node is the head and can therefore dequeue.
+     *
+     * Unlocking happens by calling state.decrementAndGet().
+     * When the returned value is 0 it means the lock is free and we can simply return.
+     * If the new state is > 0, then there are waiters. We wake up the first by unparking the head of the queue.
+     * This even works when a thread is not parked yet,
+     * since the ThreadParker can be pre-unparked resulting in the parking call to return immediately.
+     */
     private val parkingQueue = ParkingQueue()
     private val owningThread = atomic(-1L)
     private val state = atomic(0)
     private val holdCount = atomic(0)
 
 
-    actual fun lock() {
+    fun lock() {
         val currentThreadId = currentThreadId()
 
         // Has to be checked in this order!
@@ -69,7 +70,7 @@ actual class Mutex {
         }
     }
 
-    actual fun unlock() {
+    fun unlock() {
         val currentThreadId = currentThreadId()
         val currentOwnerId = owningThread.value
         if (currentThreadId != currentOwnerId) throw IllegalStateException("Thread is not holding the lock")
@@ -91,11 +92,11 @@ actual class Mutex {
         }
     }
 
-    actual fun isLocked(): Boolean {
+    fun isLocked(): Boolean {
         return state.value > 0
     }
 
-    actual fun tryLock(): Boolean {
+    fun tryLock(): Boolean {
         val currentThreadId = currentThreadId()
         if (holdCount.value > 0 && owningThread.value == currentThreadId || state.compareAndSet(0, 1)) {
             owningThread.value = currentThreadId
@@ -106,7 +107,7 @@ actual class Mutex {
     }
 
     // Based on Micheal-Scott Queue
-    internal inner class ParkingQueue {
+    private class ParkingQueue {
         private val head: AtomicRef<Node>
         private val tail: AtomicRef<Node>
 
@@ -142,7 +143,7 @@ actual class Mutex {
 
     }
 
-    internal inner class Node {
+    private class Node {
         val parker = ThreadParker()
         val next = atomic<Node?>(null)
     }

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

@@ -10,7 +10,7 @@ class NativeMutexTest {
 
     @Test
     fun testNativeMutexSlow() {
-        val mutex = Mutex()
+        val mutex = NativeMutex()
         val resultList = mutableListOf<String>()
 
         val fut1 = testThread {  

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

@@ -7,7 +7,7 @@ class ReentrancyTests {
 
     @Test
     fun reentrantTestSuccess() {
-        val lock = Mutex()
+        val lock = NativeMutex()
         lock.lock()
         lock.lock()
         lock.unlock()
@@ -16,7 +16,7 @@ class ReentrancyTests {
 
     @Test
     fun reentrantTestFail() {
-        val lock = Mutex()
+        val lock = NativeMutex()
         lock.lock()
         lock.lock()
         lock.unlock()

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

@@ -54,7 +54,7 @@ class VaryingContentionTest {
     )
 
     class LockInt {
-        private val lock = Mutex()
+        private val lock = NativeMutex()
         private val check = atomic(0)
         var n = 0
         fun lock() {

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

@@ -1,9 +1,10 @@
 package kotlinx.atomicfu.locks
 
 /**
- * Multiplatform mutex.
- * On native based on pthread system calls.
- * On JVM delegates to ReentrantLock.
+ * Part of multiplatform mutex.
+ * Since this mutex will run in a single threaded environment, it doesn't provide any real synchronization.
+ * 
+ * It does keep track of reentrancy.
  */
 actual class Mutex {
     private var state = 0

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

@@ -0,0 +1,20 @@
+package kotlinx.atomicfu.locks
+
+/**
+ * This mutex uses a [ReentrantLock].
+ * 
+ * [getReentrantLock] obtains the actual [ReentrantLock].
+ * Construct with `Mutex(reentrantLock)` to create a [Mutex] that uses an existing instance of [ReentrantLock].
+ */
+actual class Mutex(private val reentrantLock: java.util.concurrent.locks.ReentrantLock) {
+    actual constructor(): this(ReentrantLock())
+    actual fun isLocked(): Boolean = reentrantLock.isLocked
+    actual fun tryLock(): Boolean = reentrantLock.tryLock()
+    actual fun lock() = reentrantLock.lock()
+    actual fun unlock() = reentrantLock.unlock()
+
+    /**
+     * @return the underlying [ReentrantLock]
+     */
+    fun getReentrantLock(): ReentrantLock = reentrantLock
+}

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

@@ -1,4 +1,4 @@
-import kotlinx.atomicfu.locks.Mutex
+import kotlinx.atomicfu.locks.NativeMutex
 import org.jetbrains.kotlinx.lincheck.LoggingLevel
 import org.jetbrains.kotlinx.lincheck.annotations.Operation
 import org.jetbrains.kotlinx.lincheck.check
@@ -13,7 +13,7 @@ class NativeMutexLincheckReentrantTest {
         fun inc(): Int = ++value
         fun get() = value
     }
-    private val lock = Mutex()
+    private val lock = NativeMutex()
     private val counter = Counter()
 
     @Test

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

@@ -1,4 +1,4 @@
-import kotlinx.atomicfu.locks.Mutex
+import kotlinx.atomicfu.locks.NativeMutex
 import org.jetbrains.kotlinx.lincheck.LoggingLevel
 import org.jetbrains.kotlinx.lincheck.annotations.Operation
 import org.jetbrains.kotlinx.lincheck.check
@@ -13,7 +13,7 @@ class NativeMutexLincheckTest {
         fun inc(): Int = ++value
         fun get() = value
     }
-    private val lock = Mutex()
+    private val lock = NativeMutex()
     private val counter = Counter()
 
     @Test