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

Drop dependency on mutex_m #49674

Merged
merged 1 commit into from
Oct 18, 2023
Merged

Drop dependency on mutex_m #49674

merged 1 commit into from
Oct 18, 2023

Conversation

casperisfine
Copy link
Contributor

It used to be stdlib but is being extracted in modern rubies.

Overall its usefulness is dubious. In all cases it is included in Rails, it's only for the synchronize method, but end up exposing a dozen other useless methods.

In the end just using a Mutex is clearer and simpler.

In some cases we can even get away with a single mutex in a constant.

It used to be stdlib but is being extracted in modern rubies.

Overall its usefulness is dubious. In all cases it is included in
Rails, it's only for the `synchronize` method, but end up exposing
a dozen other useless methods.

In the end just using a Mutex is clearer and simpler.

In some cases we can even get away with a single mutex in a constant.
@byroot byroot merged commit 8cf4242 into rails:main Oct 18, 2023
4 checks passed
@tisba
Copy link

tisba commented Nov 14, 2023

hmm, this hasn't made it into 7.1.2, right? Quite the contrary it is a direct dependency of ActiveSupport: https://github.com/rails/rails/blob/v7.1.2/activesupport/activesupport.gemspec

🤔

@byroot
Copy link
Member

byroot commented Nov 14, 2023

We don't backport this kind of patches.

@tisba
Copy link

tisba commented Nov 14, 2023

Alright 👍

PS: I only noticed this because our builds on GH Actions started to break since mutex_m was added as an dependency 😞

/opt/hostedtoolcache/Ruby/3.2.2/x64/lib/ruby/gems/3.2.0/gems/bundler-2.4.22/lib/bundler/runtime.rb:304:in `check_for_activated_spec!': You have already activated mutex_m 0.1.2, but your Gemfile requires mutex_m 0.2.0. Since mutex_m is a default gem, you can either remove your dependency on it or try updating to a newer version of bundler that supports mutex_m as a default gem. (Gem::LoadError)

Still investigating, but will file an issue unless that's clearly something on our end.

@byroot
Copy link
Member

byroot commented Nov 14, 2023

Sounds like a bundler bug.

@byroot
Copy link
Member

byroot commented Nov 14, 2023

Actually, bundler doesn't use mutex_m, so must be something else requiring it early. I recommend editing mutex_m 0.1.2 to put a puts caller in it to track who is loading it.

@tisba
Copy link

tisba commented Nov 14, 2023

Actually, bundler doesn't use mutex_m, so must be something else requiring it early. I recommend editing mutex_m 0.1.2 to put a puts caller in it to track who is loading it.

Yeah, that was plan too. Going to test this out shortly. It fails right away with bundle exec rails db:prepare (after a ruby/setup-ruby@v1 step).

@tisba
Copy link

tisba commented Nov 14, 2023

I did not edit the mutex_m gem as I had a hunch that the only thing running before rails/my code is spring. And it looks like this is issue: https://github.com/rails/spring/blob/v4.1.1/lib/spring/watcher/abstract.rb#L2. Turns out that the last commit to main is in fact to add mutex_m as an explicit dependency 😄 rails/spring@6b2001f

I'll open an issue over there asking for a release.

@byroot
Copy link
Member

byroot commented Nov 14, 2023

I'll take care of it, thanks for finding it.

@byroot
Copy link
Member

byroot commented Nov 14, 2023

Ref: rails/spring#703

@byroot
Copy link
Member

byroot commented Nov 14, 2023

Spring 4.1.2 is out.

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

Successfully merging this pull request may close these issues.

None yet

3 participants