Skip to content
This repository has been archived by the owner on Nov 30, 2024. It is now read-only.

Use Mutex#owned? to correctly check if the Mutex is owned by the current Thread or Fiber #503

Merged
merged 1 commit into from
Apr 28, 2021

Conversation

eregon
Copy link
Contributor

@eregon eregon commented Apr 27, 2021

Based on #502 (comment)
Seems a better alternative to #502.

@ioquatix
Copy link

Great insight and improved implementation. Well done.

@pirj
Copy link
Member

pirj commented Apr 28, 2021

It's strange, on 3.0.0 it fails locally for me (macOS 10.14.6):

Failures:

  1) RSpec::Support::ReentrantMutex waits when trying to lock from another Fiber
     Failure/Error:
       expect {
         mutex.send(:enter)
         raise 'should reach here: mutex is already locked on different Fiber'
       }.to raise_error(Exception, 'waited correctly')

     Exception:
       waited correctly
     # /Users/pirj/source/rspec-dev/repos/rspec-expectations/lib/rspec/matchers.rb:771:in `require'
     # /Users/pirj/source/rspec-dev/repos/rspec-expectations/lib/rspec/matchers.rb:771:in `raise_error'
     # ./spec/rspec/support/reentrant_mutex_spec.rb:38:in `block (4 levels) in <top (required)>'

Finished in 0.02348 seconds (files took 0.39513 seconds to load)
4 examples, 1 failure

Wondering if Thread.pass's behavior is platform-specific ("A running thread may or may not switch"):

 pass → nil click to toggle source

Give the thread scheduler a hint to pass execution to another thread. A running thread may or may not switch, it depends on OS and processor.

Installing 3.0.1 and reinstalling 3.0.0 to understand if there is a difference, as CI seems to be using 3.0.1:

Installing Bundler
  /opt/hostedtoolcache/Ruby/3.0.1/x64/bin/gem install bundler -v ~> 2 --no-document

@pirj
Copy link
Member

pirj commented Apr 28, 2021

No worries on 1.8 and 1.9 failures, we can conditionally define different method implementations for ReentrantMutex, we even already do this in if defined? ::Mutex.

@pirj
Copy link
Member

pirj commented Apr 28, 2021

Same failure for me on 3.0.1. @eregon do you think it is possible to change the test not to rely on platform-specific behaviour?

@eregon
Copy link
Contributor Author

eregon commented Apr 28, 2021

The message above looks a bit like a bug of the raise_error matcher.
What I guess it is that the Thread uses a sleep status before mutex.send(:enter), I'll add a check to make sure that's not the case.
Thread.pass is fine, it's not platform-specific to wait for another thread with it.

@eregon eregon force-pushed the fix-reentrant-mutex-fiber branch from 4d324c6 to 5e76f82 Compare April 28, 2021 18:08
@eregon
Copy link
Contributor Author

eregon commented Apr 28, 2021

The test should work reliably now.

@eregon
Copy link
Contributor Author

eregon commented Apr 28, 2021

@pirj Could you make this PR work for 1.8?
I'm not willing to spend time on fixing such ancient Rubies (not even supported anymore by anyone).

Alternatively, we could target this PR at RSpec 4 I guess, WDYT @ioquatix?

@eregon
Copy link
Contributor Author

eregon commented Apr 28, 2021

Unfortunately I can't even see the CI running due that change done by GitHub :/ (https://twitter.com/eregontp/status/1387472155567394820)

@eregon eregon force-pushed the fix-reentrant-mutex-fiber branch from 5e76f82 to 708d217 Compare April 28, 2021 18:30
@eregon eregon changed the base branch from main to 4-0-dev April 28, 2021 18:30
@eregon
Copy link
Contributor Author

eregon commented Apr 28, 2021

Retargeted to 4-0-dev and then it can be backported if wanted.

@pirj
Copy link
Member

pirj commented Apr 28, 2021

The test should work reliably now.

Works seamlessly.

can't even see the CI running due that change done by GitHub

An inconvenience at least.

Could you make this PR work for 1.8?
I'm not willing to spend time on fixing such ancient Rubies

I understand. No problem, I'll take care of backporting.

Thanks a lot for putting your time in this, @eregon !

…ent Thread or Fiber

* In Ruby >= 3, Mutex are held per Fiber, not per Thread.
* Fixes rspec#501
@pirj pirj force-pushed the fix-reentrant-mutex-fiber branch from 4e3f88e to a4be9d5 Compare April 28, 2021 21:06
@pirj pirj requested review from benoittgt and JonRowe April 28, 2021 21:06
@pirj
Copy link
Member

pirj commented Apr 28, 2021

@JonRowe @benoittgt I intend to merge on green, since it's for RSpec 4.0, and will send a backport PR for RSpec 3.

@ioquatix
Copy link

Amazing work everyone!

@pirj pirj merged commit 6538c68 into rspec:4-0-dev Apr 28, 2021
yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 19, 2021
…reentrant-mutex-fiber

Use Mutex#owned? to correctly check if the Mutex is owned by the current Thread or Fiber

---
This commit was imported from rspec/rspec-support@6538c68.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ThreadError: Attempt to unlock a mutex which is locked by another thread/fiber
3 participants