[GR-7405] [GR-7408] Signal handlers can now be run without triggering multi-threading

PullRequest: truffleruby/2439

Author: eregon

CHANGELOG.md

@@ -4,6 +4,7 @@ New features:
 
 * Access to local variables of the interactive Binding via language bindings is now supported: `context.getBindings("ruby").putMember("my_var", 42);` (#2030).
 * `VALUE`s in C extensions now expose the Ruby object when viewed in the debugger, as long as they have not been converted to native values.
+* Signal handlers can now be run without triggering multi-threading.
 
 Bug fixes:
 

mx.truffleruby/mx_truffleruby.py

@@ -105,6 +105,7 @@ def contents(self, result):
         ruby_options = [
             '--experimental-options',
             '--building-core-cexts',
+            '--platform-native-interrupt=false', # no librubysignal in the ruby home yet
             '--launcher=' + result,
             '--disable-gems',
             '--disable-rubyopt',
@@ -267,6 +268,6 @@ def verify_ci(args):
     'ruby_testdownstream_hello': [ruby_testdownstream_hello, ''],
     'ruby_testdownstream_sulong': [ruby_testdownstream_sulong, ''],
     'ruby_spotbugs': [ruby_spotbugs, ''],
-    'verify-ci' : [verify_ci, '[options]'],
+    'verify-ci': [verify_ci, '[options]'],
     'ruby_jacoco_args': [ruby_jacoco_args, ''],
 })

mx.truffleruby/suite.py

@@ -2,15 +2,15 @@
     "mxversion": "5.281.0",
     "name": "truffleruby",
 
-    "imports" : {
-        "suites" : [
+    "imports": {
+        "suites": [
             {
                 "name": "regex",
                 "subdir": True,
                 "version": "8c1abd10ad2992580462d7629db316177a256f82",
                 "urls": [
-                    {"url": "https://github.com/oracle/graal.git", "kind" : "git"},
-                    {"url": "https://curio.ssw.jku.at/nexus/content/repositories/snapshots", "kind" : "binary"},
+                    {"url": "https://github.com/oracle/graal.git", "kind": "git"},
+                    {"url": "https://curio.ssw.jku.at/nexus/content/repositories/snapshots", "kind": "binary"},
                 ]
             },
             {
@@ -172,9 +172,20 @@
             "license": ["EPL-2.0"],
         },
 
+        "org.truffleruby.rubysignal": {
+            "dir": "src/main/c/rubysignal",
+            "native": "shared_lib",
+            "deliverable": "rubysignal",
+            "buildDependencies": [
+                "org.truffleruby", # for the generated JNI header file
+            ],
+            "cflags": ["-g", "-Wall", "-Werror"],
+        },
+
         "org.truffleruby": {
             "dir": "src/main",
             "sourceDirs": ["java"],
+            "jniHeaders": True,
             "dependencies": [
                 "truffleruby:TRUFFLERUBY-ANNOTATIONS",
                 "truffleruby:TRUFFLERUBY-SHARED",
@@ -446,6 +457,7 @@
                     "file:lib/cext/*.rb",
                     "dependency:org.truffleruby.cext/src/main/c/truffleposix/<lib:truffleposix>",
                     "dependency:org.truffleruby.cext/src/main/c/cext/<lib:truffleruby>",
+                    "dependency:org.truffleruby.rubysignal",
                 ],
                 "lib/cext/include/": [
                     "file:lib/cext/include/ccan",
@@ -513,14 +525,21 @@
             "distDependencies": [
                 "TRUFFLERUBY",
                 "TRUFFLERUBY-SERVICES",
+                "TRUFFLERUBY_GRAALVM_SUPPORT",
             ],
+            "javaProperties": {
+                "org.graalvm.language.ruby.home": "<path:TRUFFLERUBY_GRAALVM_SUPPORT>"
+            },
             "license": ["EPL-2.0"],
         },
 
         "TRUFFLERUBY-TCK": {
             "testDistribution": True,
             "dependencies": ["org.truffleruby.tck"],
             "distDependencies": ["truffle:TRUFFLE_TCK"],
+            "javaProperties": {
+                "org.graalvm.language.ruby.home": "<path:TRUFFLERUBY_GRAALVM_SUPPORT>"
+            },
             "license": ["EPL-2.0"],
         },
     },

spec/ruby/core/signal/trap_spec.rb

@@ -52,6 +52,21 @@
       Process.kill(:HUP, Process.pid).should == 1
     end
 
+    it "can register a new handler after :IGNORE" do
+      Signal.trap :HUP, :IGNORE
+      Process.kill(:HUP, Process.pid).should == 1
+
+      done = false
+      Signal.trap(:HUP) do
+        ScratchPad.record :block_trap
+        done = true
+      end
+
+      Process.kill(:HUP, Process.pid).should == 1
+      Thread.pass until done
+      ScratchPad.recorded.should == :block_trap
+    end
+
     it "ignores the signal when passed nil" do
       Signal.trap :HUP, nil
       Signal.trap(:HUP, @saved_trap).should be_nil

spec/ruby/optional/capi/thread_spec.rb

@@ -102,13 +102,13 @@ def call_capi_rb_thread_wakeup
   end
 
   describe "rb_thread_call_without_gvl" do
-    it "runs a C function with the global lock unlocked" do
+    it "runs a C function with the global lock unlocked and can be woken by Thread#wakeup" do
       thr = Thread.new do
         @t.rb_thread_call_without_gvl
       end
 
       # Wait until it's blocking...
-      Thread.pass while thr.status and thr.status != "sleep"
+      Thread.pass until thr.stop?
 
       # The thread status is set to sleep by rb_thread_call_without_gvl(),
       # but the thread might not be in the blocking read(2) yet, so wait a bit.
@@ -121,6 +121,40 @@ def call_capi_rb_thread_wakeup
       thr.value.should be_true
     end
 
+    platform_is_not :windows do
+      it "runs a C function with the global lock unlocked and can be woken by a signal" do
+        # Ruby signal handlers run on the main thread, so we need to reverse roles here and have a thread interrupt us
+        thr = Thread.current
+        thr.should == Thread.main
+
+        going_to_block = false
+        interrupter = Thread.new do
+          # Wait until it's blocking...
+          Thread.pass until going_to_block and thr.stop?
+
+          # The thread status is set to sleep by rb_thread_call_without_gvl(),
+          # but the thread might not be in the blocking read(2) yet, so wait a bit.
+          sleep 0.1
+
+          # Wake it up by sending a signal
+          done = false
+          prev_handler = Signal.trap(:HUP) { done = true }
+          begin
+            Process.kill :HUP, Process.pid
+            sleep 0.001 until done
+          ensure
+            Signal.trap(:HUP, prev_handler)
+          end
+        end
+
+        going_to_block = true
+        # Make sure it stopped and we got a proper value
+        @t.rb_thread_call_without_gvl.should be_true
+
+        interrupter.join
+      end
+    end
+
     guard -> { platform_is :mingw and ruby_version_is ""..."2.7" } do
       it "runs a C function with the global lock unlocked and unlocks IO with the generic RUBY_UBF_IO" do
         thr = Thread.new do

src/main/c/rubysignal/src/rubysignal.c

@@ -0,0 +1,24 @@
+#include "org_truffleruby_signal_LibRubySignal.h"
+#include <pthread.h>
+#include <signal.h>
+
+static void empty_handler(int sig) {
+}
+
+JNIEXPORT jint JNICALL Java_org_truffleruby_signal_LibRubySignal_setupSIGVTALRMEmptySignalHandler(JNIEnv *env, jclass clazz) {
+  struct sigaction action = {
+    .sa_flags = 0, /* flags = 0 is intended as we want no SA_RESTART so we can interrupt blocking syscalls */
+    .sa_handler = empty_handler,
+  };
+  return sigaction(SIGVTALRM, &action, NULL);
+}
+
+JNIEXPORT jlong JNICALL Java_org_truffleruby_signal_LibRubySignal_threadID(JNIEnv *env, jclass clazz) {
+  pthread_t pthread_id = pthread_self();
+  return (jlong) pthread_id;
+}
+
+JNIEXPORT jint JNICALL Java_org_truffleruby_signal_LibRubySignal_sendSIGVTALRMToThread(JNIEnv *env, jclass clazz, jlong threadID) {
+  pthread_t pthread_id = (pthread_t) threadID;
+  return pthread_kill(pthread_id, SIGVTALRM);
+}

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

@@ -214,7 +214,7 @@ public RubyContext(RubyLanguage language, TruffleLanguage.Env env) {
 
         Metrics.printTime("before-thread-manager");
         threadManager = new ThreadManager(this, language);
-        threadManager.initialize(truffleNFIPlatform, nativeConfiguration);
+        threadManager.initialize();
         threadManager.initializeMainThread(Thread.currentThread());
         Metrics.printTime("after-thread-manager");
 
@@ -300,7 +300,7 @@ protected boolean patch(Env newEnv) {
         this.truffleNFIPlatform = createNativePlatform();
         encodingManager.initializeDefaultEncodings(truffleNFIPlatform, nativeConfiguration);
 
-        threadManager.initialize(truffleNFIPlatform, nativeConfiguration);
+        threadManager.initialize();
         threadManager.restartMainThread(Thread.currentThread());
 
         Metrics.printTime("before-rehash");

src/main/java/org/truffleruby/core/VMPrimitiveNodes.java

@@ -80,14 +80,12 @@
 
 import com.oracle.truffle.api.CompilerDirectives;
 import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
-import com.oracle.truffle.api.TruffleContext;
 import com.oracle.truffle.api.dsl.Cached;
 import com.oracle.truffle.api.dsl.Specialization;
 import com.oracle.truffle.api.frame.VirtualFrame;
 import com.oracle.truffle.api.profiles.BranchProfile;
 import com.oracle.truffle.api.profiles.ConditionProfile;
 
-import sun.misc.Signal;
 import sun.misc.SignalHandler;
 
 @CoreModule(value = "VMPrimitives", isClass = true)
@@ -260,48 +258,16 @@ protected boolean watchSignalProc(Object signalString, RubyProc action,
             }
 
             final String signalName = libSignalString.getJavaString(signalString);
-            return registerHandler(signalName, signal -> {
-                if (context.getOptions().SINGLE_THREADED) {
-                    warnRestoreAndRaise(context, signalName, signal, "create a thread");
-                    return;
-                }
 
+            return registerHandler(signalName, signal -> {
                 final RubyThread rootThread = context.getThreadManager().getRootThread();
-
-                // Workaround: we need to register with Truffle (which means going multithreaded),
-                // so that NFI can get its context to call pthread_kill() (GR-7405).
-                final TruffleContext truffleContext = context.getEnv().getContext();
-                final Object prev;
-                try {
-                    prev = truffleContext.enter(this);
-                } catch (IllegalStateException e) { // Multi threaded access denied from Truffle
-                    warnRestoreAndRaise(context, signalName, signal, "attach a thread");
-                    return;
-                }
-                try {
-                    context.getSafepointManager().pauseAllThreadsAndExecuteFromNonRubyThread(
-                            "Handling of signal " + signal,
-                            SafepointPredicate.currentFiberOfThread(context, rootThread),
-                            (rubyThread, currentNode) -> ProcOperations.rootCall(action));
-                } finally {
-                    truffleContext.leave(this, prev);
-                }
+                context.getSafepointManager().pauseAllThreadsAndExecuteFromNonRubyThread(
+                        "Handling of signal " + signal,
+                        SafepointPredicate.currentFiberOfThread(context, rootThread),
+                        (rubyThread, currentNode) -> ProcOperations.rootCall(action));
             });
         }
 
-        private static void warnRestoreAndRaise(RubyContext context, String signalName, Signal signal, String failure) {
-            // Not in a context, so we cannot use TruffleLogger
-            context.getEnvErrStream().println(
-                    "[ruby] SEVERE: signal " + signal + " caught but can't " + failure +
-                            " to handle it so restoring the default handler and re-raising the signal");
-            Signals.restoreDefaultHandler(signalName);
-            try {
-                Signal.raise(signal);
-            } catch (IllegalArgumentException illegalArgumentException) {
-                illegalArgumentException.printStackTrace(context.getEnvErrStream());
-            }
-        }
-
         @TruffleBoundary
         private boolean restoreDefaultHandler(String signalName) {
             if (getContext().getOptions().EMBEDDED) {

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

@@ -394,7 +394,8 @@ protected Object appendFeatures(RubyModule features, RubyModule target,
     public abstract static class GeneratedReaderNode extends AlwaysInlinedMethodNode {
 
         @Specialization(limit = "getCacheLimit()")
-        protected Object reader(Frame frame, RubyDynamicObject self, Object[] args, Object block, RootCallTarget target,
+        protected Object reader(
+                Frame callerFrame, RubyDynamicObject self, Object[] args, Object block, RootCallTarget target,
                 @CachedLibrary("self") DynamicObjectLibrary objectLibrary) {
             // Or a subclass of RubyRootNode with an extra field?
             final String ivarName = RubyRootNode.of(target).getSharedMethodInfo().getNotes();
@@ -404,7 +405,7 @@ protected Object reader(Frame frame, RubyDynamicObject self, Object[] args, Obje
         }
 
         @Specialization(guards = "!isRubyDynamicObject(self)")
-        protected Object notObject(Frame frame, Object self, Object[] args, Object block, RootCallTarget target) {
+        protected Object notObject(Frame callerFrame, Object self, Object[] args, Object block, RootCallTarget target) {
             return nil;
         }
 
@@ -417,7 +418,8 @@ protected int getCacheLimit() {
     public abstract static class GeneratedWriterNode extends AlwaysInlinedMethodNode {
 
         @Specialization(guards = "!rubyLibrary.isFrozen(self)")
-        protected Object writer(Frame frame, RubyDynamicObject self, Object[] args, Object block, RootCallTarget target,
+        protected Object writer(
+                Frame callerFrame, RubyDynamicObject self, Object[] args, Object block, RootCallTarget target,
                 @CachedLibrary(limit = "getRubyLibraryCacheLimit()") RubyLibrary rubyLibrary,
                 @Cached WriteObjectFieldNode writeObjectFieldNode) {
             final String ivarName = RubyRootNode.of(target).getSharedMethodInfo().getNotes();
@@ -429,7 +431,7 @@ protected Object writer(Frame frame, RubyDynamicObject self, Object[] args, Obje
         }
 
         @Specialization(guards = "rubyLibrary.isFrozen(self)")
-        protected Object frozen(Frame frame, Object self, Object[] args, Object block, RootCallTarget target,
+        protected Object frozen(Frame callerFrame, Object self, Object[] args, Object block, RootCallTarget target,
                 @CachedLibrary(limit = "getRubyLibraryCacheLimit()") RubyLibrary rubyLibrary,
                 @CachedContext(RubyLanguage.class) RubyContext context) {
             throw new RaiseException(context, context.getCoreExceptions().frozenError(self, this));