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

Make autoload dynamically dispatch to require. Fixes #5403. #5404

Merged
merged 2 commits into from Feb 11, 2019

Conversation

Projects
None yet
3 participants
@headius
Copy link
Member

headius commented Nov 1, 2018

We'll see how this fares in CI.

@headius headius added this to the JRuby 9.2.2.0 milestone Nov 1, 2018

@headius

This comment has been minimized.

Copy link
Member Author

headius commented Nov 2, 2018

Ping #5403.

@tenderlove

This comment has been minimized.

Copy link
Contributor

tenderlove commented Nov 6, 2018

@headius I don't know if this helps, but it looks like the assertion is testing that there are no warnings and the warnings are coming from RubyGems (this is from the test failure output):

     [exec] TestRubyOptions#test_verbose [/home/travis/build/jruby/jruby/test/mri/ruby/test_rubyoptions.rb:100]:
     [exec] <[]> expected but was
     [exec] <["/home/travis/build/jruby/jruby/lib/ruby/stdlib/rubygems/version.rb:152: warning: loading in progress, circular require considered harmful - /home/travis/build/jruby/jruby/lib/ruby/stdlib/rubygems/version.rborg/jruby/RubyKernel.java:976:in `require'"
     [exec]  "/home/travis/build/jruby/jruby/lib/ruby/stdlib/rubygems/version.rb:152:in `<main>'"
     [exec]  "org/jruby/RubyKernel.java:976:in `require'"
     [exec]  "/home/travis/build/jruby/jruby/lib/ruby/stdlib/rubygems/specification.rb:1:in `<main>'"
     [exec]  "org/jruby/RubyKernel.java:976:in `require'"
     [exec]  "/home/travis/build/jruby/jruby/lib/ruby/stdlib/rubygems/specification.rb:10:in `<module:<main>>'"
     [exec]  "/home/travis/build/jruby/jruby/lib/ruby/stdlib/rubygems.rb:1:in `<main>'"
     [exec]  "org/jruby/RubyKernel.java:976:in `require'"
     [exec]  "/home/travis/build/jruby/jruby/lib/ruby/stdlib/rubygems.rb:1376:in `<main>'"
     [exec]  "org/jruby/RubyKernel.java:1000:in `load'"
     [exec]  "/home/travis/build/jruby/jruby/lib/ruby/stdlib/rubygems.rb:117:in `<main>'"     [exec] TestRubyOptions#test_verbose [/home/travis/build/jruby/jruby/test/mri/ruby/test_rubyoptions.rb:100]:
     [exec] <[]> expected but was
     [exec] <["/home/travis/build/jruby/jruby/lib/ruby/stdlib/rubygems/version.rb:152: warning: loading in progress, circular require considered harmful - /home/travis/build/jruby/jruby/lib/ruby/stdlib/rubygems/version.rborg/jruby/RubyKernel.java:976:in `require'"
     [exec]  "/home/travis/build/jruby/jruby/lib/ruby/stdlib/rubygems/version.rb:152:in `<main>'"
     [exec]  "org/jruby/RubyKernel.java:976:in `require'"
     [exec]  "/home/travis/build/jruby/jruby/lib/ruby/stdlib/rubygems/specification.rb:1:in `<main>'"
     [exec]  "org/jruby/RubyKernel.java:976:in `require'"
     [exec]  "/home/travis/build/jruby/jruby/lib/ruby/stdlib/rubygems/specification.rb:10:in `<module:<main>>'"
     [exec]  "/home/travis/build/jruby/jruby/lib/ruby/stdlib/rubygems.rb:1:in `<main>'"
     [exec]  "org/jruby/RubyKernel.java:976:in `require'"
     [exec]  "/home/travis/build/jruby/jruby/lib/ruby/stdlib/rubygems.rb:1376:in `<main>'"
     [exec]  "org/jruby/RubyKernel.java:1000:in `load'"
     [exec]  "/home/travis/build/jruby/jruby/lib/ruby/stdlib/rubygems.rb:117:in `<main>'"

I noticed this particular test comes from MRI. Here is the JRuby version, and here is the MRI version. These look mostly the same, except for the MJIT thing.

However, they both call assert_in_out_err. In upstream (MRI), that method will disable RubyGems, where the implementation in JRuby does not.

It could be that disabling RubyGems will fix this test (addressing the warning in RubyGems may have nothing to do with JRuby).

@headius

This comment has been minimized.

Copy link
Member Author

headius commented Nov 19, 2018

@tenderlove Thank you! I just circled back to this today, so I'll look into your theory.

@headius

This comment has been minimized.

Copy link
Member Author

headius commented Nov 19, 2018

It looks like that disabling of RubyGems didn't happen until this summer, but the tests that we're failing are certainly from long before that.

ruby/ruby@2ac4123

@headius

This comment has been minimized.

Copy link
Member Author

headius commented Nov 19, 2018

Ok, so the problem here is the same thing as #3656, where an explicit require of an autoloaded file does not remove that autoload from firing again. Here's a trivial reproduction:

atest/a.rb

class A
  autoload :B, 'atest/b'
end

require 'atest/b'

atest/b.rb

class A::B
end

The normal require starts the process of loading atest/b.rb. That file then attempts to (re)open A::B, which triggers the autoload detection. Autoload does not check if there's a require of that file already in progress before proceeding to dynamically invoke require, so it is detected and reported as a circular require.

I am working through CRuby logic for this. I believe what's missing is the check_autoload_required.

@headius

This comment has been minimized.

Copy link
Member Author

headius commented Nov 20, 2018

Yes, that's basically the issue. This all feeds back into us not having the same search and caching capabilities for load paths, expanded paths, and loaded features that MRI does.

I ported over the "loaded_feature_path" method and added a loop over all currently-loading scripts, as is done in the rb_feature_p function. CRuby checks for an existing feature in the following ways, if I'm following right:

  • Check the actual loaded feature index for the requested feature. This maintains a mapping of short feature name to actual loaded path. We have this index but I'm not sure how well it works.
  • Use the expanded load path and list of files currently being loaded to see if any combination matches. If load path entry + feature == currently-loading file, we consider the feature provided. I assume that require attempts to lock this path before assuming any existing lock is its own.

So basically, we're missing most of the search logic that MRI does during rb_feature_p because we don't walk through the whole load path (with pre-expanded paths) and look for the long name.

See also #2794.

@enebo It's time we bit the bullet and matched their searches, at least, which will likely necessitate incorporating the same caching.

@headius headius force-pushed the headius:autoload-require branch from 31d1414 to 9c35116 Nov 20, 2018

@headius

This comment has been minimized.

Copy link
Member Author

headius commented Nov 20, 2018

Rebased atop master with the --disable-gems tweak. I believe based on @kares comments that there should be only one failure after this, and it may be something we can skip fixing if it's not critical (dispatching to require when the file is already being loaded on this thread).

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Nov 20, 2018

@headius yeah if we need this for rails 6 then matching MRI search logic will definitely be the fastest path to a stable release.

@enebo enebo modified the milestones: JRuby 9.2.5.0, JRuby 9.2.6.0 Dec 6, 2018

@headius headius modified the milestones: JRuby 9.2.6.0, JRuby 9.2.7.0 Dec 18, 2018

@enebo enebo merged commit 6ea114b into jruby:master Feb 11, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.