Skip to content

Commit 0f15f5e

Browse files
eregoneregon
committed
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
1 parent 88935ce commit 0f15f5e

File tree

7 files changed

+89
-26
lines changed

7 files changed

+89
-26
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ Compatibility:
1919
* Fix `Pathname#relative_path_from` to convert string arguments to Pathname objects (@rwstauner).
2020
* Add `String#bytesplice` (#3039, @itarato).
2121
* Add `String#byteindex` and `String#byterindex` (#3039, @itarato).
22+
* Make `autoload` thread-safe, that is only publish the autoloaded constant once the file is fully loaded (#2431, #3040, @eregon).
2223

2324
Performance:
2425

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@ public class ConstantLookupResult {
2222
@CompilationFinal(dimensions = 1) private final Assumption[] assumptions;
2323

2424
public ConstantLookupResult(RubyConstant constant, Assumption... assumptions) {
25-
assert constant == null || !(constant.isAutoload() && constant.getAutoloadConstant().isAutoloadingThread());
25+
assert constant == null ||
26+
!(constant.isAutoload() && constant.getAutoloadConstant().isAutoloadingThreadAndUnset());
2627
this.constant = constant;
2728
this.assumptions = assumptions;
2829
}

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

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -480,17 +480,27 @@ private RubyConstant setConstantInternal(RubyContext context, Node currentNode,
480480
do {
481481
previousEntry = constants.get(name);
482482
previous = previousEntry != null ? previousEntry.getConstant() : null;
483-
if (autoload && previous != null) {
484-
if (previous.hasValue()) {
485-
// abort, do not set an autoload constant, the constant already has a value
486-
return null;
487-
} else if (previous.isAutoload() &&
488-
previous.getAutoloadConstant().getAutoloadPath().equals(autoloadPath)) {
489-
// already an autoload constant with the same path,
490-
// do nothing so we don't replace the AutoloadConstant#autoloadLock which might be already acquired
491-
return null;
483+
484+
if (previous != null) {
485+
if (autoload) {
486+
if (previous.hasValue()) {
487+
// abort, do not set an autoload constant, the constant already has a value
488+
return null;
489+
} else if (previous.isAutoload() &&
490+
previous.getAutoloadConstant().getAutoloadPath().equals(autoloadPath)) {
491+
// already an autoload constant with the same path,
492+
// do nothing so we don't replace the AutoloadConstant#autoloadLock which might be already acquired
493+
return null;
494+
}
495+
} else {
496+
if (previous.isAutoload() && previous.getAutoloadConstant().isAutoloadingThread() &&
497+
!previous.getAutoloadConstant().isPublished()) {
498+
previous.getAutoloadConstant().setUnpublishedValue(value);
499+
return previous;
500+
}
492501
}
493502
}
503+
494504
newConstant = newConstant(currentNode, name, value, autoloadConstant, previous);
495505
} while (!ConcurrentOperations.replace(constants, name, previousEntry, new ConstantEntry(newConstant)));
496506

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ public static Iterable<Entry<String, ConstantEntry>> getAllConstants(RubyModule
117117
/** NOTE: This method returns false for an undefined RubyConstant */
118118
public static boolean isConstantDefined(RubyConstant constant) {
119119
return constant != null && !constant.isUndefined() &&
120-
!(constant.isAutoload() && constant.getAutoloadConstant().isAutoloadingThread());
120+
!(constant.isAutoload() && constant.getAutoloadConstant().isAutoloadingThreadAndUnset());
121121
}
122122

123123
/** NOTE: This method might return an undefined RubyConstant */
@@ -128,7 +128,8 @@ private static boolean constantExists(RubyConstant constant, ArrayList<Assumptio
128128
// Cannot cache the lookup of an autoloading constant as the result depends on the calling thread
129129
assumptions.add(Assumption.NEVER_VALID);
130130
}
131-
return !constant.getAutoloadConstant().isAutoloadingThread();
131+
132+
return !constant.getAutoloadConstant().isAutoloadingThreadAndUnset();
132133
} else {
133134
return true;
134135
}
@@ -137,6 +138,7 @@ private static boolean constantExists(RubyConstant constant, ArrayList<Assumptio
137138
}
138139
}
139140

141+
/** NOTE: This method might return an undefined RubyConstant */
140142
private static boolean constantExists(ConstantEntry constantEntry, ArrayList<Assumption> assumptions) {
141143
return constantExists(constantEntry.getConstant(), assumptions);
142144
}

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ public final class AutoloadConstant {
2222
private final Object feature;
2323
private final String autoloadPath;
2424
private volatile ReentrantLock autoloadLock;
25+
private Object unpublishedValue = null;
26+
private boolean published = false;
2527

2628
public AutoloadConstant(Object feature) {
2729
assert RubyStringLibrary.getUncached().isRubyString(feature);
@@ -64,4 +66,35 @@ public boolean isAutoloadingThread() {
6466
return autoloadLock != null && autoloadLock.isHeldByCurrentThread();
6567
}
6668

69+
public boolean isAutoloadingThreadAndUnset() {
70+
return isAutoloadingThread() && !hasUnpublishedValue();
71+
}
72+
73+
public boolean hasUnpublishedValue() {
74+
assert isAutoloadingThread();
75+
return unpublishedValue != null;
76+
}
77+
78+
public Object getUnpublishedValue() {
79+
assert isAutoloadingThread();
80+
return unpublishedValue;
81+
}
82+
83+
public void setUnpublishedValue(Object unpublishedValue) {
84+
assert isAutoloadingThread();
85+
assert RubyGuards.assertIsValidRubyValue(unpublishedValue);
86+
this.unpublishedValue = unpublishedValue;
87+
}
88+
89+
public boolean isPublished() {
90+
assert isAutoloadingThread();
91+
return published;
92+
}
93+
94+
public void publish(RubyContext context, RubyConstant constant, Node node) {
95+
assert isAutoloadingThread();
96+
assert hasUnpublishedValue();
97+
this.published = true;
98+
constant.getDeclaringModule().fields.setConstant(context, node, constant.getName(), getUnpublishedValue());
99+
}
67100
}

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

Lines changed: 26 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import org.truffleruby.core.module.ModuleOperations;
2222
import org.truffleruby.core.module.RubyModule;
2323
import org.truffleruby.core.symbol.RubySymbol;
24+
import org.truffleruby.language.AutoloadConstant;
2425
import org.truffleruby.language.LexicalScope;
2526
import org.truffleruby.language.RubyBaseNode;
2627
import org.truffleruby.language.RubyConstant;
@@ -71,27 +72,33 @@ protected Object getConstant(
7172
}
7273

7374
@TruffleBoundary
74-
@Specialization(guards = { "autoloadConstant != null", "autoloadConstant.isAutoload()" })
75+
@Specialization(guards = { "constant != null", "constant.isAutoload()" })
7576
protected Object autoloadConstant(
7677
LexicalScope lexicalScope,
7778
RubyModule module,
7879
String name,
79-
RubyConstant autoloadConstant,
80+
RubyConstant constant,
8081
LookupConstantInterface lookupConstantNode,
8182
boolean callConstMissing,
8283
@Cached @Shared LazyDispatchNode constMissingNode,
8384
@Cached DispatchNode callRequireNode) {
8485

85-
final Object feature = autoloadConstant.getAutoloadConstant().getFeature();
86+
final AutoloadConstant autoloadConstant = constant.getAutoloadConstant();
87+
final Object feature = autoloadConstant.getFeature();
8688

87-
if (autoloadConstant.getAutoloadConstant().isAutoloadingThread()) {
88-
// Pretend the constant does not exist while it is autoloading
89-
return doMissingConstant(module, name, getSymbol(name), callConstMissing, constMissingNode.get(this));
89+
if (autoloadConstant.isAutoloadingThread()) {
90+
var unpublishedValue = autoloadConstant.getUnpublishedValue();
91+
if (unpublishedValue != null) {
92+
return unpublishedValue;
93+
} else {
94+
// Pretend the constant does not exist while it is autoloading
95+
return doMissingConstant(module, name, getSymbol(name), callConstMissing, constMissingNode.get(this));
96+
}
9097
}
9198

9299
final FeatureLoader featureLoader = getContext().getFeatureLoader();
93100
final String expandedPath = featureLoader
94-
.findFeature(autoloadConstant.getAutoloadConstant().getAutoloadPath());
101+
.findFeature(autoloadConstant.getAutoloadPath());
95102
if (expandedPath != null && featureLoader.getFileLocks().isCurrentThreadHoldingLock(expandedPath)) {
96103
// We found an autoload constant while we are already require-ing the autoload file,
97104
// consider it missing to avoid circular require warnings and calling #require twice.
@@ -112,20 +119,26 @@ protected Object autoloadConstant(
112119
RubyLanguage.LOGGER.info(() -> String.format(
113120
"%s: autoloading %s with %s",
114121
getContext().fileLine(getContext().getCallStack().getTopMostUserSourceSection()),
115-
autoloadConstant,
116-
autoloadConstant.getAutoloadConstant().getAutoloadPath()));
122+
constant,
123+
autoloadConstant.getAutoloadPath()));
117124
}
118125

119126
// Mark the autoload constant as loading already here and not in RequireNode so that recursive lookups act as "being loaded"
120-
autoloadConstantStart(getContext(), autoloadConstant, this);
127+
autoloadConstantStart(getContext(), constant, this);
121128
try {
122-
callRequireNode.call(coreLibrary().mainObject, "require", feature);
129+
try {
130+
callRequireNode.call(coreLibrary().mainObject, "require", feature);
131+
} finally {
132+
if (autoloadConstant.hasUnpublishedValue()) {
133+
autoloadConstant.publish(getContext(), constant, this);
134+
}
135+
}
123136

124137
// This needs to run while the autoload is marked as isAutoloading(), to avoid infinite recursion
125-
return autoloadResolveConstant(lexicalScope, module, name, autoloadConstant, lookupConstantNode,
138+
return autoloadResolveConstant(lexicalScope, module, name, constant, lookupConstantNode,
126139
callConstMissing);
127140
} finally {
128-
autoloadConstantStop(autoloadConstant);
141+
autoloadConstantStop(constant);
129142
}
130143
}
131144

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,6 @@ private boolean requireConsideringAutoload(String feature, String expandedPath,
104104
}
105105
}
106106

107-
108107
if (getContext().getOptions().LOG_AUTOLOAD && !toAutoload.isEmpty()) {
109108
String info = toAutoload
110109
.stream()
@@ -130,6 +129,10 @@ private boolean requireConsideringAutoload(String feature, String expandedPath,
130129
final List<RubyConstant> releasedConstants = featureLoader.getAutoloadConstants(expandedPath);
131130
for (RubyConstant constant : releasedConstants) {
132131
if (constant.getAutoloadConstant().isAutoloadingThread() && !alreadyAutoloading.contains(constant)) {
132+
if (constant.getAutoloadConstant().hasUnpublishedValue()) {
133+
constant.getAutoloadConstant().publish(getContext(), constant, this);
134+
}
135+
133136
final boolean undefined = GetConstantNode.autoloadUndefineConstantIfStillAutoload(constant);
134137
GetConstantNode.logAutoloadResult(getContext(), constant, undefined);
135138
GetConstantNode.autoloadConstantStop(constant);

0 commit comments

Comments
 (0)