[GR-18163] Use a ReentrantLock for TruffleRuby.synchronized

PullRequest: truffleruby/2479

Author: eregon

CHANGELOG.md

@@ -19,6 +19,7 @@ Bug fixes:
 * Fix `Enumerator::Lazy#{chunk_while, slice_before, slice_after, slice_when}` to return instances of `Enumerator::Lazy` (#2273).
 * Fix `Truffle::Interop.source_location` to return unavailable source sections for modules instead of null (#2257).
 * Fix usage of `Thread.handle_interrupt` in `MonitorMixin#mon_synchronize`.
+* Fixed `TruffleRuby.synchronized` to handle guest safepoints (#2277).
 
 Compatibility:
 
@@ -78,6 +79,7 @@ Changes:
 
 * Standalone builds of TruffleRuby are now based on JDK11 (they used JDK8 previously). There should be no user-visible changes. Similarly, JDK11 is now used by default in development instead of JDK8.
 * The deprecated `Truffle::System.synchronized` has been removed.
+* `Java.synchronized` has been removed, it did not work on host objects.
 
 # 21.0.0
 

doc/user/polyglot.md

@@ -128,8 +128,6 @@ To access instance methods use `.class`, such as `MyClass.class.getName`.
 
 To import a Java class as a top-level constant, use `Java.import 'name'`.
 
-`Java.synchronized(object) { }` will use the Java object's monitor to synchronize.
-
 ## Embedding in Java
 
 TruffleRuby is embedded via the Polyglot API, which is part of GraalVM.

lib/truffle/timeout.rb

@@ -131,7 +131,7 @@ def self.ensure_timeout_thread_running
   def self.add_timeout(time, exc, message)
     r = TimeoutRequest.new(time, Thread.current, exc, message)
     @chan << r
-    ensure_timeout_thread_running
+    ensure_timeout_thread_running unless defined?(@controller)
     r
   end
 

spec/truffle/interop/java_synchronized_spec.rb

@@ -19,6 +19,7 @@ public abstract class Layouts {
 
     public static final HiddenKey OBJECT_ID_IDENTIFIER = new HiddenKey("object_id"); // long
     public static final HiddenKey FROZEN_IDENTIFIER = new HiddenKey("frozen?"); // boolean
+    public static final HiddenKey OBJECT_LOCK = new HiddenKey("object_lock"); // ReentrantLock
     public static final HiddenKey ASSOCIATED_IDENTIFIER = new HiddenKey("associated"); // Pointer[]
     public static final HiddenKey FINALIZER_REF_IDENTIFIER = new HiddenKey("finalizerRef"); // FinalizerReference
     public static final HiddenKey MARKED_OBJECTS_IDENTIFIER = new HiddenKey("marked_objects"); // Object[]

src/main/java/org/truffleruby/Layouts.java

@@ -10,14 +10,17 @@
 package org.truffleruby.extra;
 
 import com.oracle.truffle.api.dsl.Cached;
+import com.oracle.truffle.api.object.DynamicObjectLibrary;
 import org.jcodings.specific.UTF8Encoding;
+import org.truffleruby.Layouts;
 import org.truffleruby.RubyContext;
 import org.truffleruby.RubyLanguage;
 import org.truffleruby.builtins.CoreMethod;
 import org.truffleruby.builtins.CoreMethodArrayArgumentsNode;
 import org.truffleruby.builtins.CoreMethodNode;
 import org.truffleruby.builtins.CoreModule;
 import org.truffleruby.builtins.YieldingCoreMethodNode;
+import org.truffleruby.core.mutex.MutexOperations;
 import org.truffleruby.core.proc.RubyProc;
 import org.truffleruby.core.rope.CodeRange;
 import org.truffleruby.core.string.StringNodes;
@@ -29,6 +32,8 @@
 import com.oracle.truffle.api.dsl.Specialization;
 import org.truffleruby.language.RubyDynamicObject;
 
+import java.util.concurrent.locks.ReentrantLock;
+
 @CoreModule("TruffleRuby")
 public abstract class TruffleRubyNodes {
 
@@ -104,11 +109,39 @@ protected Object fullMemoryBarrier() {
     @CoreMethod(names = "synchronized", onSingleton = true, required = 1, needsBlock = true)
     public abstract static class SynchronizedNode extends YieldingCoreMethodNode {
 
-        // We must not allow to synchronize on boxed primitives.
+        /** We must not allow to synchronize on boxed primitives as that would be misleading. We use a ReentrantLock and
+         * not simply Java's {@code synchronized} here as we need to be able to interrupt for guest safepoints and it is
+         * not possible to interrupt Java's {@code synchronized (object) {}}. */
         @Specialization
         protected Object synchronize(RubyDynamicObject object, RubyProc block) {
-            synchronized (object) {
+            final ReentrantLock lock = getLockAndLock(object);
+            try {
                 return yield(block);
+            } finally {
+                MutexOperations.unlockInternal(lock);
+            }
+        }
+
+        @TruffleBoundary
+        private ReentrantLock getLockAndLock(RubyDynamicObject object) {
+            final ReentrantLock lock = getLock(object);
+            MutexOperations.lockInternal(getContext(), lock, this);
+            return lock;
+        }
+
+        @TruffleBoundary
+        private ReentrantLock getLock(RubyDynamicObject object) {
+            final DynamicObjectLibrary objectLibrary = DynamicObjectLibrary.getUncached();
+
+            synchronized (object) {
+                ReentrantLock lock = (ReentrantLock) objectLibrary.getOrDefault(object, Layouts.OBJECT_LOCK, null);
+                if (lock != null) {
+                    return lock;
+                } else {
+                    lock = new ReentrantLock();
+                    objectLibrary.put(object, Layouts.OBJECT_LOCK, lock);
+                    return lock;
+                }
             }
         }
 

src/main/java/org/truffleruby/extra/TruffleRubyNodes.java

@@ -58,12 +58,6 @@ def self.import(name)
     type
   end
 
-  def self.synchronized(object)
-    TruffleRuby.synchronized(object) do
-      yield
-    end
-  end
-
   # test-unit expects `Java::JavaLang::Throwable` to be resolvable if `::Java` is defined (see assertions.rb).
   # When doing JRuby-style interop, that's a fine assumption. However, we have `::Java` defined for Truffle-style
   # interop and in that case, the assumption does not hold. In order to allow the gem to work properly, we define