Use a separate mutex for feeble references.

substratevm/src/com.oracle.svm.core.genscavenge/src/com/oracle/svm/core/genscavenge/DiscoverableReferenceProcessing.java

@@ -281,8 +281,7 @@ static void distributeReferences() {
             if (SubstrateOptions.MultiThreaded.getValue()) {
                 trace.string("  broadcasting").newline();
                 /* Notify anyone blocked waiting for FeebleReferences to be available. */
-                FeebleReferenceList.guaranteeIsLocked();
-                FeebleReferenceList.broadcast();
+                FeebleReferenceList.signalWaiters();
             }
             trace.string("]").newline();
         }

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/heap/FeebleReferenceList.java

@@ -36,7 +36,7 @@
 import com.oracle.svm.core.thread.JavaThreads;
 import com.oracle.svm.core.thread.ThreadStatus;
 import com.oracle.svm.core.thread.VMOperation;
-import com.oracle.svm.core.thread.VMThreads;
+import com.oracle.svm.core.thread.VMOperationControl;
 import com.oracle.svm.core.util.TimeUtils;
 
 /** A feeble version of java.lang.ref.ReferenceQueue. */
@@ -47,12 +47,12 @@ public final class FeebleReferenceList<T> {
     private final UninterruptibleUtils.AtomicReference<FeebleReference<? extends T>> head;
 
     /**
-     * Notification of other threads that FeebleReferences might be available.
-     *
-     * I am re-using the VMThreads mutex, but using my own condition variable.
+     * This mutex is used by the GC and the application. The application must use this mutex only in
+     * uninterruptible code to prevent the case that a safepoint is initiated while waiting on the
+     * lock. Otherwise, we risk deadlocks between the application and the GC.
      */
-    private static final VMMutex availableLock = VMThreads.THREAD_MUTEX;
-    private static final VMCondition availableCondition = new VMCondition(availableLock);
+    private static final VMMutex REF_MUTEX = new VMMutex();
+    private static final VMCondition REF_CONDITION = new VMCondition(REF_MUTEX);
 
     /** A pre-allocated InterruptedException. */
     private static final InterruptedException preallocatedInterruptedException = new InterruptedException();
@@ -171,10 +171,30 @@ private FeebleReference<? extends T> popWithBlocking(long timeoutNanos) throws I
                 return result;
             }
 
-            /* If I have been interrupted, throw an exception. */
+            /**
+             * TODO: We have a potential race condition here:
+             * <ul>
+             * <li>Thread A executes pop(), the queue is empty, null is returned.</li>
+             * <li>Thread A gets blocked by a safepoint between pop() and the VMCondition.block()
+             * that is called somewhere in await().</li>
+             * <li>The GC pushes elements to the queue and sends a notification that new elements
+             * were queued</li>
+             * <li>Thread A executes VMCondition.block() and blocks even though new elements were
+             * queued in the meanwhile.</li>
+             * </ul>
+             */
             if (Thread.interrupted()) {
                 throw preallocatedInterruptedException;
             }
+            /**
+             * TODO: We have a potential race condition here:
+             * <ul>
+             * <li>Thread A executes Thread.interrupted() - this returns false.</li>
+             * <li>Thread B interrupts thread A. This sets the interrupted flag and wakes up all
+             * threads that are currently waiting for feeble references.</li>
+             * <li>Thread A executes await() and gets blocked.</li>
+             * </ul>
+             */
 
             /* Either wait forever or for however long is requested. */
             if (timeoutNanos == 0) {
@@ -207,20 +227,6 @@ private boolean compareAndSetHead(FeebleReference<? extends T> expect, FeebleRef
         return head.compareAndSet(expect, update);
     }
 
-    /*
-     * Methods on the lock and condition variable, for {@link #remove(long)}.
-     */
-
-    @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
-    private static void lock() {
-        availableLock.lockNoTransition();
-    }
-
-    @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
-    private static void unlock() {
-        availableLock.unlock();
-    }
-
     /** Await with thread status change. */
     private static void await() {
         final Thread thread = Thread.currentThread();
@@ -300,61 +306,58 @@ private static long awaitWithTransition(long waitNanos) {
     @Uninterruptible(reason = "Must not stop while in native.")
     @NeverInline("Provide a return address for the Java frame anchor.")
     private static void awaitInNative() {
-        /* Lock the mutex. */
-        lock();
+        REF_MUTEX.lockNoTransition();
         try {
             /*
              * Wait until the condition is notified, unlocking the mutex. Do not transition back to
              * Java on return, because I do not want to stop in this method.
              */
-            availableCondition.blockNoTransition();
+            REF_CONDITION.blockNoTransition();
         } finally {
-            unlock();
+            REF_MUTEX.unlock();
         }
     }
 
     @Uninterruptible(reason = "Must not stop while in native.")
     @NeverInline("Provide a return address for the Java frame anchor.")
     private static long awaitInNative(long waitNanos) {
         long result = waitNanos;
-        /* Lock the mutex. */
-        lock();
+        REF_MUTEX.lockNoTransition();
         try {
             /*
              * Wait until the condition is notified, unlocking the mutex. Do not transition back to
              * Java on return, because I do not want to stop in this method.
              */
-            result = availableCondition.blockNoTransition(waitNanos);
+            result = REF_CONDITION.blockNoTransition(waitNanos);
         } finally {
-            unlock();
+            REF_MUTEX.unlock();
         }
         return result;
     }
 
-    public static void broadcast() {
-        guaranteeIsLocked();
-        availableCondition.broadcast();
+    @Uninterruptible(reason = "No GC is allowed while holding the lock - otherwise the application and the GC would deadlock.")
+    public static void interruptWaiters() {
+        REF_MUTEX.lockNoTransition();
+        try {
+            REF_CONDITION.broadcast();
+        } finally {
+            REF_MUTEX.unlock();
+        }
     }
 
-    /**
-     * Called by {@code Target_java_lang_Thread.interrupt0()} to notify waiters to check to see if
-     * they have been interrupted.
-     */
     public static void signalWaiters() {
-        broadcast();
+        assert VMOperationControl.isAtSafepoint() : "must only be called by the GC";
+        REF_MUTEX.lock();
+        try {
+            REF_CONDITION.broadcast();
+        } finally {
+            REF_MUTEX.unlock();
+        }
     }
 
-    /*
-     * Other methods.
-     */
-
     /** Clean the list state that is kept in a FeebleReference. */
     @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
     protected static void clean(FeebleReference<?> fr) {
         fr.listRemove();
     }
-
-    public static void guaranteeIsLocked() {
-        availableLock.guaranteeIsLocked("Should hold VMThreads lock when notifying waiters.");
-    }
 }

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/Target_java_lang_Thread.java

@@ -41,6 +41,7 @@
 import com.oracle.svm.core.annotate.Substitute;
 import com.oracle.svm.core.annotate.TargetClass;
 import com.oracle.svm.core.annotate.TargetElement;
+import com.oracle.svm.core.heap.FeebleReferenceList;
 import com.oracle.svm.core.jdk.JDK11OrLater;
 import com.oracle.svm.core.jdk.JDK8OrEarlier;
 import com.oracle.svm.core.jdk.JavaLangSubstitutions;