Remove autoload for constant if the autoload fails

Previously, if an autoload failed (the file was loaded, but the
constant was not defined by the autoloaded file). Ruby will try
to autoload again if you delete the autoloaded file from
$LOADED_FEATURES.  With this change, the autoload and the
constant itself are removed as soon as it fails.

To handle cases where multiple threads are autoloading, when
deleting an autoload, handle the case where another thread
already deleted it.

Fixes [Bug #15790]

4 files changed, 121 insertions, 47 deletions

diff --git a/spec/ruby/core/module/autoload_spec.rb b/spec/ruby/core/module/autoload_spec.rb
index f17675846b..1d61646db5 100644
--- a/spec/ruby/core/module/autoload_spec.rb
+++ b/spec/ruby/core/module/autoload_spec.rb
@@ -441,21 +441,42 @@ describe "Module#autoload" do
     ScratchPad.recorded.should == [:raise, :raise]
   end
 
-  it "does not remove the constant from Module#constants if the loaded file does not define it, but leaves it as 'undefined'" do
-    path = fixture(__FILE__, "autoload_o.rb")
-    ScratchPad.record []
-    ModuleSpecs::Autoload.autoload :O, path
+  ruby_version_is "3.1" do
+    it "removes the constant from Module#constants if the loaded file does not define it" do
+      path = fixture(__FILE__, "autoload_o.rb")
+      ScratchPad.record []
+      ModuleSpecs::Autoload.autoload :O, path
 
-    ModuleSpecs::Autoload.const_defined?(:O).should == true
-    ModuleSpecs::Autoload.should have_constant(:O)
-    ModuleSpecs::Autoload.autoload?(:O).should == path
+      ModuleSpecs::Autoload.const_defined?(:O).should == true
+      ModuleSpecs::Autoload.should have_constant(:O)
+      ModuleSpecs::Autoload.autoload?(:O).should == path
 
-    -> { ModuleSpecs::Autoload::O }.should raise_error(NameError)
+      -> { ModuleSpecs::Autoload::O }.should raise_error(NameError)
 
-    ModuleSpecs::Autoload.should have_constant(:O)
-    ModuleSpecs::Autoload.const_defined?(:O).should == false
-    ModuleSpecs::Autoload.autoload?(:O).should == nil
-    -> { ModuleSpecs::Autoload.const_get(:O) }.should raise_error(NameError)
+      ModuleSpecs::Autoload.const_defined?(:O).should == false
+      ModuleSpecs::Autoload.should_not have_constant(:O)
+      ModuleSpecs::Autoload.autoload?(:O).should == nil
+      -> { ModuleSpecs::Autoload.const_get(:O) }.should raise_error(NameError)
+    end
+  end
+
+  ruby_version_is ""..."3.1" do
+    it "does not remove the constant from Module#constants if the loaded file does not define it, but leaves it as 'undefined'" do
+      path = fixture(__FILE__, "autoload_o.rb")
+      ScratchPad.record []
+      ModuleSpecs::Autoload.autoload :O, path
+
+      ModuleSpecs::Autoload.const_defined?(:O).should == true
+      ModuleSpecs::Autoload.should have_constant(:O)
+      ModuleSpecs::Autoload.autoload?(:O).should == path
+
+      -> { ModuleSpecs::Autoload::O }.should raise_error(NameError)
+
+      ModuleSpecs::Autoload.const_defined?(:O).should == false
+      ModuleSpecs::Autoload.should have_constant(:O)
+      ModuleSpecs::Autoload.autoload?(:O).should == nil
+      -> { ModuleSpecs::Autoload.const_get(:O) }.should raise_error(NameError)
+    end
   end
 
   it "does not try to load the file again if the loaded file did not define the constant" do
@@ -554,31 +575,54 @@ describe "Module#autoload" do
         # Basically, the parent autoload constant remains in a "undefined" state
         self.autoload?(:DeclaredInParentDefinedInCurrent).should == nil
         const_defined?(:DeclaredInParentDefinedInCurrent).should == false
-        self.should have_constant(:DeclaredInParentDefinedInCurrent)
         -> { DeclaredInParentDefinedInCurrent }.should raise_error(NameError)
 
         ModuleSpecs::Autoload::LexicalScope.send(:remove_const, :DeclaredInParentDefinedInCurrent)
       end
     end
 
-    it "and 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 -> {
-          DeclaredInCurrentDefinedInParent = :declared_in_current_defined_in_parent
-        }
+    ruby_version_is "3.1" do
+      it "looks up in parent scope after failed autoload" do
+        @remove << :DeclaredInCurrentDefinedInParent
+        module ModuleSpecs::Autoload
+          ScratchPad.record -> {
+            DeclaredInCurrentDefinedInParent = :declared_in_current_defined_in_parent
+          }
 
-        class LexicalScope
-          autoload :DeclaredInCurrentDefinedInParent, fixture(__FILE__, "autoload_callback.rb")
-          -> { DeclaredInCurrentDefinedInParent }.should raise_error(NameError)
-          # Basically, the autoload constant remains in a "undefined" state
-          self.autoload?(:DeclaredInCurrentDefinedInParent).should == nil
-          const_defined?(:DeclaredInCurrentDefinedInParent).should == false
-          self.should have_constant(:DeclaredInCurrentDefinedInParent)
-          -> { const_get(:DeclaredInCurrentDefinedInParent) }.should raise_error(NameError)
+          class LexicalScope
+            autoload :DeclaredInCurrentDefinedInParent, fixture(__FILE__, "autoload_callback.rb")
+            -> { DeclaredInCurrentDefinedInParent }.should_not raise_error(NameError)
+            # Basically, the autoload constant remains in a "undefined" state
+            self.autoload?(:DeclaredInCurrentDefinedInParent).should == nil
+            const_defined?(:DeclaredInCurrentDefinedInParent).should == false
+            -> { const_get(:DeclaredInCurrentDefinedInParent) }.should raise_error(NameError)
+          end
+
+          DeclaredInCurrentDefinedInParent.should == :declared_in_current_defined_in_parent
         end
+      end
+    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
+        @remove << :DeclaredInCurrentDefinedInParent
+        module ModuleSpecs::Autoload
+          ScratchPad.record -> {
+            DeclaredInCurrentDefinedInParent = :declared_in_current_defined_in_parent
+          }
+
+          class LexicalScope
+            autoload :DeclaredInCurrentDefinedInParent, fixture(__FILE__, "autoload_callback.rb")
+            -> { DeclaredInCurrentDefinedInParent }.should raise_error(NameError)
+            # Basically, the autoload constant remains in a "undefined" state
+            self.autoload?(:DeclaredInCurrentDefinedInParent).should == nil
+            const_defined?(:DeclaredInCurrentDefinedInParent).should == false
+            self.should have_constant(:DeclaredInCurrentDefinedInParent)
+            -> { const_get(:DeclaredInCurrentDefinedInParent) }.should raise_error(NameError)
+          end
 
-        DeclaredInCurrentDefinedInParent.should == :declared_in_current_defined_in_parent
+          DeclaredInCurrentDefinedInParent.should == :declared_in_current_defined_in_parent
+        end
       end
     end
 
diff --git a/spec/ruby/core/module/const_set_spec.rb b/spec/ruby/core/module/const_set_spec.rb
index b537d3f133..ba7810d17b 100644
--- a/spec/ruby/core/module/const_set_spec.rb
+++ b/spec/ruby/core/module/const_set_spec.rb
@@ -101,7 +101,7 @@ describe "Module#const_set" do
       mod.const_get(:Foo).should == 1
     end
 
-    it "does not warn if the previous value was undefined" do
+    it "does not warn after a failed autoload" do
       path = fixture(__FILE__, "autoload_o.rb")
       ScratchPad.record []
       mod = Module.new
@@ -109,7 +109,6 @@ describe "Module#const_set" do
       mod.autoload :Foo, path
       -> { mod::Foo }.should raise_error(NameError)
 
-      mod.should have_constant(:Foo)
       mod.const_defined?(:Foo).should == false
       mod.autoload?(:Foo).should == nil
 
diff --git a/test/ruby/test_autoload.rb b/test/ruby/test_autoload.rb
index 98d513da87..7709760d19 100644
--- a/test/ruby/test_autoload.rb
+++ b/test/ruby/test_autoload.rb
@@ -456,6 +456,31 @@ p Foo::Bar
     end;
   end
 
+  def test_autoload_after_failed_and_removed_from_loaded_features
+    Dir.mktmpdir('autoload') do |tmpdir|
+      autoload_path = File.join(tmpdir, "test-bug-15790.rb")
+      File.write(autoload_path, '')
+
+      assert_separately(%W[-I #{tmpdir}], <<-RUBY)
+        path = #{File.realpath(autoload_path).inspect}
+        autoload :X, path
+        assert_equal(path, Object.autoload?(:X))
+
+        assert_raise(NameError){X}
+        assert_nil(Object.autoload?(:X))
+        assert_equal(false, Object.const_defined?(:X))
+
+        $LOADED_FEATURES.delete(path)
+        assert_equal(false, Object.const_defined?(:X))
+        assert_nil(Object.autoload?(:X))
+
+        assert_raise(NameError){X}
+        assert_equal(false, Object.const_defined?(:X))
+        assert_nil(Object.autoload?(:X))
+      RUBY
+    end
+  end
+
   def add_autoload(path)
     (@autoload_paths ||= []) << path
     ::Object.class_eval {autoload(:AutoloadTest, path)}
@@ -463,7 +488,7 @@ p Foo::Bar
 
   def remove_autoload_constant
     $".replace($" - @autoload_paths)
-    ::Object.class_eval {remove_const(:AutoloadTest)}
+    ::Object.class_eval {remove_const(:AutoloadTest)} if defined? Object::AutoloadTest
     TestAutoload.class_eval {remove_const(:AutoloadTest)} if defined? TestAutoload::AutoloadTest
   end
 end
diff --git a/variable.c b/variable.c
index 8bdc733a4c..5712ccf0ba 100644
--- a/variable.c
+++ b/variable.c
@@ -2237,23 +2237,26 @@ autoload_delete(VALUE mod, ID id)
 	struct autoload_const *ac;
 
 	st_delete(tbl, &n, &load);
-	ele = get_autoload_data((VALUE)load, &ac);
-	VM_ASSERT(ele);
-	if (ele) {
-	    VM_ASSERT(!list_empty(&ele->constants));
-	}
+        /* Qfalse can indicate already deleted */
+        if (load != Qfalse) {
+            ele = get_autoload_data((VALUE)load, &ac);
+            VM_ASSERT(ele);
+            if (ele) {
+                VM_ASSERT(!list_empty(&ele->constants));
+            }
 
-	/*
-	 * we must delete here to avoid "already initialized" warnings
-	 * with parallel autoload.  Using list_del_init here so list_del
-	 * works in autoload_c_free
-	 */
-	list_del_init(&ac->cnode);
+            /*
+             * we must delete here to avoid "already initialized" warnings
+             * with parallel autoload.  Using list_del_init here so list_del
+             * works in autoload_c_free
+             */
+            list_del_init(&ac->cnode);
 
-	if (tbl->num_entries == 0) {
-	    n = autoload;
-	    st_delete(RCLASS_IV_TBL(mod), &n, &val);
-	}
+            if (tbl->num_entries == 0) {
+                n = autoload;
+                st_delete(RCLASS_IV_TBL(mod), &n, &val);
+            }
+        }
     }
 }
 
@@ -2502,7 +2505,10 @@ rb_autoload_load(VALUE mod, ID id)
     result = rb_ensure(autoload_require, (VALUE)&state,
 		       autoload_reset, (VALUE)&state);
 
-    if (flag > 0 && (ce = rb_const_lookup(mod, id))) {
+    if (!(ce = rb_const_lookup(mod, id)) || ce->value == Qundef) {
+        rb_const_remove(mod, id);
+    }
+    else if (flag > 0) {
         ce->flag |= flag;
     }
     RB_GC_GUARD(load);