From c8e7749222e692c9fba54908bce5ec4fc01f80e5 Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Tue, 29 Jun 2021 17:23:55 +0300 Subject: [PATCH 1/2] ~remove old test for #80, the issue is no longer relevant --- .../common/test/sync/MutexTest.kt | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/kotlinx-coroutines-core/common/test/sync/MutexTest.kt b/kotlinx-coroutines-core/common/test/sync/MutexTest.kt index 4f428bc4b0..9b8ea717d7 100644 --- a/kotlinx-coroutines-core/common/test/sync/MutexTest.kt +++ b/kotlinx-coroutines-core/common/test/sync/MutexTest.kt @@ -59,22 +59,6 @@ class MutexTest : TestBase() { assertFalse(mutex.isLocked) } - @Test - fun testUnconfinedStackOverflow() { - val waiters = 10000 - val mutex = Mutex(true) - var done = 0 - repeat(waiters) { - GlobalScope.launch(Dispatchers.Unconfined) { // a lot of unconfined waiters - mutex.withLock { - done++ - } - } - } - mutex.unlock() // should not produce StackOverflowError - assertEquals(waiters, done) - } - @Test fun holdLock() = runTest { val mutex = Mutex() From 7924f6c0560b0895ceb894fe4818d1c6e84dec0a Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Tue, 29 Jun 2021 18:01:13 +0300 Subject: [PATCH 2/2] Tweak and enforce Mutex.unlock contract * Ensure that when unlock without an owner cannot successfully unlock Mutex that was originally locked with an owner Fixes #2401 --- .../common/src/sync/Mutex.kt | 45 ++++++++++++------- .../common/test/sync/MutexTest.kt | 43 ++++++++++++++++-- 2 files changed, 69 insertions(+), 19 deletions(-) diff --git a/kotlinx-coroutines-core/common/src/sync/Mutex.kt b/kotlinx-coroutines-core/common/src/sync/Mutex.kt index 7d0a343d95..fb8853b6c3 100644 --- a/kotlinx-coroutines-core/common/src/sync/Mutex.kt +++ b/kotlinx-coroutines-core/common/src/sync/Mutex.kt @@ -10,7 +10,6 @@ import kotlinx.coroutines.internal.* import kotlinx.coroutines.intrinsics.* import kotlinx.coroutines.selects.* import kotlin.contracts.* -import kotlin.coroutines.* import kotlin.jvm.* import kotlin.native.concurrent.* @@ -20,11 +19,6 @@ import kotlin.native.concurrent.* * Mutex has two states: _locked_ and _unlocked_. * It is **non-reentrant**, that is invoking [lock] even from the same thread/coroutine that currently holds * the lock still suspends the invoker. - * - * JVM API note: - * Memory semantic of the [Mutex] is similar to `synchronized` block on JVM: - * An unlock on a [Mutex] happens-before every subsequent successful lock on that [Mutex]. - * Unsuccessful call to [tryLock] do not have any memory effects. */ public interface Mutex { /** @@ -81,8 +75,8 @@ public interface Mutex { * Unlocks this mutex. Throws [IllegalStateException] if invoked on a mutex that is not locked or * was locked with a different owner token (by identity). * - * @param owner Optional owner token for debugging. When `owner` is specified (non-null value) and this mutex - * was locked with the different token (by identity), this function throws [IllegalStateException]. + * @param owner Optional owner token for debugging. When `owner` does not match the owner (or its absence) + * that locked this mutex, this function throws [IllegalStateException]. */ public fun unlock(owner: Any? = null) } @@ -136,6 +130,12 @@ private val EMPTY_LOCKED = Empty(LOCKED) private val EMPTY_UNLOCKED = Empty(UNLOCKED) private class Empty( + /* + * State of the lock: + * - LOCKED + * - UNLOCKED + * - owner (of the lock, passed via 'lock(owner)') + */ @JvmField val locked: Any ) { override fun toString(): String = "Empty[$locked]" @@ -163,7 +163,7 @@ internal class MutexImpl(locked: Boolean) : Mutex, SelectClause2 { return state is LockedQueue && state.isEmpty } - public override fun tryLock(owner: Any?): Boolean { + override fun tryLock(owner: Any?): Boolean { _state.loop { state -> when (state) { is Empty -> { @@ -183,7 +183,7 @@ internal class MutexImpl(locked: Boolean) : Mutex, SelectClause2 { } } - public override suspend fun lock(owner: Any?) { + override suspend fun lock(owner: Any?) { // fast-path -- try lock if (tryLock(owner)) return // slow-path -- suspend @@ -304,16 +304,23 @@ internal class MutexImpl(locked: Boolean) : Mutex, SelectClause2 { _state.loop { state -> when (state) { is Empty -> { - if (owner == null) + if (owner == null) { check(state.locked !== UNLOCKED) { "Mutex is not locked" } - else - check(state.locked === owner) { "Mutex is locked by ${state.locked} but expected $owner" } + expectNoOwner(state.locked) + } + else { + val actualOwner = state.locked + expectOwner(actualOwner, owner) + } if (_state.compareAndSet(state, EMPTY_UNLOCKED)) return } is OpDescriptor -> state.perform(this) is LockedQueue -> { - if (owner != null) - check(state.owner === owner) { "Mutex is locked by ${state.owner} but expected $owner" } + if (owner != null) { + expectOwner(state.owner, owner) + } else { + expectNoOwner(state.owner) + } val waiter = state.removeFirstOrNull() if (waiter == null) { val op = UnlockOp(state) @@ -332,6 +339,14 @@ internal class MutexImpl(locked: Boolean) : Mutex, SelectClause2 { } } + private fun expectNoOwner(l: Any) { + check(l == LOCKED) { "Mutex is locked by $l but expected no owner" } + } + + private fun expectOwner(actualOwner: Any, owner: Any?) { + check(actualOwner === owner) { "Mutex is locked by $actualOwner but expected $owner" } + } + override fun toString(): String { _state.loop { state -> when (state) { diff --git a/kotlinx-coroutines-core/common/test/sync/MutexTest.kt b/kotlinx-coroutines-core/common/test/sync/MutexTest.kt index 9b8ea717d7..917c0e56f5 100644 --- a/kotlinx-coroutines-core/common/test/sync/MutexTest.kt +++ b/kotlinx-coroutines-core/common/test/sync/MutexTest.kt @@ -4,7 +4,6 @@ package kotlinx.coroutines.sync -import kotlinx.atomicfu.* import kotlinx.coroutines.* import kotlin.test.* @@ -32,7 +31,7 @@ class MutexTest : TestBase() { } @Test - fun tryLockTest() { + fun testTryLock() { val mutex = Mutex() assertFalse(mutex.isLocked) assertTrue(mutex.tryLock()) @@ -50,7 +49,7 @@ class MutexTest : TestBase() { } @Test - fun withLockTest() = runTest { + fun testWithLock() = runTest { val mutex = Mutex() assertFalse(mutex.isLocked) mutex.withLock { @@ -60,7 +59,7 @@ class MutexTest : TestBase() { } @Test - fun holdLock() = runTest { + fun testHoldsLock() = runTest { val mutex = Mutex() val firstOwner = Any() val secondOwner = Any() @@ -91,4 +90,40 @@ class MutexTest : TestBase() { assertFalse(mutex.holdsLock(firstOwner)) assertFalse(mutex.holdsLock(secondOwner)) } + + @Test + fun testUnlockWithoutOwner() { + val owner = Any() + val mutex = Mutex() + assertTrue(mutex.tryLock(owner)) + assertFailsWith { mutex.unlock(Any()) } + assertFailsWith { mutex.unlock() } + assertFailsWith { mutex.unlock(null) } + assertTrue(mutex.holdsLock(owner)) + mutex.unlock(owner) + assertFalse(mutex.isLocked) + } + + @Test + fun testUnlockWithoutOwnerWithLockedQueue() = runTest { + val owner = Any() + val owner2 = Any() + val mutex = Mutex() + assertTrue(mutex.tryLock(owner)) + expect(1) + launch { + expect(2) + mutex.lock(owner2) + } + yield() + expect(3) + assertFailsWith { mutex.unlock(owner2) } + assertFailsWith { mutex.unlock() } + assertFailsWith { mutex.unlock(null) } + assertTrue(mutex.holdsLock(owner)) + mutex.unlock(owner) + assertTrue(mutex.isLocked) + assertTrue(mutex.holdsLock(owner2)) + finish(4) + } }