From 26569b3066ac44e1e2c3f0cd82c45961e23f8f45 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Mon, 10 Jan 2022 09:58:16 +0100 Subject: [PATCH] LoadedFeaturesIndex#register: Ignore absolute paths entirely Fix: https://github.com/Shopify/bootsnap/issues/383 Ref: https://github.com/fxn/zeitwerk/issues/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. --- lib/bootsnap.rb | 10 ++++++++++ lib/bootsnap/load_path_cache/cache.rb | 12 +----------- .../load_path_cache/loaded_features_index.rb | 5 +++++ test/load_path_cache/loaded_features_index_test.rb | 14 +++++++++++++- 4 files changed, 29 insertions(+), 12 deletions(-) diff --git a/lib/bootsnap.rb b/lib/bootsnap.rb index 06a32e85..842973d9 100644 --- a/lib/bootsnap.rb +++ b/lib/bootsnap.rb @@ -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 diff --git a/lib/bootsnap/load_path_cache/cache.rb b/lib/bootsnap/load_path_cache/cache.rb index b1e82328..33ea1967 100644 --- a/lib/bootsnap/load_path_cache/cache.rb +++ b/lib/bootsnap/load_path_cache/cache.rb @@ -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 @@ -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) } diff --git a/lib/bootsnap/load_path_cache/loaded_features_index.rb b/lib/bootsnap/load_path_cache/loaded_features_index.rb index 769093fe..12cc0fb1 100644 --- a/lib/bootsnap/load_path_cache/loaded_features_index.rb +++ b/lib/bootsnap/load_path_cache/loaded_features_index.rb @@ -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 diff --git a/test/load_path_cache/loaded_features_index_test.rb b/test/load_path_cache/loaded_features_index_test.rb index 5e5f02d1..fba42b4c 100644 --- a/test/load_path_cache/loaded_features_index_test.rb +++ b/test/load_path_cache/loaded_features_index_test.rb @@ -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