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

LoadedFeaturesIndex#register: Ignore absolute paths entirely #385

Merged
merged 1 commit into from Jan 10, 2022

Conversation

casperisfine
Copy link
Contributor

Fix: #383
Ref: fxn/zeitwerk#198

The loaded feature index goal is to prevent requiring the same "feature" twice if the $LOAD_PATH was mutated. e.g:

require "bundler" # ~/gems/bundler/lib/bundler.rb
$LOAD_PATH.unshift("some/path")
require "bundler" # some/path/bundler.rb

In such scenario Ruby remember that you require "bundler" already and even though it now maps to a new path, it won't load it again.

This was causing issues with Zeitwerk if it is loaded before bootsnap (see refs), because the cache wouldn't be cleared.

But ultimately Zeitwerk always require absolute paths, and the concern described above simply doesn't apply to absolute paths. So we can simply bail out early in these cases. That fixes the bug, and also means less work and a smaller index, so win-win.

cc @fxn

@zarqman
Copy link

zarqman commented Jan 10, 2022

Not the original issue owner, but we saw this too and can confirm this PR fixes the issue here. Thanks!

CHANGELOG.md Outdated
@@ -1,5 +1,9 @@
# Unreleased

* Ignore absolute paths in the loaded feature index. (#385)
This fixes a compatibility issue with Zeitwerk when Zeitwerk is loaded before bootsnap. It also should
Reduce the memory usage and improve load performance of Zeitwerk managed files.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Reduce the memory usage and improve load performance of Zeitwerk managed files.
reduce the memory usage and improve load performance of Zeitwerk managed files.

@fxn
Copy link

fxn commented Jan 10, 2022

Tried and works as expected. Thanks @casperisfine ❤️.

Fix: #383
Ref: fxn/zeitwerk#198

The loaded feature index goal is to prevent requiring the same "feature"
twice if the `$LOAD_PATH` was mutated. e.g:

```ruby
require "bundler" # ~/gems/bundler/lib/bundler.rb
$LOAD_PATH.unshift("some/path")
require "bundler" # some/path/bundler.rb
```

In such scenario Ruby remember that you require "bundler" already
and even though it now maps to a new path, it won't load it again.

This was causing issues with Zeitwerk if it is loaded before bootsnap (see refs),
because the cache wouldn't be cleared.

But ultimately Zeitwerk always require absolute paths, and the concern
described above simply doesn't apply to absolute paths. So we can simply
bail out early in these cases. That fixes the bug, and also means less work
and a smaller index, so win-win.
@casperisfine casperisfine merged commit 7ccddb7 into master Jan 10, 2022
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems January 10, 2022 21:23 Inactive
@casperisfine
Copy link
Contributor Author

1.9.4 is out!

@shopify-shipit shopify-shipit bot temporarily deployed to rubygems January 12, 2022 21:44 Inactive
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.

Kernel#require conflict with Zeitwerk
4 participants