Skip to content

Commit

Permalink
LoadedFeaturesIndex#register: Ignore absolute paths entirely
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
byroot committed Jan 10, 2022
1 parent a0aa3d2 commit 26569b3
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 12 deletions.
10 changes: 10 additions & 0 deletions lib/bootsnap.rb
Original file line number Diff line number Diff line change
Expand Up @@ -123,4 +123,14 @@ def self.default_setup
end
end
end

if RbConfig::CONFIG['host_os'] =~ /mswin|mingw|cygwin/
def self.absolute_path?(path)
path[1] == ':'
end
else
def self.absolute_path?(path)
path.start_with?('/')
end
end
end
12 changes: 1 addition & 11 deletions lib/bootsnap/load_path_cache/cache.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ def find(feature, try_extensions: true)
reinitialize if (@has_relative_paths && dir_changed?) || stale?
feature = feature.to_s.freeze

return feature if absolute_path?(feature)
return feature if Bootsnap.absolute_path?(feature)

if feature.start_with?('./', '../')
return try_extensions ? expand_path(feature) : File.expand_path(feature).freeze
Expand Down Expand Up @@ -98,16 +98,6 @@ def find(feature, try_extensions: true)
raise(LoadPathCache::FallbackScan, '', []) if @development_mode
end

if RbConfig::CONFIG['host_os'] =~ /mswin|mingw|cygwin/
def absolute_path?(path)
path[1] == ':'
end
else
def absolute_path?(path)
path.start_with?(SLASH)
end
end

def unshift_paths(sender, *paths)
return unless sender == @path_obj
@mutex.synchronize { unshift_paths_locked(*paths) }
Expand Down
5 changes: 5 additions & 0 deletions lib/bootsnap/load_path_cache/loaded_features_index.rb
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,11 @@ def key?(feature)
# 2. Inspect $LOADED_FEATURES upon return from yield to find the matching
# entry.
def register(short, long = nil)
# Absolute paths are not a concern.
if Bootsnap.absolute_path?(short.to_s)
return yield
end

if long.nil?
len = $LOADED_FEATURES.size
ret = yield
Expand Down
14 changes: 13 additions & 1 deletion test/load_path_cache/loaded_features_index_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -136,11 +136,23 @@ def test_derives_initial_state_from_loaded_features
end

def test_works_with_pathname
path = '/tmp/bundler.rb'
path = 'bundler.rb'
pathname = Pathname.new(path)
@index.register(pathname, path) { true }
assert(@index.key?(pathname))
end

def test_ignores_absolute_paths
path = '/tmp/bundler.rb'
@index.register(path) { true }
refute(@index.key?(path))

path = '/tmp/bundler.rb'
pathname = Pathname.new(path)
@index.register(pathname, path) { true }
refute(@index.key?(path))
refute(@index.key?(pathname))
end
end
end
end

0 comments on commit 26569b3

Please sign in to comment.