Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
lukellmann opened this issue Jan 28, 2024 · 4 comments
Open
Labels

Comments

@lukellmann
Copy link
Contributor

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
@lukellmann lukellmann added the bug label Jan 28, 2024
@dkhalanskyjb
Copy link
Collaborator

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
Copy link
Collaborator

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 #2045, this will get fixed automatically.

@qwwdfsad
Copy link
Member

@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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants