Allow successful mocking of Mutex#synchronize

[nevinera]

Fixes the issue described in #1574, which might have other presentations.

Fundamentally, it appears that the Mutex used by RSpec::Mocks::Proxy is not necessarily using the 'stashed' original Mutex implementation. I believe that this problem will happen only when the first mock attempted is a mock of the Mutex class, as that's when the Proxy instance gets created with its @messages_received_mutex instance (though I'm unsure how often Proxy is recreated).

This PR should fix #1574, but I'm not confident that the spec I've included is appropriate, or in the right place.

[pirj]

The ‘unless defined?(Mutex)’ suggests that if it happened that Mutex was somehow loaded before rspec-mocks, then we consider that we don’t need to load rspec-support’s stashed mutex.

I find your explanation interesting, but I couldn’t really follow. Could you explain in more detail?

In any case, thanks for the fix!

[pirj]

The failures seem to be coming from a warning issued on proxy.rb:14
Do we even need this Proxy::Mutex now when we have to refer to it with a namespace anyway? Would checking if Support::Mutex is defined and using it everywhere good enough?

[nevinera]

The failures seem to be coming from a warning issued on proxy.rb:14 Do we even need this Proxy::Mutex now when we have to refer to it with a namespace anyway? Would checking if Support::Mutex is defined and using it everywhere good enough?

Well, I think either change would suffice, but I'm not confident I understand how 'stashing' is supposed to behave. Is the intent to make it so that the code in this library uses RSpec::Support::Mutex instead of the (potentially mocked) ::Mutex, as I was guessing? If so, I assume they were trying to accomplish more here than just retargeting this one Mutex reference, since it would have been easier to just do that. I'll need to figure out what the warnings are about, I'll get to it after this talk (I'm at railsconf).

The explanation though! Basically (if I'm reading the code right myself), that unless defined? block was intended to define RSpec::Mocks::Proxy::Mutex = RSpec::Support::Mutex, but only if that's not already set. And then the line in the initializer created a Mutex; if the other block had run, it would have created a RSpec::Support::Mutex (because it would have found RSpec::Mocks::Proxy::Mutex first when it evaluated Mutex). But since it hadn't, it instead searched up the namespace tree and found ::Mutex, which is the one that gets mocked.

In fact, it's pretty illustrative - if you swap the mocking of Mutex#synchronize and Mutex.new (so that you mock new second), you no longer encounter the issue. That's because the Proxy gets instantiated the first time you mock something, and if you weren't mocking Mutex.new at the time, you don't end up with a mocked Mutex for @messages_received_mutex (it's already been instantiated before the mock was added).

[pirj]

how 'stashing' is supposed to behave. Is the intent to make it so that the code in this library uses RSpec::Support::Mutex instead of the (potentially mocked) ::Mutex

Exactly.

Proxy gets instantiated the first time you mock something, and if you weren't mocking Mutex.new at the time, you don't end up with a mocked Mutex

Ha! Indeed.

I must admit that ‘ruby -e “puts defined?(Mutex)”’ prints “true”, so my assumption is that the stashing didn’t work properly. If I change Mutex.new to ::Mutex.new, no specs fail.

[pirj]

I also find it unnecessary to conditionally require the mutex from rspec-support as we do this unconditionally from message_expectation.rb. And we do refer to it as Support::Mutex in MessageExpectation.

[nevinera]

Hmm, the warning being issued is that, after calling Support.require_rspec_support 'mutex', RSpec::Support::Mutex isn't defined. Which .. I'm still trying to work out how that can happen, but if it's obvious to you feel free to let me know :-)

[nevinera]

I also find it unnecessary to conditionally require the mutex from rspec-support as we do this unconditionally from message_expectation.rb. And we do refer to it as Support::Mutex in MessageExpectation.

You're right, that other usage is completely analogous, simpler, and resolves the CI failures (locally at least).

[pirj]

We also require rspec-support’s proxy in lib/rspec/mocks unconditionally.

But the failing library_wide_checks spec seems to skip liading any other gems, and their use on the class level results in a failure. I don’t agree or disagree with that, I suggest to avoid breaking its peace especially since for us those solutions are equivalent.

[nevinera]

Okay, this looks to be passing (on everything but ruby-head, which is also failing rspec-mocks/main atm). I still suspect the spec belongs somewhere more specific, if anyone has guidance about that, but I think it's otherwise good to review now :-)

[JonRowe]

The code that actually "stashes" Mutex.new to prevent it being mocked is in https://github.com/rspec/rspec-support/blob/main/lib/rspec/support/reentrant_mutex.rb#L68 there is no harm in this being merged though as we should always use our internal mutex and it looks like it might be avoiding a load order issue.

[JonRowe]

Released in 3.13.1

[pirj]

Thank you, @nevinera !

[nevinera]

Ah, definitely looks like a load-order issue that I just side-stepped then. Thanks, that'll be a good pattern to understand for later :-)