Implement thread-safe autoloading

* We stash the value of the constant being autoloaded in AutoloadConstant
  and only publish it once the autoload ends.
* Fixes #2431
  and #3040

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:
 

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

@@ -22,7 +22,8 @@ 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;
     }

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

@@ -480,17 +480,27 @@ private RubyConstant setConstantInternal(RubyContext context, Node currentNode,
         do {
             previousEntry = constants.get(name);
             previous = previousEntry != null ? previousEntry.getConstant() : null;
-            if (autoload && previous != null) {
-                if (previous.hasValue()) {
-                    // abort, do not set an autoload constant, the constant already has a value
-                    return null;
-                } else if (previous.isAutoload() &&
-                        previous.getAutoloadConstant().getAutoloadPath().equals(autoloadPath)) {
-                    // already an autoload constant with the same path,
-                    // do nothing so we don't replace the AutoloadConstant#autoloadLock which might be already acquired
-                    return null;
+
+            if (previous != null) {
+                if (autoload) {
+                    if (previous.hasValue()) {
+                        // abort, do not set an autoload constant, the constant already has a value
+                        return null;
+                    } else if (previous.isAutoload() &&
+                            previous.getAutoloadConstant().getAutoloadPath().equals(autoloadPath)) {
+                        // already an autoload constant with the same path,
+                        // do nothing so we don't replace the AutoloadConstant#autoloadLock which might be already acquired
+                        return null;
+                    }
+                } else {
+                    if (previous.isAutoload() && previous.getAutoloadConstant().isAutoloadingThread() &&
+                            !previous.getAutoloadConstant().isPublished()) {
+                        previous.getAutoloadConstant().setUnpublishedValue(value);
+                        return previous;
+                    }
                 }
             }
+
             newConstant = newConstant(currentNode, name, value, autoloadConstant, previous);
         } while (!ConcurrentOperations.replace(constants, name, previousEntry, new ConstantEntry(newConstant)));
 

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

@@ -117,7 +117,7 @@ public static Iterable<Entry<String, ConstantEntry>> getAllConstants(RubyModule
     /** NOTE: This method returns false for an undefined RubyConstant */
     public static boolean isConstantDefined(RubyConstant constant) {
         return constant != null && !constant.isUndefined() &&
-                !(constant.isAutoload() && constant.getAutoloadConstant().isAutoloadingThread());
+                !(constant.isAutoload() && constant.getAutoloadConstant().isAutoloadingThreadAndUnset());
     }
 
     /** NOTE: This method might return an undefined RubyConstant */
@@ -128,7 +128,8 @@ private static boolean constantExists(RubyConstant constant, ArrayList<Assumptio
                     // Cannot cache the lookup of an autoloading constant as the result depends on the calling thread
                     assumptions.add(Assumption.NEVER_VALID);
                 }
-                return !constant.getAutoloadConstant().isAutoloadingThread();
+
+                return !constant.getAutoloadConstant().isAutoloadingThreadAndUnset();
             } else {
                 return true;
             }
@@ -137,6 +138,7 @@ private static boolean constantExists(RubyConstant constant, ArrayList<Assumptio
         }
     }
 
+    /** NOTE: This method might return an undefined RubyConstant */
     private static boolean constantExists(ConstantEntry constantEntry, ArrayList<Assumption> assumptions) {
         return constantExists(constantEntry.getConstant(), assumptions);
     }

src/main/java/org/truffleruby/language/AutoloadConstant.java

@@ -22,6 +22,8 @@ public final class AutoloadConstant {
     private final Object feature;
     private final String autoloadPath;
     private volatile ReentrantLock autoloadLock;
+    private Object unpublishedValue = null;
+    private boolean published = false;
 
     public AutoloadConstant(Object feature) {
         assert RubyStringLibrary.getUncached().isRubyString(feature);
@@ -64,4 +66,35 @@ public boolean isAutoloadingThread() {
         return autoloadLock != null && autoloadLock.isHeldByCurrentThread();
     }
 
+    public boolean isAutoloadingThreadAndUnset() {
+        return isAutoloadingThread() && !hasUnpublishedValue();
+    }
+
+    public boolean hasUnpublishedValue() {
+        assert isAutoloadingThread();
+        return unpublishedValue != null;
+    }
+
+    public Object getUnpublishedValue() {
+        assert isAutoloadingThread();
+        return unpublishedValue;
+    }
+
+    public void setUnpublishedValue(Object unpublishedValue) {
+        assert isAutoloadingThread();
+        assert RubyGuards.assertIsValidRubyValue(unpublishedValue);
+        this.unpublishedValue = unpublishedValue;
+    }
+
+    public boolean isPublished() {
+        assert isAutoloadingThread();
+        return published;
+    }
+
+    public void publish(RubyContext context, RubyConstant constant, Node node) {
+        assert isAutoloadingThread();
+        assert hasUnpublishedValue();
+        this.published = true;
+        constant.getDeclaringModule().fields.setConstant(context, node, constant.getName(), getUnpublishedValue());
+    }
 }

src/main/java/org/truffleruby/language/constants/GetConstantNode.java

@@ -21,6 +21,7 @@
 import org.truffleruby.core.module.ModuleOperations;
 import org.truffleruby.core.module.RubyModule;
 import org.truffleruby.core.symbol.RubySymbol;
+import org.truffleruby.language.AutoloadConstant;
 import org.truffleruby.language.LexicalScope;
 import org.truffleruby.language.RubyBaseNode;
 import org.truffleruby.language.RubyConstant;
@@ -71,27 +72,33 @@ protected Object getConstant(
     }
 
     @TruffleBoundary
-    @Specialization(guards = { "autoloadConstant != null", "autoloadConstant.isAutoload()" })
+    @Specialization(guards = { "constant != null", "constant.isAutoload()" })
     protected Object autoloadConstant(
             LexicalScope lexicalScope,
             RubyModule module,
             String name,
-            RubyConstant autoloadConstant,
+            RubyConstant constant,
             LookupConstantInterface lookupConstantNode,
             boolean callConstMissing,
             @Cached @Shared LazyDispatchNode constMissingNode,
             @Cached DispatchNode callRequireNode) {
 
-        final Object feature = autoloadConstant.getAutoloadConstant().getFeature();
+        final AutoloadConstant autoloadConstant = constant.getAutoloadConstant();
+        final Object feature = autoloadConstant.getFeature();
 
-        if (autoloadConstant.getAutoloadConstant().isAutoloadingThread()) {
-            // Pretend the constant does not exist while it is autoloading
-            return doMissingConstant(module, name, getSymbol(name), callConstMissing, constMissingNode.get(this));
+        if (autoloadConstant.isAutoloadingThread()) {
+            var unpublishedValue = autoloadConstant.getUnpublishedValue();
+            if (unpublishedValue != null) {
+                return unpublishedValue;
+            } else {
+                // Pretend the constant does not exist while it is autoloading
+                return doMissingConstant(module, name, getSymbol(name), callConstMissing, constMissingNode.get(this));
+            }
         }
 
         final FeatureLoader featureLoader = getContext().getFeatureLoader();
         final String expandedPath = featureLoader
-                .findFeature(autoloadConstant.getAutoloadConstant().getAutoloadPath());
+                .findFeature(autoloadConstant.getAutoloadPath());
         if (expandedPath != null && featureLoader.getFileLocks().isCurrentThreadHoldingLock(expandedPath)) {
             // We found an autoload constant while we are already require-ing the autoload file,
             // consider it missing to avoid circular require warnings and calling #require twice.
@@ -112,20 +119,26 @@ protected Object autoloadConstant(
             RubyLanguage.LOGGER.info(() -> String.format(
                     "%s: autoloading %s with %s",
                     getContext().fileLine(getContext().getCallStack().getTopMostUserSourceSection()),
-                    autoloadConstant,
-                    autoloadConstant.getAutoloadConstant().getAutoloadPath()));
+                    constant,
+                    autoloadConstant.getAutoloadPath()));
         }
 
         // Mark the autoload constant as loading already here and not in RequireNode so that recursive lookups act as "being loaded"
-        autoloadConstantStart(getContext(), autoloadConstant, this);
+        autoloadConstantStart(getContext(), constant, this);
         try {
-            callRequireNode.call(coreLibrary().mainObject, "require", feature);
+            try {
+                callRequireNode.call(coreLibrary().mainObject, "require", feature);
+            } finally {
+                if (autoloadConstant.hasUnpublishedValue()) {
+                    autoloadConstant.publish(getContext(), constant, this);
+                }
+            }
 
             // This needs to run while the autoload is marked as isAutoloading(), to avoid infinite recursion
-            return autoloadResolveConstant(lexicalScope, module, name, autoloadConstant, lookupConstantNode,
+            return autoloadResolveConstant(lexicalScope, module, name, constant, lookupConstantNode,
                     callConstMissing);
         } finally {
-            autoloadConstantStop(autoloadConstant);
+            autoloadConstantStop(constant);
         }
     }
 

src/main/java/org/truffleruby/language/loader/RequireNode.java

@@ -104,7 +104,6 @@ private boolean requireConsideringAutoload(String feature, String expandedPath,
                 }
             }
 
-
             if (getContext().getOptions().LOG_AUTOLOAD && !toAutoload.isEmpty()) {
                 String info = toAutoload
                         .stream()
@@ -130,6 +129,10 @@ private boolean requireConsideringAutoload(String feature, String expandedPath,
             final List<RubyConstant> releasedConstants = featureLoader.getAutoloadConstants(expandedPath);
             for (RubyConstant constant : releasedConstants) {
                 if (constant.getAutoloadConstant().isAutoloadingThread() && !alreadyAutoloading.contains(constant)) {
+                    if (constant.getAutoloadConstant().hasUnpublishedValue()) {
+                        constant.getAutoloadConstant().publish(getContext(), constant, this);
+                    }
+
                     final boolean undefined = GetConstantNode.autoloadUndefineConstantIfStillAutoload(constant);
                     GetConstantNode.logAutoloadResult(getContext(), constant, undefined);
                     GetConstantNode.autoloadConstantStop(constant);