Skip to content

Rework load service, load path, loaded features, and autoload #5764

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

Merged
merged 91 commits into from
Dec 15, 2020

Conversation

headius
Copy link
Member

@headius headius commented Jun 11, 2019

This is a big one. This PR includes rewrites of how we handle load/require, load path lookups/caching/canonicalization, loaded feature caching, and autoload lifecycle relating to the rest.

Parts of this are loose ports from MRI, and still need some refinement and optimization. In particular, we need to make sure the load path and loaded features caches are tight, efficient, and not getting regenerated excessively. This also should be audited for concurrent state changes in autoload, since there's a few complicated interactions with threads and in-progress autoloads. We are passing most of the related specs (notably not the tags added in aea0e65) but the code is not proven safe in my eyes.

Hopefully others can help me audit this. I know it's a lot of code.

With this PR, here's status:

  • MRI test_autoload: two excludes, for deprecation warning and a possibly bad test (threading)
  • rubyspec kernel/module autoload_spec: 100%
  • zeitwerk: 100%

headius added 30 commits March 19, 2019 20:00
* Constants for 's', 'r' values indicating type of feature.
* Store actual loaded feature string in feature rather than using
  index into original list. The index gets wiped out if loaded
  features get modified anyway.
* Simplify deduplication of a Java string.
* New description of indexing logic.
Module to use from Kernel methods should be determined via cref,
aka StaticScope, so those methods require caller's scope now. The
methods in Module become the master impls.
* Don't add . to LOAD_PATH for specs anymore. Fixes regressions
  on load_service_redux branch where spec files from . started to
  be visible due to load service fixes.
* Only show attr boolean warning in verbose mode.
When an autoload runs successfully but does not set the constant
in question, it should clear out any autoload plumbing that was
used but leave the UNDEF value in place, to indicate this value
still is not defined. This change removes the explicit delete of
the constant, to correspond to MRI logic in autoload_reset (which
only sets the actual constant if the autoload set it to a non-null
value) and modifies the getConstantSkipAutoload logic, used by
`defined?` logic, to consider an unresolved undef as undefined if
there's no autoload plumbing installed.
@alecgorge
Copy link

@headius is there anything I can do here to help get this over the finish line? Do you need more eyes on the code or is there functionality not finished yet?

@headius
Copy link
Member Author

headius commented Sep 17, 2020

@alecgorge The remaining work is to get autoloading with concurrent requires thread-safe. This logic was loosely ported from CRuby, which does not allow these operations to be performed in parallel, but the locking mechanisms we have available in JRuby are insufficient to make sure the case is safe. This is, I believe, the reason why the MRI suite has some sporadic failures; sometimes the parallel autoload tests work and sometimes they don't.

@rickhull
Copy link

rickhull commented Dec 8, 2020

I see the Travis CI stuff is failing -- due to Travis CI issues. FYI, Travis CI is basically dead for open source projects.

I just did a migration to GitHub Actions for a simple repo. I used https://bibwild.wordpress.com/2020/11/12/deep-dive-moving-ruby-projects-from-travis-to-github-actions-for-ci/ as a guide, and it was fairly straightforward. I found my way to this issue because my tests against jruby are showing the "circular require considered harmful" warning.

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

Successfully merging this pull request may close these issues.

Autoload is not overwritten by subsequent explicit require
6 participants