rspec-dev icon indicating copy to clipboard operation
rspec-dev copied to clipboard

Thread-unsafe "autoload" code can cause constants to be missing

Open headius opened this issue 9 years ago • 6 comments

This may or may not be serious, depending on how thread-safe you consider RSpec itself to be, but it did lead to some red herring bugs in JRuby and Rubinius (https://github.com/jruby/jruby/issues/2874 and https://github.com/rubinius/rubinius/issues/2934).

This code to "autoload" a few namespaces under RSpec is not thread-safe:

  MODULES_TO_AUTOLOAD = {
    :Matchers     => "rspec/expectations",
    :Expectations => "rspec/expectations",
    :Mocks        => "rspec/mocks"
  }

  def self.const_missing(name)
    # Load rspec-expectations when RSpec::Matchers is referenced. This allows
    # people to define custom matchers (using `RSpec::Matchers.define`) before
    # rspec-core has loaded rspec-expectations (since it delays the loading of
    # it to allow users to configure a different assertion/expectation
    # framework). `autoload` can't be used since it works with ruby's built-in
    # require (e.g. for files that are available relative to a load path dir),
    # but not with rubygems' extended require.
    #
    # As of rspec 2.14.1, we no longer require `rspec/mocks` and
    # `rspec/expectations` when `rspec` is required, so we want
    # to make them available as an autoload. For more info, see:
    require MODULES_TO_AUTOLOAD.fetch(name) { return super }
    ::RSpec.const_get(name)
  end

This logic is essentially the same problem as using defined?(SomeConst) to determine whether a library has been loaded. Here's a possible sequence leading to the error above.

  • Thread A begins accessing RSpec::Matchers
  • A can't find the constant, so the const_missing triggers
  • A starts loading matchers.rb, and right away Matchers is defined (but still empty)
  • Thread B begins accessing RSpec::Matchers
  • B sees that the constant is there and proceeds to look up BuiltIn, skipping the const_missing altogether
  • B has an incomplete view of Matchers since it is still being loaded, and as a result BuiltIn (or any number of other constants and methods) is missing.

Synchronizing const_missing does not help, because once the Matchers module is defined we won't even call it. Autoload is not at fault here obviously, but neither is require logic, since no amount of locking can prevent this code from possibly seeing a partially-initialized Matchers module. RSpec needs to change how it manages these constants.

And finally, a detail I should have checked immediately...this happens in MRI too:

[] ~/projects/jruby-1.7 $ rvm ruby-2.2 do ruby blah.rb
blah.rb:11:in `block (2 levels) in <main>': uninitialized constant RSpec::Matchers::BuiltIn (NameError)

headius avatar Apr 27 '15 14:04 headius