[GR-22447] Support closing the Context on other threads than the main Ruby Thread

PullRequest: truffleruby/2687

Author: eregon

src/main/java/org/truffleruby/RubyContext.java

@@ -512,7 +512,6 @@ private void dispose() {
             consoleHolder.close();
         }
 
-        threadManager.cleanupMainThread();
         threadManager.dispose();
         threadManager.checkNoRunningThreads();
 

src/main/java/org/truffleruby/RubyLanguage.java

@@ -521,6 +521,8 @@ protected boolean isThreadAccessAllowed(Thread thread, boolean singleThreaded) {
 
     @Override
     public void initializeThread(RubyContext context, Thread thread) {
+        LOGGER.fine(() -> "initializeThread(#" + thread.getId() + " " + thread + ")");
+
         if (thread == context.getThreadManager().getOrInitializeRootJavaThread()) {
             // Already initialized when creating the context
             return;
@@ -546,14 +548,24 @@ public void initializeThread(RubyContext context, Thread thread) {
 
     @Override
     public void disposeThread(RubyContext context, Thread thread) {
+        LOGGER.fine(
+                () -> "disposeThread(#" + thread.getId() + " " + thread + " " +
+                        context.getThreadManager().getCurrentThreadOrNull() + ")");
+
         if (thread == context.getThreadManager().getRootJavaThread()) {
             if (context.getEnv().isPreInitialization()) {
                 // Cannot save the root Java Thread instance in the image
                 context.getThreadManager().resetMainThread();
                 context.getThreadManager().dispose();
+                return;
+            } else if (!context.isInitialized()) {
+                // Context patching failed, we cannot cleanup the main thread as it was not initialized
+                return;
+            } else {
+                // Cleanup the main thread, this is done between finalizeContext() and disposeContext()
+                context.getThreadManager().cleanupThreadState(context.getThreadManager().getRootThread(), thread);
+                return;
             }
-            // Let the context shutdown cleanup the main thread
-            return;
         }
 
         if (context.getThreadManager().isRubyManagedThread(thread)) {
@@ -563,12 +575,13 @@ public void disposeThread(RubyContext context, Thread thread) {
                     throw CompilerDirectives.shouldNotReachHere("Ruby threads should be disposed on their Java thread");
                 }
                 context.getThreadManager().cleanupThreadState(rubyThread, thread);
-            } else {
-                // Fiber
+            } else { // (non-root) Fiber
+                // Fibers are always cleaned up by their thread's cleanup with FiberManager#killOtherFibers()
             }
             return;
         }
 
+        // A foreign Thread, its Fibers are considered isRubyManagedThread()
         final RubyThread rubyThread = context.getThreadManager().getRubyThread(thread);
         context.getThreadManager().cleanup(rubyThread, thread);
     }

src/main/java/org/truffleruby/core/fiber/FiberManager.java

@@ -397,14 +397,12 @@ public String getFiberDebugInfo() {
         for (RubyFiber fiber : runningFibers) {
             builder.append("  fiber @");
             builder.append(ObjectIDNode.getUncached().execute(fiber));
-            builder.append(" #");
 
             final Thread thread = fiber.thread;
-
             if (thread == null) {
-                builder.append("(no Java thread)");
+                builder.append(" (no Java thread)");
             } else {
-                builder.append(thread.getId());
+                builder.append(" #").append(thread.getId()).append(' ').append(thread);
             }
 
             if (fiber == rootFiber) {

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

@@ -677,39 +677,21 @@ public void checkNoRunningThreads() {
         }
     }
 
-    private void checkCalledInMainThreadRootFiber() {
-        final RubyThread currentThread = getCurrentThread();
-        if (currentThread != rootThread) {
-            throw new UnsupportedOperationException(StringUtils.format(
-                    "ThreadManager.shutdown() must be called on the root Ruby Thread (%s) but was called on %s",
-                    rootThread,
-                    currentThread));
-        }
-
-        final FiberManager fiberManager = rootThread.fiberManager;
-
-        if (getRubyFiberFromCurrentJavaThread() != fiberManager.getRootFiber()) {
-            throw new UnsupportedOperationException(
-                    "ThreadManager.shutdown() must be called on the root Fiber of the main Thread");
-        }
-    }
-
     @TruffleBoundary
     public void killAndWaitOtherThreads() {
-        checkCalledInMainThreadRootFiber();
-
         // Disallow new Fibers to be created
         fiberPool.shutdown();
 
         // Kill all Ruby Threads and Fibers
 
         // The logic below avoids using the SafepointManager if there is
-        // only the root thread and the reference processing thread.
+        // only the current thread and the reference processing thread.
+        final RubyThread currentThread = getCurrentThread();
         boolean otherThreads = false;
         RubyThread referenceProcessingThread = null;
         for (RubyThread thread : runningRubyThreads) {
-            if (thread == rootThread) {
-                // clean up later in #cleanupMainThread
+            if (thread == currentThread) {
+                // no need to kill the current thread
             } else if (thread == context.getReferenceProcessor().getProcessingThread()) {
                 referenceProcessingThread = thread;
             } else {
@@ -727,7 +709,7 @@ public void killAndWaitOtherThreads() {
         if (otherThreads) {
             doKillOtherThreads();
         }
-        rootThread.fiberManager.killOtherFibers();
+        currentThread.fiberManager.killOtherFibers();
 
         // Wait and join all Java threads we created
         for (Thread thread : rubyManagedThreads) {
@@ -755,12 +737,6 @@ public void run(RubyThread rubyThread, Node currentNode) {
                 });
     }
 
-    @TruffleBoundary
-    public void cleanupMainThread() {
-        checkCalledInMainThreadRootFiber();
-        cleanup(rootThread, rootJavaThread);
-    }
-
     @TruffleBoundary
     public Object[] getThreadList() {
         // This must not pre-allocate an array, or it could contain null's.
@@ -784,7 +760,7 @@ public String getThreadDebugInfo() {
             }
 
             // cannot use getCurrentThread() as it might have been cleared
-            if (thread == currentThread.get()) {
+            if (thread == getCurrentThreadOrNull()) {
                 builder.append(" (current)");
             }
 

src/test/java/org/truffleruby/MiscTest.java

@@ -18,7 +18,6 @@
 import org.graalvm.polyglot.PolyglotException;
 import org.graalvm.polyglot.Value;
 import org.junit.Assert;
-import org.junit.Ignore;
 import org.junit.Test;
 
 public class MiscTest {
@@ -112,9 +111,9 @@ public void testIntegratorThreadRubyThreadInitialization() throws Throwable {
             TestingThread thread = new TestingThread(() -> {
                 // Run code that requires the Ruby Thread object to be initialized
                 Value recursiveArray = context.eval("ruby", "a = [0]; a[0] = a; a.inspect"); // Access @recursive_objects
-                Assert.assertEquals("[[...]]", recursiveArray.asString());
-                Assert.assertTrue(context.eval("ruby", "Thread.current.thread_variable_get('foo')").isNull());
-                Assert.assertTrue(context.eval("ruby", "rand").fitsInDouble());
+                assertEquals("[[...]]", recursiveArray.asString());
+                assertTrue(context.eval("ruby", "Thread.current.thread_variable_get('foo')").isNull());
+                assertTrue(context.eval("ruby", "rand").fitsInDouble());
             });
             thread.start();
             thread.join();
@@ -155,12 +154,12 @@ public void testForeignThread() throws Throwable {
         }
     }
 
-    @Ignore // TODO (eregon, 8 April 2020): not yet working
     @Test
     public void testIntegratorThreadContextClosedOnOtherThread() throws Throwable {
         try (Context context = Context.newBuilder("ruby").allowCreateThread(true).build()) {
             TestingThread thread = new TestingThread(() -> {
-                Assert.assertEquals(42, context.eval("ruby", "6 * 7").asInt());
+                assertEquals(context.eval("ruby", "Thread.current"), context.eval("ruby", "Thread.main"));
+                assertEquals(42, context.eval("ruby", "6 * 7").asInt());
             });
             thread.start();
             thread.join();