From 6a4473352207d208900e04bfe960b4a3c4265af2 Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Fri, 1 Nov 2024 12:15:32 +0100 Subject: [PATCH 1/2] [GR-59531] Call fiber.finishedLatch.countDown() in afterLeave() * Otherwise multi-threading can be triggerred due to concurrently running: * disposeThread() for the main Thread, which kills Fibers of the main Ruby Thread * disposeThread() on one of these Fiber thread as a result of being killed * The former runs in leaveAndEnter() but the early countDown() meant it can re-enter before the Fiber has left the context. --- src/main/java/org/truffleruby/core/fiber/FiberManager.java | 3 +-- src/main/java/org/truffleruby/core/thread/ThreadManager.java | 3 +++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/truffleruby/core/fiber/FiberManager.java b/src/main/java/org/truffleruby/core/fiber/FiberManager.java index d21e409eda6a..1b30e785880d 100644 --- a/src/main/java/org/truffleruby/core/fiber/FiberManager.java +++ b/src/main/java/org/truffleruby/core/fiber/FiberManager.java @@ -176,6 +176,7 @@ private void afterLeave(RubyFiber fiber) { fiber.returnFiber = null; fiber.lastMessage = null; } + fiber.finishedLatch.countDown(); } public RubyFiber getReturnFiber(RubyFiber currentFiber, Node currentNode, InlinedBranchProfile errorProfile) { @@ -353,8 +354,6 @@ public void cleanup(RubyFiber fiber, Thread javaThread) { fiber.rubyThread.runningFibers.remove(fiber); fiber.thread = null; - - fiber.finishedLatch.countDown(); } @TruffleBoundary diff --git a/src/main/java/org/truffleruby/core/thread/ThreadManager.java b/src/main/java/org/truffleruby/core/thread/ThreadManager.java index 8b86aef99e58..971ea3209c8f 100644 --- a/src/main/java/org/truffleruby/core/thread/ThreadManager.java +++ b/src/main/java/org/truffleruby/core/thread/ThreadManager.java @@ -438,6 +438,9 @@ public void cleanupThreadState(RubyThread thread, Thread javaThread) { } thread.finishedLatch.countDown(); + + // Not strictly needed as this is the root Fiber and nothing currently uses its finishedLatch but clearer this way than leaving it with count 1 forever. + thread.getRootFiber().finishedLatch.countDown(); } public Thread getRootJavaThread() { From 732fa696dff54bf06f40440321157930aebc6f02 Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Fri, 1 Nov 2024 13:48:12 +0100 Subject: [PATCH 2/2] Use Thread#join() to wait for Fibers instead of a CountDownLatch * Now that we don't use a thread pool for Fibers anymore this should be fine. --- .../truffleruby/core/fiber/FiberManager.java | 17 +++++++++++------ .../org/truffleruby/core/fiber/RubyFiber.java | 1 - .../truffleruby/core/thread/ThreadManager.java | 7 +------ 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/truffleruby/core/fiber/FiberManager.java b/src/main/java/org/truffleruby/core/fiber/FiberManager.java index 1b30e785880d..a7ce15439750 100644 --- a/src/main/java/org/truffleruby/core/fiber/FiberManager.java +++ b/src/main/java/org/truffleruby/core/fiber/FiberManager.java @@ -176,7 +176,7 @@ private void afterLeave(RubyFiber fiber) { fiber.returnFiber = null; fiber.lastMessage = null; } - fiber.finishedLatch.countDown(); + fiber.thread = null; } public RubyFiber getReturnFiber(RubyFiber currentFiber, Node currentNode, InlinedBranchProfile errorProfile) { @@ -352,8 +352,6 @@ public void cleanup(RubyFiber fiber, Thread javaThread) { fiber.status = FiberStatus.TERMINATED; fiber.rubyThread.runningFibers.remove(fiber); - - fiber.thread = null; } @TruffleBoundary @@ -387,9 +385,16 @@ private void doKillOtherFibers(RubyThread thread) throws InterruptedException { if (!fiber.isRootFiber()) { addToMessageQueue(fiber, new FiberShutdownMessage()); - // Wait for the Fiber to finish so we only run one Fiber at a time - final CountDownLatch finishedLatch = fiber.finishedLatch; - finishedLatch.await(); + /* Wait for the Fiber to finish, so we only run one Fiber at a time. If fiber.thread is null it means + * the Fiber never started and shouldn't ever start since we are killing all fibers of this Ruby thread + * and so no more Ruby code (which could resume that Fiber) should run there. Adding a + * FiberShutdownMessage above won't cause the Fiber to start, because the only thing causing the Fiber + * to create a thread are callers of resumeAndWait(), which are Fiber#resume, Fiber#transfer, + * Fiber#raise and Fiber.yield. */ + Thread fiberThread = fiber.thread; + if (fiberThread != null) { + fiberThread.join(); + } final Throwable uncaughtException = fiber.uncaughtException; if (uncaughtException != null) { diff --git a/src/main/java/org/truffleruby/core/fiber/RubyFiber.java b/src/main/java/org/truffleruby/core/fiber/RubyFiber.java index ea80274e3cf9..9caeec1615d5 100644 --- a/src/main/java/org/truffleruby/core/fiber/RubyFiber.java +++ b/src/main/java/org/truffleruby/core/fiber/RubyFiber.java @@ -80,7 +80,6 @@ public enum FiberStatus { public final RubyBasicObject fiberLocals; public final RubyArray catchTags; public final CountDownLatch initializedLatch = new CountDownLatch(1); - public CountDownLatch finishedLatch = new CountDownLatch(1); final BlockingQueue messageQueue = newMessageQueue(); public final RubyThread rubyThread; // @formatter:off diff --git a/src/main/java/org/truffleruby/core/thread/ThreadManager.java b/src/main/java/org/truffleruby/core/thread/ThreadManager.java index 971ea3209c8f..75e17657e99d 100644 --- a/src/main/java/org/truffleruby/core/thread/ThreadManager.java +++ b/src/main/java/org/truffleruby/core/thread/ThreadManager.java @@ -134,7 +134,6 @@ public void restartMainThread(Thread mainJavaThread) { final RubyFiber rootFiber = rootThread.getRootFiber(); rootFiber.restart(); - rootFiber.finishedLatch = new CountDownLatch(1); PRNGRandomizerNodes.resetSeed(context, rootThread.randomizer); } @@ -200,8 +199,6 @@ private static Thread.UncaughtExceptionHandler uncaughtExceptionHandler(RubyFibe // the Fiber is not yet initialized, unblock the caller and rethrow the exception to it fiber.initializedLatch.countDown(); - // the Fiber thread is dying, unblock the caller - fiber.finishedLatch.countDown(); } catch (Throwable t) { // exception inside this UncaughtExceptionHandler t.initCause(throwable); printInternalError(t); @@ -423,6 +420,7 @@ public void cleanupThreadState(RubyThread thread, Thread javaThread) { unregisterThread(thread); thread.thread = null; + thread.getRootFiber().thread = null; if (Thread.currentThread() == javaThread) { for (ReentrantLock lock : thread.ownedLocks) { @@ -438,9 +436,6 @@ public void cleanupThreadState(RubyThread thread, Thread javaThread) { } thread.finishedLatch.countDown(); - - // Not strictly needed as this is the root Fiber and nothing currently uses its finishedLatch but clearer this way than leaving it with count 1 forever. - thread.getRootFiber().finishedLatch.countDown(); } public Thread getRootJavaThread() {