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

Explicit require of autoloaded class triggers double load #2510

Closed
bbrowning opened this issue Jan 23, 2015 · 5 comments
Closed

Explicit require of autoloaded class triggers double load #2510

bbrowning opened this issue Jan 23, 2015 · 5 comments

Comments

@bbrowning
Copy link
Contributor

Explicitly requiring a class that's also marked to autoload can result in that class getting loaded twice. Normally that's not a huge problem, but in cases where the class undefs methods or something else similar it's an issue.

Here's a simple example created from a real-world ActiveSupport 4.1.8 issue I hit:

https://gist.github.com/bbrowning/cb59f764a697b93b5b6c

@headius
Copy link
Member

headius commented Jan 23, 2015

This is a big job. Here's what we know.

JRuby 1.7 and 9k require logic looks mostly identical, so I don't think this is a bug in require. Most likely this is a bug that has always existed in how we search for loaded and loading features, and it is being exposed now because 9k is (properly, I think) expanding absolute paths at various places during load.

I started looking into MRI's logic, and it has many differences from ours.

  • search_required, which should be equivalent to our featureAlreadyLoaded, has a great deal more logic. It is extension-aware, and performs lower level searches for loaded features with the extension in mind.
  • rb_feature_p, upon which we probably based our featureAlreadyLoaded, has logic to check expanded load path entries, circular load lock ("loading" collection) and more.
  • MRI's feature_index appears to aggregate not just the file loaded for a feature, but other file paths that should match that feature.

I'm attempting a partial port, but I have other responsibilities that will pull me away from this.

@headius
Copy link
Member

headius commented Jan 23, 2015

I started the port of search_required. Here's what I have: https://gist.github.com/headius/a78c68145e2f8c739b1e

This is mostly finished except for the goto in there.

The big snag came while attempting to port rb_feature_p, due to differences in our feature_index and how we maintain it.

We also don't have an expanded load_path as in MRI. They use this expanded load_path cache for trying different filesystem prefixes.

On the up side, if we manage to port all this logic over it could speed up require/load searching too.

@headius
Copy link
Member

headius commented Jan 23, 2015

I will be unable to work on this for a while. Pinging @ratnikov and @Who828 since they might have interest in picking up where I left off.

@headius
Copy link
Member

headius commented Apr 17, 2015

We actually had to fix this to get Rails working, because an autoloaded class that undef'ed a method was getting run twice (and blowing up). Should be fixed in 9k.pre2.

@headius headius closed this as completed Apr 17, 2015
@headius headius modified the milestones: JRuby 1.7.20, 9.0.0.0.pre2 Apr 20, 2015
@headius
Copy link
Member

headius commented Apr 20, 2015

Apparently a big problem for others on 1.7, so I backported the fixes there too.

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

No branches or pull requests

2 participants