`MutexImpl.unlock()` hangs forever when locked through `Semaphore.aquire()`

[lukellmann]

The following code hangs forever on the call to mutex.unlock():

val mutex = Mutex()
(mutex as Semaphore).aquire()
println("locked: ${mutex.isLocked}")
mutex.unlock()
println("unlocked: ${!mutex.isLocked}")

The reason seems to be that MutexImpl extends SemaphoreImpl but SemaphoreImpl.aquire() bypasses the invariants of MutexImpl.owner: aquire doesn't change owner but unlock spin loops because it expects to see something different than NO_OWNER.

Similar bugs might be possible with other combinations of Mutex and Semaphore calls for MutexImpl, I haven't checked yet.

Admittedly, this is a bit contrived, so I'm not sure if this is really a bug or just a missuse of implementatiom details (the public Mutex interface doesn't extend Semaphore). But if this is considered worth fixing I see two possible approaches:

• changing the behavior of the SemaphoreImpl methods (through override etc.) so they don't break MutexImpl invariants
• splitting up SemaphoreImpl into a new abstract base type for both Mutex and Semaphore (that however doesn't implement Semaphore directly) and a thin wrapper around it to actually implement Semaphore - that way interacting with instances of MutexImpl through the methods of Semaphore won't be possible in the first place

[dkhalanskyjb]

I'm not sure if this is really a bug

It is, by the substitution principle (https://en.wikipedia.org/wiki/Liskov_substitution_principle)

[dkhalanskyjb]

However, given that it requires a cast that relies on knowing the implementation details, it's a bug typically invisible to the user, so it's low-priority. When we merge https://github.com/Kotlin/kotlinx.coroutines/pull/2045, this will get fixed automatically.

[qwwdfsad]

@lukellmann could you please elaborate on how you've stumbled across this bug?

Mutex implementing Semaphore is a private implementation detail that we cannot hide (or don't want to; it incurs a small footprint hit); I wonder if this is something you use in your codebase: understanding the scenario would help us designing better and more robust primitives

[lukellmann]

I read the source and realized that this could happen. I then constructed this code. So it doesn't impact any real code of mine.

[qwwdfsad]

Example by Dmitry to better showcase the potential confusion:

fun myLock(lockImpl: Any) {
  when (lockImpl) {
    is Semaphore -> lockImpl.acquire()
    is Mutex -> lockImpl.lock()
  }
}

fun myUnlock(lockImpl: Any) {
  when (lockImpl) {
    is Mutex -> lockImpl.unlock()
    is Semaphore -> lockImpl.release()
  }
}