Merge pull request #402 from Shopify/skip-realpath-cache-if-useless

Get rid of RealPathCache and the `Kernel.require_relative` decorator

CHANGELOG.md

@@ -1,6 +1,9 @@
 # Unreleased
 
-* Drop support for Ruby 2.3. 
+* Get rid of the `Kernel.require_relative` decorator by resolving `$LOAD_PATH` members to their real path.
+  This way we handle symlinks in `$LOAD_PATH` much more efficiently. See #402 for the detailed explanation.
+
+* Drop support for Ruby 2.3 (to allow gettind rid of the `Kernel.require_relative` decorator).
 
 # 1.10.3
 

lib/bootsnap/load_path_cache.rb

@@ -22,7 +22,7 @@ module LoadPathCache
     CACHED_EXTENSIONS = DLEXT2 ? [DOT_RB, DLEXT, DLEXT2] : [DOT_RB, DLEXT]
 
     class << self
-      attr_reader(:load_path_cache, :loaded_features_index, :realpath_cache)
+      attr_reader(:load_path_cache, :loaded_features_index)
 
       def setup(cache_path:, development_mode:)
         unless supported?
@@ -33,7 +33,6 @@ def setup(cache_path:, development_mode:)
         store = Store.new(cache_path)
 
         @loaded_features_index = LoadedFeaturesIndex.new
-        @realpath_cache = RealpathCache.new
 
         @load_path_cache = Cache.new(store, $LOAD_PATH, development_mode: development_mode)
         require_relative("load_path_cache/core_ext/kernel_require")
@@ -55,5 +54,4 @@ def supported?
   require_relative("load_path_cache/store")
   require_relative("load_path_cache/change_observer")
   require_relative("load_path_cache/loaded_features_index")
-  require_relative("load_path_cache/realpath_cache")
 end

lib/bootsnap/load_path_cache/cache.rb

@@ -142,6 +142,8 @@ def push_paths_locked(*paths)
             @has_relative_paths = true if p.relative?
             next if p.non_directory?
 
+            p = p.to_realpath
+
             expanded_path = p.expanded_path
             entries, dirs = p.entries_and_dirs(@store)
             # push -> low precedence -> set only if unset
@@ -157,6 +159,8 @@ def unshift_paths_locked(*paths)
             p = Path.new(path)
             next if p.non_directory?
 
+            p = p.to_realpath
+
             expanded_path = p.expanded_path
             entries, dirs = p.entries_and_dirs(@store)
             # unshift -> high precedence -> unconditional set

lib/bootsnap/load_path_cache/core_ext/kernel_require.rb

@@ -33,15 +33,6 @@ def require(path)
     end
   end
 
-  alias_method(:require_relative_without_bootsnap, :require_relative)
-  def require_relative(path)
-    location = caller_locations(1..1).first
-    realpath = Bootsnap::LoadPathCache.realpath_cache.call(
-      location.absolute_path || location.path, path
-    )
-    require(realpath)
-  end
-
   alias_method(:load_without_bootsnap, :load)
   def load(path, wrap = false)
     if (resolved = Bootsnap::LoadPathCache.load_path_cache.find(path, try_extensions: false))

lib/bootsnap/load_path_cache/path.rb

@@ -21,8 +21,26 @@ def volatile?
 
       attr_reader(:path)
 
-      def initialize(path)
+      def initialize(path, real: false)
         @path = path.to_s.freeze
+        @real = real
+      end
+
+      def to_realpath
+        return self if @real
+
+        realpath = begin
+          File.realpath(path)
+        rescue Errno::ENOENT
+          return self
+        end
+
+        if realpath != path
+          Path.new(realpath, real: true)
+        else
+          @real = true
+          self
+        end
       end
 
       # True if the path exists, but represents a non-directory object
@@ -62,7 +80,11 @@ def entries_and_dirs(store)
       end
 
       def expanded_path
-        File.expand_path(path).freeze
+        if @real
+          path
+        else
+          @expanded_path ||= File.expand_path(path).freeze
+        end
       end
 
       private

test/integration/kernel_test.rb

@@ -0,0 +1,214 @@
+# frozen_string_literal: true
+
+require("test_helper")
+
+module Bootsnap
+  class KernelTest < Minitest::Test
+    include TmpdirHelper
+
+    def test_require_symlinked_file_twice
+      setup_symlinked_files
+      if RUBY_VERSION >= "3.1"
+        # Fixed in https://github.com/ruby/ruby/commit/79a4484a072e9769b603e7b4fbdb15b1d7eccb15 (Ruby 3.1)
+        assert_both_pass(<<~RUBY)
+          require "symlink/test"
+          require "real/test"
+        RUBY
+      else
+        assert_both_pass(<<~RUBY)
+          require "symlink/test"
+          begin
+            require "real/test"
+          rescue RuntimeError
+            exit 0
+          else
+            exit 1
+          end
+        RUBY
+      end
+    end
+
+    def test_require_symlinked_file_twice_aliased
+      setup_symlinked_files
+      assert_both_pass(<<~RUBY)
+        $LOAD_PATH.unshift(File.expand_path("symlink"))
+        require "test"
+
+        $LOAD_PATH.unshift(File.expand_path("a"))
+        require "test"
+      RUBY
+    end
+
+    def test_require_relative_symlinked_file_twice
+      setup_symlinked_files
+      if RUBY_VERSION >= "3.1"
+        # Fixed in https://github.com/ruby/ruby/commit/79a4484a072e9769b603e7b4fbdb15b1d7eccb15 (Ruby 3.1)
+        assert_both_pass(<<~RUBY)
+          require_relative "symlink/test"
+          require_relative "real/test"
+        RUBY
+      else
+        assert_both_pass(<<~RUBY)
+          require_relative "symlink/test"
+          begin
+            require_relative "real/test"
+          rescue RuntimeError
+            exit 0
+          else
+            exit 1
+          end
+        RUBY
+      end
+    end
+
+    def test_require_and_then_require_relative_symlinked_file
+      setup_symlinked_files
+      assert_both_pass(<<~RUBY)
+        $LOAD_PATH.unshift(File.expand_path("symlink"))
+        require "test"
+
+        require_relative "real/test"
+      RUBY
+    end
+
+    def test_require_relative_and_then_require_symlinked_file
+      setup_symlinked_files
+      assert_both_pass(<<~RUBY)
+        require_relative "real/test"
+
+        $LOAD_PATH.unshift(File.expand_path("symlink"))
+        require "test"
+      RUBY
+    end
+
+    def test_require_deep_symlinked_file_twice
+      setup_symlinked_files
+      if RUBY_VERSION >= "3.1"
+        # Fixed in https://github.com/ruby/ruby/commit/79a4484a072e9769b603e7b4fbdb15b1d7eccb15 (Ruby 3.1)
+        assert_both_pass(<<~RUBY)
+          require "symlink/dir/deep"
+          require "real/dir/deep"
+        RUBY
+      else
+        assert_both_pass(<<~RUBY)
+          require "symlink/dir/deep"
+          begin
+            require "real/dir/deep"
+          rescue RuntimeError
+            exit 0
+          else
+            exit 1
+          end
+        RUBY
+      end
+    end
+
+    def test_require_deep_symlinked_file_twice_aliased
+      setup_symlinked_files
+      assert_both_pass(<<~RUBY)
+        $LOAD_PATH.unshift(File.expand_path("symlink"))
+        require "dir/deep"
+
+        $LOAD_PATH.unshift(File.expand_path("a"))
+        require "dir/deep"
+      RUBY
+    end
+
+    def test_require_relative_deep_symlinked_file_twice
+      setup_symlinked_files
+      if RUBY_VERSION >= "3.1"
+        # Fixed in https://github.com/ruby/ruby/commit/79a4484a072e9769b603e7b4fbdb15b1d7eccb15 (Ruby 3.1)
+        assert_both_pass(<<~RUBY)
+          require_relative "symlink/dir/deep"
+          require_relative "real/dir/deep"
+        RUBY
+      else
+        assert_both_pass(<<~RUBY)
+          require_relative "symlink/dir/deep"
+          begin
+            require_relative "real/dir/deep"
+          rescue RuntimeError
+            exit 0
+          else
+            exit 1
+          end
+        RUBY
+      end
+    end
+
+    def test_require_and_then_require_relative_deep_symlinked_file
+      setup_symlinked_files
+      assert_both_pass(<<~RUBY)
+        $LOAD_PATH.unshift(File.expand_path("symlink"))
+        require "dir/deep"
+
+        require_relative "real/dir/deep"
+      RUBY
+    end
+
+    def test_require_relative_and_then_require_deep_symlinked_file
+      setup_symlinked_files
+      assert_both_pass(<<~RUBY)
+        require_relative "real/dir/deep"
+
+        $LOAD_PATH.unshift(File.expand_path("symlink"))
+        require "dir/deep"
+      RUBY
+    end
+
+    private
+
+    def assert_both_pass(source)
+      Help.set_file("without_bootsnap.rb", source)
+      unless execute("without_bootsnap.rb", "debug.txt")
+        flunk "expected snippet to pass WITHOUT bootsnap enabled:\n#{debug_output}"
+      end
+
+      Help.set_file("with_bootsnap.rb", %{require "bootsnap/setup"\n#{source}})
+      unless execute("with_bootsnap.rb", "debug.txt")
+        flunk "expected snippet to pass WITH bootsnap enabled:\n#{debug_output}"
+      end
+    end
+
+    def debug_output
+      File.read("debug.txt")
+    rescue Errno::ENOENT
+    end
+
+    def execute(script_path, output_path)
+      system(
+        {"BOOTSNAP_CACHE_DIR" => "tmp/cache"},
+        RbConfig.ruby, "-I.", script_path,
+        out: output_path, err: output_path
+      )
+    end
+
+    def assert_successful(source)
+      Help.set_file("test_case.rb", source)
+
+      Help.set_file("test_case.rb", %{require "bootsnap/setup"\n#{source}})
+      assert system({"BOOTSNAP_CACHE_DIR" => "tmp/cache"}, RbConfig.ruby, "-I.", "test_case.rb")
+    end
+
+    def setup_symlinked_files
+      skip("Platform doesn't support symlinks") unless File.respond_to?(:symlink)
+
+      Help.set_file("real/test.rb", <<-RUBY)
+        if $test_already_required
+          raise "test.rb required more than once"
+        else
+          $test_already_required = true
+        end
+      RUBY
+
+      Help.set_file("real/dir/deep.rb", <<-RUBY)
+        if $deep_already_required
+          raise "deep.rb required more than once"
+        else
+          $deep_already_required = true
+        end
+      RUBY
+      File.symlink("real", "symlink")
+    end
+  end
+end