Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Monitor prevents concurrent requires regardless of whether they come from gems #640

Closed
headius opened this Issue · 6 comments

4 participants

@headius

#637 details the problem and provided a patch that did not pass tests (my fault, sorry). This is still an issue since that PR had to be reverted.

The method in question needs to be refactored to pull out the require logic from the RG internals, but here is the fix that @evanphx originally suggested to me to help mitigate jruby/jruby#993.

https://gist.github.com/headius/6516066

We may ship JRuby with this fix in place, since we have at least one user whose app won't boot under JRuby 1.7.5.dev.

And we need a better fix for this issue in RG 2.1, since it effectively prevents any requires from running concurrently.

@headius

So yeah, I tried again to move the bulk of this logic out of Kernel, and that seems to be what causes failures. I have no idea why.

I expanded the patch (same gist as above) to release the monitor for all gem_original_require calls, and all tests pass plus the concurrent require test from jruby/jruby#993. It seems likely we will ship this in JRuby 1.7.5.

@drbrain
Owner

The rescue needs to stay in Kernel#require to handle failure of the require following your extracted Gem.resolve. It's not clear at all how the current Kernel.require works.

I see no reason why we can't ship your patch tomorrow in RubyGems 2.1.2. Will that be enough time for JRuby 1.7.5?

@headius

Yeah, my move to Gem namespace basically took this entire logic and moved it... so there's something about this logic that has to run within the Kernel namespace rather than the Gem namespace. Baffling.

Yes, a release of RG 2.1.2 tomorrow would get into our release. We're still tidying up some issues users have reported against master over the weekend.

@drbrain
Owner

Patch 2 of 3 in my comment on #637 may help clear up why you can't move all of it, it's not about namespace but about program flow. The rescue I moved back needs to handle a failure in the require in the second line of Kernel#require after your patch in #637.

@drbrain drbrain closed this issue from a commit
@drbrain drbrain Restore concurrent requires
When #8374 was fixed all requires were serialized, so one thread would
be blocked waiting for gem resolution (if any) and require in another
thread.  This is undesirable for JRuby, in particular.

The monitor protecting the RubyGems internals only needs to cover gem
activation and modifying $LOAD_PATH, not requiring files.

Now the monitor is released before calling the original Kernel#require
which allows other threads to require files without waiting for
RubyGems.

Fixes #640

See also #637
16fc8e8
@drbrain drbrain closed this in 16fc8e8
@voxik

Could you please backport it for 2.0 branch as well? This should never be introduced there in the first place. It breaks real world code geemus/shindo#12 e.g. there is called bin/shindo which in turns require lib/shindo/bin. Requires in this file causes deadlock.

@voxik voxik referenced this issue in geemus/shindo
Closed

Shindo does not work with Rubygems 2.0.4+ #12

@nagachika

Hi, is there any chance to this patch to 2.0? There's another report about it in ruby's bug tracker (https://bugs.ruby-lang.org/issues/9224). I'd like to contain this fix in next ruby 2.0.0 patchlevel release. The patch can be cleanly merged and pass make test-all of ruby.

@nagachika nagachika referenced this issue from a commit in nagachika/rubygems
@nagachika nagachika Backport 16fc8e8 to 2.0 branch.
Restore concurrent requires

When #8374 was fixed all requires were serialized, so one thread would
be blocked waiting for gem resolution (if any) and require in another
thread.  This is undesirable for JRuby, in particular.

The monitor protecting the RubyGems internals only needs to cover gem
activation and modifying $LOAD_PATH, not requiring files.

Now the monitor is released before calling the original Kernel#require
which allows other threads to require files without waiting for
RubyGems.

Fixes #640

See also #637
935e50b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.