Use realpaths in Cache @path_obj

[ojab]

If gems are loaded from symlinked directory, $LOAD_PATH has mixed symlinked
and real paths, while subsequent requires are resolved to real path, so we can
end up in situation when we rerequire the same gem with different path, require
loads this gem again and things go BOOM. for example:

From: /…/src/bootsnap/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb @ line 18 Kernel#require:

    15: def require(path)
    16:   if resolved = Bootsnap::LoadPathCache.load_path_cache.find(path)
    17:     binding.pry if path == 'set'
 => 18:     require_without_cache(resolved)
    19:   else
    20:     raise Bootsnap::LoadPathCache::CoreExt.make_load_error(path)
    21:   end
    22: rescue Bootsnap::LoadPathCache::ReturnFalse
    23:   return false
    24: rescue Bootsnap::LoadPathCache::FallbackScan
    25:   require_without_cache(path)
    26: end

[1] pry(main)> path
=> "set"
[2] pry(main)> Set.new.method(:to_s).source_location
=> ["/real_path/.rvm/rubies/ruby-2.5.0/lib/ruby/2.5.0/set.rb", 629]
[3] pry(main)> resolved
=> "/symlinked_path/.rvm/rubies/ruby-2.5.0/lib/ruby/2.5.0/set.rb"
[4] pry(main)> require_without_cache(resolved)
/symlinked_path/.rvm/rubies/ruby-2.5.0/lib/ruby/2.5.0/set.rb:625: warning: already initialized constant Set::InspectKey
/real_path/.rvm/rubies/ruby-2.5.0/lib/ruby/2.5.0/set.rb:625: warning: previous definition of InspectKey was here
=> true
[5] pry(main)> Set.new.method(:to_s).source_location
=> ["/symlinked_path/.rvm/rubies/ruby-2.5.0/lib/ruby/2.5.0/set.rb", 629]
[6] pry(main)>

[ojab]

Hopefully this fixes a bunch of already initialized constant issues that exists today, at least it fixes rails start for me.

• I'm not sure if it's MRI bug (and $LOAD_PATH should always be real paths in the first place)
• I have no idea how it can be tested
• AFAIU all subsequent requires are resolved to real path, so we only need this on $LOAD_PATH, but I can be wrong

Please advise how should I proceed with this PR.

[ojab]

#54
#75
#112
#114
#122
#123
#134

Maybe the same issue with different symptoms:
#86
#103
#106

[burke]

IIRC realpath (or the C equivalent thereof) is normally called after the point at which we've hooked require, so this would cause mismatches in some cases. (additionally, calling realpath is frustratingly expensive, though this isn't as big a deal for this initialization case).

[ojab]

Hmm, could you elaborate where how exactly we can have mismatch? AFAIU files are always resolved using realpath, but $LOAD_PATH sometimes contains symlinked paths.
So far it fixes issue for some people (#123 (comment), #112 (comment), #75 (comment), myself included).

I understand that realpath is expensive, but alternative right now is disabling bootsnap completely :(.

[ojab]

And of course I'm happy to rework this PR if other solution is more suitable, but unfortunately I haven't found such solution.

[burke]

The main case in which this is an issue is:

1. The user has the application source at some path on disk, say, /Users/me/src/github.com/org/app
2. The user symlinks the application somewhere else, say, /Users/me/app
3. The application adds (e.g.) lib to the $LOAD_PATH
4. The application uses a combination of require and require_relative to load files from lib.

For example, let's say there's a file lib/util.rb, which is required sometimes with require_relative and sometimes with require.

One file might require util.rb as require_relative '../lib/util.rb'; another as require 'util'.

If bootsnap is configured to resolve indexed paths to their realpath, then the first invocation would resolve to /Users/me/src/github.com/org/app/lib/util.rb, while the second would resolve to /Users/me/app/lib/util.rb, and the file would be double-loaded.

Here's some exploration of this issue, though it doesn't precisely reproduce the case described here:

require 'fileutils'

$info = ->(indent, msg) do
  STDERR.puts(format("%*s\x1b[1;34m%% \x1b[1;37m%s\x1b[0m", indent, "", msg))
end

$real_app_root      = 'realpath'
$symlinked_app_root = 'expanded_path'
$load_path_entry    = 'expanded_path/lib'
$some_file          = 'expanded_path/lib/some-file.rb'

at_exit do
  File.unlink($some_file)           rescue nil
  FileUtils.rmdir($load_path_entry) rescue nil
  File.unlink($symlinked_app_root)  rescue nil
  FileUtils.rmdir($real_app_root)   rescue nil
end

FileUtils.mkdir($real_app_root)
FileUtils.ln_s($real_app_root, $symlinked_app_root)
FileUtils.mkdir($load_path_entry)
File.write($some_file, 'puts "\x1b[1;31m!\x1b[0;31m evaluated contents\x1b[0m"')

$LOAD_PATH.unshift(File.expand_path($load_path_entry))

alias :req :require
def require(f)
  $info.(0, "require(#{f.inspect})")
  req(f)
end

def (RubyVM::InstructionSequence).load_iseq(f)
  $info.(2, "load_iseq(#{f.inspect})")
end

require 'some-file'
puts $LOADED_FEATURES.grep(/some-file/)

require_relative './expanded_path/lib/some-file'
puts $LOADED_FEATURES.grep(/some-file/)

require_relative './realpath/lib/some-file'
puts $LOADED_FEATURES.grep(/some-file/)

It might be possible to work around this by also hooking require_relative to also realpath its argument.

[ojab]

Idea to override $LOAD_PATH modifying methods and make all paths realpaths on insert/whatever sounds too crazy?

[ojab]

Forget it, doesn't help here.
Looks like we can't do this without realpath overhead on each require_relative :/
I'll try to think more about it and write some code this weekend.

[burke]

I think it might not be completely crazy to add that overhead to require_relative.

One other possibility would be to take the realpath of the directory containing the target of require_relative and cache that, joining the basename to the resolved/cached dirname. If we wanted to be extra safe, we could lstat the target and, if the file itself is a symlink, run a full realpath on that file to resolve it.

[burke]

Something like this might do the trick:

require 'English'

module RealpathCache
  @cache = {}

  class << self
    def call(path)
      call_rec(File.expand_path(path))
    end

    private

    def call_rec(path)
      if found = @cache[path]
        return found
      end

      # base case: FS root
      return path if File.dirname(path) == path

      if File.lstat(path).symlink?
        link_target = File.readlink(path)
        link_base   = File.dirname(path)
        expanded    = File.expand_path(link_target, link_base)
        return @cache[path] = call_rec(expanded)
      end

      File.join(
        call_rec(File.dirname(path)),
        File.basename(path)
      )
    end
  end
end

if __FILE__ == $PROGRAM_NAME
  puts RealpathCache.call(ARGV.first)
end

[ojab]

Sorry for delay (been lazy/other stuff).
Pushed version that should correctly handle require_relative w/ symlinks.
Basically it's doing the same stuff that ruby does in require_relative under the hood, but using real directory instead of possible symlink.

Do we care that required file itself can be a symlink?

[ojab]

If we wanted to be extra safe, we could lstat the target and, if the file itself is a symlink, run a full realpath on that file to resolve it.

ah, looks like we care.
But we don't have actual filename (i. e. with extension), so how can we know if file.rb or file.so is actually required? Something like CACHED_EXTENSIONS.each { |ext| File.exist?("#{file}#{ext}" } doesn't look ok for me ._.

[rlue]

Quick question, fellas — is there any way that this is related to this Ruby bug, require_relative and require should be compatible with each other when symlinks are used? It must not be the root cause, because it looks like fixes were merged before the release of v2.5.0, but the revision that fixed it sounds like it should be awful relevant:

Revision 62440

file.c: rb_check_realpath

* file.c (rb_check_realpath): returns real path which has no
  symbolic links.  similar to rb_realpath except for returning
  Qnil if any parts did not exist.

load.c: real path to load

* load.c (rb_construct_expanded_load_path): expand load paths to
  real paths to get rid of duplicate loading from symbolic-linked
  directories.  [Feature #10222]

Changes were committed to Ruby trunk September of last year (5754f15 & b6d3927), but I'm having trouble making out how exactly this patches the require and require_relative methods.

[ojab]

We're hijacking require/require_relative to load cached versions, so we're basically doing the same thing here ourself.

[ojab]

Added tests for RealpathCache and fixed an issue when required file is a symlink itself.
Anything else should be done to get it merged?

[ojab]

Pushed fixed version.

[yskkin]

As I commented in #147 (comment), I still have an error for my case.

[ojab]

Pushed version that iterates over ['', *CACHED_EXTENSIONS] to find required file.

@yskkin please check if it works for you.

[yskkin]

It works. 🎉 Thank you @ojab!

[burke]

I've pushed this as 1.3.0.beta, but it broke Shopify boot. I'll try to dig into the issue.

[wpolicarpo]

@ojab I have the same issue here when I try to start the application in a symlink'ed folder (like mina and capistrano do).

@burke 1.3.0.beta also broke the normal usage (no symlinks) for me.

[ojab]

@wpolicarpo could you create a new issue with description/backtrace and cc me?

[burke]

Ah, this is the problem! @path_obj needs to continue to be an actual reference to $LOAD_PATH or to ActiveSupport::Dependencies.autoload_paths, and any change to it needs to trigger a corresponding event in ChangeObserver.

One way to do this here would be to:

        path_obj.map! { |f| File.exist?(f) ? File.realpath(f) : f }
        @path_obj = path_obj

[burke]

It would be best too, at this point, to either:

1. Make the ChangeObserver understand map!; or
2. Make this line blow up if the ChangeObserver has bound to this object`.

[ojab]

ouch, understood, will take a look.

[ojab]

AFAICS ChangeObserver already understands map!, so .map! and we should be fine?
Any hints how it can be reproduced locally?

[burke]

Oh great then.

To reproduce, I guess:

1. load bootsnap
2. Add a new dir to the load path via, say, $LOAD_PATH.unshift
3. Observe that bootsnap will not load features from this path since the ChangeObserver hook did not fire.

An easy way to catch this is to just log or whatever from the hooked method in ChangeObserver.

[ojab]

@wpolicarpo could you try gem 'bootsnap', github: 'ojab/bootsnap', require: false?

[burke]

1.3.0.beta2 is pushed. Problem seems fixed for me.

[wpolicarpo]

@ojab it's working fine now. Thanks!

[gathuku]

Am having this issue any help.

 Unable to load application: I18n::InvalidLocaleData: can not load translations from /usr/local/bundle/gems/responders-3.0.0/lib/responders/locales/en.yml: #<NameError: uninitialized constant Bootsnap::CompileCache::ISeq>