[GR-18163] Implement thread-safe autoloading

CHANGELOG.md

@@ -21,6 +21,7 @@ Compatibility:
 * Add `String#bytesplice` (#3039, @itarato).
 * Add `String#byteindex` and `String#byterindex` (#3039, @itarato).
 * Add implementations of `rb_proc_call_with_block`, `rb_proc_call_kw`, `rb_proc_call_with_block_kw` and `rb_funcall_with_block_kw` (#3068, @andrykonchin).
+* Make `autoload` thread-safe, that is only publish the autoloaded constant once the file is fully loaded (#2431, #3040, @eregon).
 
 Performance:
 

spec/mspec/lib/mspec/matchers/have_class_variable.rb

@@ -1,8 +1,11 @@
 require 'mspec/matchers/variable'
 
 class HaveClassVariableMatcher < VariableMatcher
-  self.variables_method = :class_variables
-  self.description      = 'class variable'
+  self.description = 'class variable'
+
+  private def check(object, variable)
+    object.class_variable_defined?(variable)
+  end
 end
 
 module MSpecMatchers

spec/mspec/lib/mspec/matchers/have_constant.rb

@@ -1,8 +1,12 @@
 require 'mspec/matchers/variable'
 
 class HaveConstantMatcher < VariableMatcher
-  self.variables_method = :constants
-  self.description      = 'constant'
+  self.description = 'constant'
+
+  private def check(object, variable)
+    # Differs from object.const_defined?(variable, false) for undefined constants
+    object.constants(false).include?(variable)
+  end
 end
 
 module MSpecMatchers

spec/mspec/lib/mspec/matchers/have_instance_variable.rb

@@ -1,8 +1,11 @@
 require 'mspec/matchers/variable'
 
 class HaveInstanceVariableMatcher < VariableMatcher
-  self.variables_method = :instance_variables
-  self.description      = 'instance variable'
+  self.description = 'instance variable'
+
+  private def check(object, variable)
+    object.instance_variable_defined?(variable)
+  end
 end
 
 module MSpecMatchers

spec/mspec/lib/mspec/matchers/variable.rb

@@ -1,6 +1,6 @@
 class VariableMatcher
   class << self
-    attr_accessor :variables_method, :description
+    attr_accessor :description
   end
 
   def initialize(variable)
@@ -9,7 +9,7 @@ def initialize(variable)
 
   def matches?(object)
     @object = object
-    @object.send(self.class.variables_method).include? @variable
+    check(@object, @variable)
   end
 
   def failure_message

spec/ruby/core/kernel/autoload_spec.rb

@@ -111,7 +111,7 @@ def go
   end
 end
 
-Kernel.autoload :KSAutoloadBB, "no_autoload.rb"
+Kernel.autoload :KSAutoloadBB, "autoload_never_used.rb"
 
 describe "Kernel.autoload" do
   before :all do
@@ -133,8 +133,17 @@ def go
     Kernel.autoload?(:KSAutoloadAA).should == @non_existent
   end
 
-  it "sets the autoload constant in Object's constant table" do
+  it "sets the autoload constant in the caller class's constant table" do
     Object.should have_constant(:KSAutoloadBB)
+    Kernel.should_not have_constant(:KSAutoloadBB)
+
+    module KernelSpecs
+      class AutoloadInCallerClass
+        Kernel.autoload :KSAutoloadCC, "autoload_never_used.rb"
+        AutoloadInCallerClass.should have_constant(:KSAutoloadCC)
+        Kernel.should_not have_constant(:KSAutoloadBB)
+      end
+    end
   end
 
   it "calls #to_path on non-String filenames" do
@@ -167,6 +176,9 @@ def go
   it "returns the name of the file that will be autoloaded" do
     Kernel.autoload :KSAutoload, "autoload.rb"
     Kernel.autoload?(:KSAutoload).should == "autoload.rb"
+    Kernel.should_not have_constant(:KSAutoload)
+    Object.should_not have_constant(:KSAutoload)
+    self.class.should have_constant(:KSAutoload)
   end
 
   it "returns nil if no file has been registered for a constant" do

spec/ruby/core/module/autoload_spec.rb

@@ -299,25 +299,28 @@ module ModuleSpecs::Autoload
         autoload :RequiredDirectlyNoConstant, fixture(__FILE__, "autoload_required_directly_no_constant.rb")
       end
       @path = fixture(__FILE__, "autoload_required_directly_no_constant.rb")
-      @remove << :RequiredDirectlyNoConstant
       @check = -> {
         [
             defined?(ModuleSpecs::Autoload::RequiredDirectlyNoConstant),
-            ModuleSpecs::Autoload.constants(false).include?(:RequiredDirectlyNoConstant),
             ModuleSpecs::Autoload.const_defined?(:RequiredDirectlyNoConstant),
             ModuleSpecs::Autoload.autoload?(:RequiredDirectlyNoConstant)
         ]
       }
       ScratchPad.record @check
-      @check.call.should == ["constant", true, true, @path]
+      @check.call.should == ["constant", true, @path]
       $:.push File.dirname(@path)
       begin
         require "autoload_required_directly_no_constant.rb"
       ensure
         $:.pop
       end
-      ScratchPad.recorded.should == [nil, true, false, nil]
-      @check.call.should == [nil, true, false, nil]
+      ScratchPad.recorded.should == [nil, false, nil]
+      @check.call.should == [nil, false, nil]
+
+      # undefined constant, CRuby 3.1 & 3.2 still do it here, but it is inconsistent, the constant should be removed
+      if ModuleSpecs::Autoload.constants(false).include?(:RequiredDirectlyNoConstant)
+        @remove << :RequiredDirectlyNoConstant
+      end
     end
   end
 
@@ -568,10 +571,17 @@ class LexicalScope
           LexicalScope::DeclaredInParentDefinedInCurrent.should == :declared_in_parent_defined_in_current
         end
 
-        # Basically, the parent autoload constant remains in a "undefined" state
         self.autoload?(:DeclaredInParentDefinedInCurrent).should == nil
         const_defined?(:DeclaredInParentDefinedInCurrent).should == false
         -> { DeclaredInParentDefinedInCurrent }.should raise_error(NameError)
+        ruby_version_is ""..."3.1" do
+          # Basically, the parent autoload constant remains in a "undefined" state
+          self.constants(false).should.include?(:DeclaredInParentDefinedInCurrent)
+        end
+        ruby_version_is "3.1" do
+          # The autoload constant has been removed
+          self.constants(false).should_not.include?(:DeclaredInParentDefinedInCurrent)
+        end
 
         ModuleSpecs::Autoload::LexicalScope.send(:remove_const, :DeclaredInParentDefinedInCurrent)
       end
@@ -592,13 +602,11 @@ module Autoload
             -> {
               DeclaredInCurrentDefinedInParent
             }.should complain(
-              /Expected .*autoload_callback.rb to define ModuleSpecs::Autoload::DeclaredInCurrentDefinedInParent but it didn't/,
-              verbose: true,
-            )
+              /Expected .*autoload_callback.rb to define ModuleSpecs::Autoload::DeclaredInCurrentDefinedInParent but it didn't/, verbose: true)
 
             -> {
               DeclaredInCurrentDefinedInParent
-            }.should_not complain(/.*/, verbose: true)
+            }.should_not complain(verbose: true)
             self.autoload?(:DeclaredInCurrentDefinedInParent).should == nil
             const_defined?(:DeclaredInCurrentDefinedInParent).should == false
             ModuleSpecs.const_defined?(:DeclaredInCurrentDefinedInParent).should == true
@@ -607,8 +615,9 @@ module Autoload
       end
     end
 
+    # like net/https used to do `module Net; autoload :OpenSSL, 'openssl'; end` before https://github.com/ruby/net-http/commit/369c3fd708
     ruby_version_is "3.1" do
-      it "looks up in parent scope after failed autoload" do
+      it "looks up in parent scope when declared in current and defined in parent" do
         @remove << :DeclaredInCurrentDefinedInParent
         module ModuleSpecs::Autoload
           ScratchPad.record -> {
@@ -617,10 +626,11 @@ module ModuleSpecs::Autoload
 
           class LexicalScope
             autoload :DeclaredInCurrentDefinedInParent, fixture(__FILE__, "autoload_callback.rb")
-            -> { DeclaredInCurrentDefinedInParent }.should_not raise_error(NameError)
-            # Basically, the autoload constant remains in a "undefined" state
+            DeclaredInCurrentDefinedInParent.should == :declared_in_current_defined_in_parent
+            # The autoload constant has been removed
             self.autoload?(:DeclaredInCurrentDefinedInParent).should == nil
             const_defined?(:DeclaredInCurrentDefinedInParent).should == false
+            self.constants(false).should_not.include?(:DeclaredInCurrentDefinedInParent)
             -> { const_get(:DeclaredInCurrentDefinedInParent) }.should raise_error(NameError)
           end
 
@@ -630,7 +640,7 @@ class LexicalScope
     end
 
     ruby_version_is ""..."3.1" do
-      it "and fails when finding the undefined autoload constant in the current scope when declared in current and defined in parent" do
+      it "fails when finding the undefined autoload constant in the current scope when declared in current and defined in parent" do
         @remove << :DeclaredInCurrentDefinedInParent
         module ModuleSpecs::Autoload
           ScratchPad.record -> {
@@ -643,7 +653,7 @@ class LexicalScope
             # Basically, the autoload constant remains in a "undefined" state
             self.autoload?(:DeclaredInCurrentDefinedInParent).should == nil
             const_defined?(:DeclaredInCurrentDefinedInParent).should == false
-            self.should have_constant(:DeclaredInCurrentDefinedInParent)
+            self.constants(false).should.include?(:DeclaredInCurrentDefinedInParent)
             -> { const_get(:DeclaredInCurrentDefinedInParent) }.should raise_error(NameError)
           end
 
@@ -792,14 +802,12 @@ module ModuleSpecs::Autoload
     -> { ModuleSpecs.autoload "a name", @non_existent }.should raise_error(NameError)
   end
 
-  it "shares the autoload request across dup'ed copies of modules" do
-    require fixture(__FILE__, "autoload_s.rb")
-    @remove << :S
+  it "does not share the autoload request internals across dup'ed copies of modules" do
     filename = fixture(__FILE__, "autoload_t.rb")
     mod1 = Module.new { autoload :T, filename }
-    -> {
-      ModuleSpecs::Autoload::S = mod1
-    }.should complain(/already initialized constant/)
+    ModuleSpecs::Autoload::S = mod1
+    @remove << :S
+
     mod2 = mod1.dup
 
     mod1.autoload?(:T).should == filename
@@ -905,6 +913,20 @@ class ModuleSpecs::Autoload::Z < ModuleSpecs::Autoload::ZZ
       t2_exc.should be_nil
     end
 
+    it "blocks other threads until the file is done loading so no partial modules are seen" do
+      ModuleSpecs::Autoload.autoload :ThreadSafe, fixture(__FILE__, "autoload_thread_safe.rb")
+      barrier = ModuleSpecs::CyclicBarrier.new 2
+      ScratchPad.record(barrier)
+
+      thread = Thread.new { # starts the autoload
+        ModuleSpecs::Autoload::ThreadSafe.foo.should == 42
+      }
+      barrier.await
+      ModuleSpecs::Autoload::ThreadSafe.foo.should == 42 # ThreadSafe should only be published once the whole file is loaded
+
+      thread.join
+    end
+
     # https://bugs.ruby-lang.org/issues/10892
     it "blocks others threads while doing an autoload" do
       file_path     = fixture(__FILE__, "repeated_concurrent_autoload.rb")

spec/ruby/core/module/fixtures/autoload_thread_safe.rb

@@ -0,0 +1,20 @@
+module ModuleSpecs::Autoload
+  class ThreadSafe
+    def self.bar # to illustrate partial loading
+    end
+
+    barrier = ScratchPad.recorded
+    barrier.await
+
+    # the main thread should be waiting for this file to be fully loaded
+    Thread.pass until Thread.main.stop?
+    10.times do
+      Thread.pass
+      Thread.main.should.stop?
+    end
+
+    def self.foo
+      42
+    end
+  end
+end

spec/ruby/core/module/fixtures/classes.rb

@@ -475,7 +475,7 @@ def extend_object(obj)
   end
 
   class CyclicBarrier
-    def initialize(count = 1)
+    def initialize(count)
       @count = count
       @state = 0
       @mutex = Mutex.new

spec/ruby/library/syslog/constants_spec.rb

@@ -17,7 +17,7 @@
 
       it "includes the Syslog constants" do
         @constants.each do |c|
-          Syslog::Constants.should have_constant(c)
+          Syslog::Constants.should.const_defined?(c, true)
         end
       end
     end

spec/tags/core/kernel/require_tags.txt

@@ -2,8 +2,6 @@ slow:Kernel#require (concurrently) blocks based on the path
 slow:Kernel.require (concurrently) blocks based on the path
 slow:Kernel#require ($LOADED_FEATURES) unicode_normalize is part of core and not $LOADED_FEATURES
 slow:Kernel.require ($LOADED_FEATURES) unicode_normalize is part of core and not $LOADED_FEATURES
-fails:Kernel#require (non-extensioned path) loads a .rb extensioned file when a C-extension file exists on an earlier load path
-fails:Kernel#require (non-extensioned path) does not load a feature twice when $LOAD_PATH has been modified
 slow:Kernel#require ($LOADED_FEATURES) complex, enumerator, rational, thread, ruby2_keywords are already required
 slow:Kernel.require ($LOADED_FEATURES) complex, enumerator, rational, thread, ruby2_keywords are already required
 slow:Kernel#require complex, enumerator, rational, thread, ruby2_keywords, fiber are already required

src/main/java/org/truffleruby/core/kernel/KernelNodes.java

@@ -572,7 +572,7 @@ protected static RubyDynamicObject copyable(Object object, Object freeze,
             final RubyClass selfMetaClass = metaClassNode.execute(node, object);
             if (isSingletonProfile.profile(node, selfMetaClass.isSingleton)) {
                 final RubyClass newObjectMetaClass = lazySingletonClassNode.get(node).execute(newObject);
-                newObjectMetaClass.fields.initCopy(selfMetaClass);
+                newObjectMetaClass.fields.initCopy(getContext(node), selfMetaClass, node);
             }
 
             final boolean copyFrozen = freeze instanceof Nil;

src/main/java/org/truffleruby/core/klass/ClassNodes.java

@@ -335,6 +335,14 @@ protected Object getSuperClass(RubyClass rubyClass) {
         }
     }
 
+    @Primitive(name = "class_non_singleton_class")
+    public abstract static class NonSingletonClassNode extends PrimitiveArrayArgumentsNode {
+        @Specialization
+        protected RubyClass nonSingletonClass(RubyClass rubyClass) {
+            return rubyClass.nonSingletonClass;
+        }
+    }
+
     @CoreMethod(names = { "__allocate__", "__layout_allocate__" }, constructor = true, visibility = Visibility.PRIVATE)
     public abstract static class AllocateNode extends CoreMethodArrayArgumentsNode {
         @Specialization

src/main/java/org/truffleruby/core/module/ConstantLookupResult.java

@@ -22,13 +22,14 @@ public class ConstantLookupResult {
     @CompilationFinal(dimensions = 1) private final Assumption[] assumptions;
 
     public ConstantLookupResult(RubyConstant constant, Assumption... assumptions) {
-        assert constant == null || !(constant.isAutoload() && constant.getAutoloadConstant().isAutoloadingThread());
+        assert constant == null ||
+                !(constant.isAutoload() && constant.getAutoloadConstant().isAutoloadingThreadAndUnset());
         this.constant = constant;
         this.assumptions = assumptions;
     }
 
     public boolean isFound() {
-        return constant != null && !constant.isUndefined();
+        return constant != null;
     }
 
     public boolean isDeprecated() {