diff --git a/CHANGELOG.md b/CHANGELOG.md index a984e19b4026..7808ccf7ccb9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ Compatibility: * Fix `Pathname#relative_path_from` to convert string arguments to Pathname objects (@rwstauner). * Add `String#bytesplice` (#3039, @itarato). * Add `String#byteindex` and `String#byterindex` (#3039, @itarato). +* Make `autoload` thread-safe, that is only publish the autoloaded constant once the file is fully loaded (#2431, #3040, @eregon). Performance: diff --git a/src/main/java/org/truffleruby/core/module/ConstantLookupResult.java b/src/main/java/org/truffleruby/core/module/ConstantLookupResult.java index 408059dc5e61..1caddd5748f5 100644 --- a/src/main/java/org/truffleruby/core/module/ConstantLookupResult.java +++ b/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; } diff --git a/src/main/java/org/truffleruby/core/module/ModuleFields.java b/src/main/java/org/truffleruby/core/module/ModuleFields.java index 109fffa9a9d6..09ee5113e357 100644 --- a/src/main/java/org/truffleruby/core/module/ModuleFields.java +++ b/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))); diff --git a/src/main/java/org/truffleruby/core/module/ModuleOperations.java b/src/main/java/org/truffleruby/core/module/ModuleOperations.java index 9e87b02666d1..977e49053fac 100644 --- a/src/main/java/org/truffleruby/core/module/ModuleOperations.java +++ b/src/main/java/org/truffleruby/core/module/ModuleOperations.java @@ -117,7 +117,7 @@ public static Iterable> 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 assumptions) { return constantExists(constantEntry.getConstant(), assumptions); } diff --git a/src/main/java/org/truffleruby/language/AutoloadConstant.java b/src/main/java/org/truffleruby/language/AutoloadConstant.java index 0849821f87b5..f9c342afe0b8 100644 --- a/src/main/java/org/truffleruby/language/AutoloadConstant.java +++ b/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()); + } } diff --git a/src/main/java/org/truffleruby/language/constants/GetConstantNode.java b/src/main/java/org/truffleruby/language/constants/GetConstantNode.java index e992fbc01d63..9d856c6e83d6 100644 --- a/src/main/java/org/truffleruby/language/constants/GetConstantNode.java +++ b/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); } } diff --git a/src/main/java/org/truffleruby/language/loader/RequireNode.java b/src/main/java/org/truffleruby/language/loader/RequireNode.java index d8b696be062c..659905faec51 100644 --- a/src/main/java/org/truffleruby/language/loader/RequireNode.java +++ b/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 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);