[GR-26850] Fix synchronization for concurrent Module#autoload on the same constant

PullRequest: truffleruby/2097

Author: eregon

src/main/java/org/truffleruby/core/array/ArrayNodes.java

@@ -1591,7 +1591,7 @@ private RubyString finishPack(int formatLength, BytesResult result) {
 
         @TruffleBoundary
         protected RootCallTarget compileFormat(RubyString format) {
-            return new PackCompiler(getContext(), this).compile(format.toString());
+            return new PackCompiler(getContext(), this).compile(format.getJavaString());
         }
 
         protected int getCacheLimit() {

src/main/java/org/truffleruby/core/exception/CoreExceptions.java

@@ -89,6 +89,14 @@ public void showExceptionIfDebug(RubyClass rubyClass, Object message, Backtrace
         }
     }
 
+    public String inspectReceiver(Object receiver) {
+        RubyString rubyString = (RubyString) context.send(
+                context.getCoreLibrary().truffleExceptionOperationsModule,
+                "receiver_string",
+                receiver);
+        return rubyString.getJavaString();
+    }
+
     // ArgumentError
 
     public RubyException argumentErrorOneHashRequired(Node currentNode) {
@@ -488,17 +496,17 @@ public RubyException typeErrorCantConvertInto(Object from, String toClass, Node
 
     @TruffleBoundary
     public RubyException typeErrorIsNotA(Object value, String expectedType, Node currentNode) {
-        return typeErrorIsNotA(value.toString(), expectedType, currentNode);
+        return typeErrorIsNotA(inspectReceiver(value), expectedType, currentNode);
     }
 
     @TruffleBoundary
     public RubyException typeErrorIsNotA(String value, String expectedType, Node currentNode) {
-        return typeError(StringUtils.format("%s is not a %s", value, expectedType), currentNode);
+        return typeError(value + " is not a " + expectedType, currentNode);
     }
 
     @TruffleBoundary
     public RubyException typeErrorIsNotAClassModule(Object value, Node currentNode) {
-        return typeError(StringUtils.format("%s is not a class/module", value), currentNode);
+        return typeError(inspectReceiver(value) + " is not a class/module", currentNode);
     }
 
     @TruffleBoundary

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

@@ -343,6 +343,10 @@ public RubyConstant setConstant(RubyContext context, Node currentNode, String na
     @TruffleBoundary
     public void setAutoloadConstant(RubyContext context, Node currentNode, String name, RubyString filename) {
         RubyConstant autoloadConstant = setConstantInternal(context, currentNode, name, filename, true);
+        if (autoloadConstant == null) {
+            return;
+        }
+
         if (context.getOptions().LOG_AUTOLOAD) {
             RubyLanguage.LOGGER.info(() -> String.format(
                     "%s: setting up autoload %s with %s",
@@ -351,7 +355,7 @@ public void setAutoloadConstant(RubyContext context, Node currentNode, String na
                     filename));
         }
         final ReentrantLockFreeingMap<String> fileLocks = getContext().getFeatureLoader().getFileLocks();
-        final ReentrantLock lock = fileLocks.get(filename.toString());
+        final ReentrantLock lock = fileLocks.get(filename.getJavaString());
         if (lock.isLocked()) {
             // We need to handle the new autoload constant immediately
             // if Object.autoload(name, filename) is executed from filename.rb
@@ -367,10 +371,22 @@ private RubyConstant setConstantInternal(RubyContext context, Node currentNode,
 
         SharedObjects.propagate(context, rubyModule, value);
 
+        final String autoloadPath = autoload ? ((RubyString) value).getJavaString() : null;
         RubyConstant previous;
         RubyConstant newConstant;
         do {
             previous = constants.get(name);
+            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;
+                }
+            }
             newConstant = newConstant(currentNode, name, value, autoload, previous);
         } while (!ConcurrentOperations.replace(constants, name, previous, newConstant));
 

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

@@ -580,11 +580,7 @@ protected Object autoload(RubyModule module, String name, RubyString filename) {
                 throw new RaiseException(getContext(), coreExceptions().argumentError("empty file name", this));
             }
 
-            final RubyConstant constant = module.fields.getConstant(name);
-            if (constant == null || !constant.hasValue()) {
-                module.fields.setAutoloadConstant(getContext(), this, name, filename);
-            }
-
+            module.fields.setAutoloadConstant(getContext(), this, name, filename);
             return nil;
         }
     }

src/main/java/org/truffleruby/core/regexp/MatchDataNodes.java

@@ -389,11 +389,11 @@ private int getBackRefFromRope(RubyMatchData matchData, Object index, Rope value
                         0,
                         value.byteLength(),
                         matchData.region);
-            } catch (final ValueException e) {
+            } catch (ValueException e) {
                 throw new RaiseException(
                         getContext(),
                         coreExceptions().indexError(
-                                StringUtils.format("undefined group name reference: %s", index.toString()),
+                                StringUtils.format("undefined group name reference: %s", index),
                                 this));
             }
         }

src/main/java/org/truffleruby/core/rope/ManagedRope.java

@@ -54,12 +54,14 @@ public final byte[] getBytes() {
 
     @Override
     public final String toString() {
-        assert ALLOW_TO_STRING;
-
-        final byte[] bytesBefore = bytes;
-        final String string = RopeOperations.decodeOrEscapeBinaryRope(this, RopeOperations.flattenBytes(this));
-        assert bytes == bytesBefore : "bytes should not be modified by Rope#toString() as otherwise inspecting a Rope would have a side effect";
-        return string;
+        if (DEBUG_ROPE_BYTES) {
+            final byte[] bytesBefore = bytes;
+            final String string = RopeOperations.decodeOrEscapeBinaryRope(this, RopeOperations.flattenBytes(this));
+            assert bytes == bytesBefore : "bytes should not be modified by Rope#toString() as otherwise inspecting a Rope would have a side effect";
+            return string;
+        } else {
+            return RopeOperations.decodeRope(this);
+        }
     }
 
 }

src/main/java/org/truffleruby/core/rope/NativeRope.java

@@ -184,7 +184,6 @@ public int hashCode() {
 
     @Override
     public String toString() {
-        assert ALLOW_TO_STRING;
         return toLeafRope().toString();
     }
 

src/main/java/org/truffleruby/core/rope/Rope.java

@@ -20,8 +20,8 @@ public abstract class Rope implements Comparable<Rope> {
     // NativeRope, RepeatingRope, 3 LeafRope, ConcatRope, SubstringRope, 1 LazyRope
     public static final int NUMBER_OF_CONCRETE_CLASSES = 8;
 
-    // Useful for debugging. Setting to false allow to catch wrong usages.
-    protected static final boolean ALLOW_TO_STRING = true;
+    // Useful for debugging. Setting to true avoids ManagedRope#toString to populate bytes as a side-effect of the debugger calling toString().
+    protected static final boolean DEBUG_ROPE_BYTES = false;
 
     public final Encoding encoding;
     private final int byteLength;

src/main/java/org/truffleruby/core/thread/ThreadManager.java

@@ -315,6 +315,8 @@ private void threadMain(RubyThread thread, Node currentNode, Supplier<Object> ta
             final String message = StringUtils
                     .format("%s terminated with internal error:", Thread.currentThread().getName());
             final RuntimeException runtimeException = new RuntimeException(message, e);
+            // Immediately print internal exceptions, in case they would cause a deadlock
+            runtimeException.printStackTrace();
             rethrowOnMainThread(currentNode, runtimeException);
             setThreadValue(context, thread, Nil.INSTANCE);
         } finally {

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

@@ -209,10 +209,12 @@ private void driveArrivalAtPhaser() {
     }
 
     private void printStacktracesOfBlockedThreads() {
+        final Thread drivingThread = Thread.currentThread();
+
         System.err.println("Dumping stacktraces of all threads:");
         for (Entry<Thread, StackTraceElement[]> entry : Thread.getAllStackTraces().entrySet()) {
             final Thread thread = entry.getKey();
-            if (thread != Thread.currentThread() && runningThreads.contains(thread)) {
+            if (runningThreads.contains(thread)) {
                 final StackTraceElement[] stackTrace = entry.getValue();
                 boolean blocked = true;
 
@@ -225,7 +227,8 @@ private void printStacktracesOfBlockedThreads() {
                     }
                 }
 
-                System.err.println((blocked ? "BLOCKED: " : "IN SAFEPOINT: ") + thread);
+                String kind = thread == drivingThread ? "DRIVER" : (blocked ? "BLOCKED" : "IN SAFEPOINT");
+                System.err.println(kind + ": " + thread);
                 for (StackTraceElement stackTraceElement : stackTrace) {
                     System.err.println(stackTraceElement);
                 }