-
-
Notifications
You must be signed in to change notification settings - Fork 928
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
Conversation
* 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.
@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? |
@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. |
These will still need to be fixed before release but we need to get this branch merged and work from there.
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. |
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: