Ensure symlinked realpaths are not loaded twice

CHANGELOG.md

@@ -31,6 +31,7 @@ Compatibility:
 * Add `Refinement#refined_class` (#3039, @itarato).
 * Add `rb_hash_new_capa` function (#3039, @itarato).
 * Fix `Encoding::Converter#primitive_convert` and raise `FrozenError` when a destination buffer argument is frozen (@andrykonchin).
+* Ensure symlinked paths aren't loaded more than once (@rwstauner).
 
 Performance:
 

spec/ruby/core/kernel/require_relative_spec.rb

@@ -235,6 +235,24 @@
           features.should_not include(canonical_path)
         end
 
+        ruby_version_is "3.1" do
+          it "does not require symlinked target file twice" do
+            symlink_path = "#{@symlink_basename}/load_fixture.rb"
+            absolute_path = "#{tmp("")}#{symlink_path}"
+            canonical_path = "#{CODE_LOADING_DIR}/load_fixture.rb"
+            touch(@requiring_file) { |f|
+              f.puts "require_relative #{symlink_path.inspect}"
+              f.puts "require_relative #{canonical_path.inspect}"
+            }
+            load(@requiring_file)
+            ScratchPad.recorded.should == [:loaded]
+
+            features = $LOADED_FEATURES.select { |path| path.end_with?('load_fixture.rb') }
+            features.should include(absolute_path)
+            features.should_not include(canonical_path)
+          end
+        end
+
         it "stores the same path that __FILE__ returns in the required file" do
           symlink_path = "#{@symlink_basename}/load_fixture_and__FILE__.rb"
           touch(@requiring_file) { |f|

spec/ruby/core/kernel/shared/require.rb

@@ -403,6 +403,19 @@
           $".last.should == loaded_feature
           $LOAD_PATH[0].should == @symlink_to_dir
         end
+
+        ruby_version_is "3.1" do
+          it "requires the realpath only once" do
+            $LOAD_PATH.unshift(@symlink_to_dir)
+            @object.require("symfile").should be_true
+            loaded_feature = "#{@dir}/symfile.rb"
+            ScratchPad.recorded.should == [loaded_feature]
+            $".last.should == loaded_feature
+
+            @object.require("realfile").should be_false
+            ScratchPad.recorded.should == [loaded_feature]
+          end
+        end
       end
     end
 

src/main/ruby/truffleruby/core/kernel.rb

@@ -231,7 +231,7 @@ def autoload?(name)
     when :feature_loaded
       false
     when :feature_found
-      Primitive.load_feature(feature, path)
+      Truffle::FeatureLoader.load_unless_realpath_loaded(feature, path)
     when :not_found
       raise Truffle::KernelOperations.load_error(feature)
     end
@@ -257,7 +257,7 @@ def require(feature)
     when :feature_loaded
       false
     when :feature_found
-      Primitive.load_feature(feature, path)
+      Truffle::FeatureLoader.load_unless_realpath_loaded(feature, path)
     when :not_found
       if lazy_rubygems
         Truffle::KernelOperations.loading_rubygems = true
@@ -290,7 +290,7 @@ def require_relative(feature)
       false
     when :feature_found
       # The first argument needs to be the expanded path here for patching to work correctly
-      Primitive.load_feature(path, path)
+      Truffle::FeatureLoader.load_unless_realpath_loaded(path, path)
     when :not_found
       raise Truffle::KernelOperations.load_error(feature)
     end

src/main/ruby/truffleruby/core/truffle/feature_loader.rb

@@ -35,6 +35,9 @@ module FeatureLoader
     # A snapshot of $LOADED_FEATURES, to check if the @loaded_features_index cache is up to date.
     @loaded_features_version = -1
 
+    @features_realpath_cache = {}
+    @loaded_features_realpaths = {}
+
     @expanded_load_path = []
     # A snapshot of $LOAD_PATH, to check if the @expanded_load_path cache is up to date.
     @load_path_version = -1
@@ -140,6 +143,16 @@ def self.find_feature_or_file(feature, use_feature_provided = true)
       end
     end
 
+    def self.load_unless_realpath_loaded(feature, path)
+      # TODO: does this need to be synchronized?
+      realpath = (@features_realpath_cache[path] ||= File.realpath(path) || path)
+      return false if @loaded_features_realpaths.key?(realpath)
+
+      result = Primitive.load_feature(feature, path)
+      @loaded_features_realpaths[realpath] = path
+      result
+    end
+
     def self.expanded_path_provided(path, ext, use_feature_provided)
       if use_feature_provided && feature_provided?(path, true)
         [:feature_loaded, path, ext]
@@ -292,13 +305,24 @@ def self.get_loaded_features_index
       unless @loaded_features_version == $LOADED_FEATURES.version
         raise '$LOADED_FEATURES is frozen; cannot append feature' if $LOADED_FEATURES.frozen?
         @loaded_features_index.clear
+        previous_realpaths = @features_realpath_cache.dup
+        @features_realpath_cache.clear
+        @loaded_features_realpaths.clear
         $LOADED_FEATURES.map! do |val|
           val = StringValue(val)
           #val.freeze # TODO freeze these but post-boot.rb issue using replace
           val
         end
         $LOADED_FEATURES.each_with_index do |val, idx|
           features_index_add(val, idx)
+          # TODO: do we need to do this in a separate loop on a copy to avoid
+          # concurrency issues? it's already called from with_synchronized_features.
+          # https://github.com/ruby/ruby/pull/4887/commits/972f2744d8145db965f1c4218313bd200ea0a740#:~:text=To%20avoid%20concurrency%20issues%20when%20rebuilding%20the%20loaded%20features%0Aindex%2C%20the%20building%20of%20the%20index%20itself%20is%20left%20alone%2C%20and%0Aafterwards%2C%20a%20separate%20loop%20is%20done%20on%20a%20copy%20of%20the%20loaded%20feature%0Asnapshot%20in%20order%20to%20rebuild%20the%20realpaths%20hash.
+        # end
+        # $LOADED_FEATURES.dup.each_with_index do |val, idx|
+          realpath = previous_realpaths[val] || (File.exist?(val) ? File.realpath(val) : val)
+          @features_realpath_cache[val] = realpath
+          @loaded_features_realpaths[realpath] = val
         end
         @loaded_features_version = $LOADED_FEATURES.version
       end