[GR-19220] Fix use of vm_watch_signal as it now passes signo (#2293)

PullRequest: truffleruby/2490

Author: eregon

spec/ruby/core/exception/interrupt_spec.rb

@@ -29,7 +29,23 @@
       sleep
     rescue Interrupt => e
       e.signo.should == Signal.list["INT"]
-      e.signm.should == ""
+      ["", "Interrupt"].should.include?(e.message)
     end
   end
 end
+
+describe "Interrupt" do
+  # This spec is basically the same as above,
+  # but it does not rely on Signal.trap(:INT, :SIG_DFL) which can be tricky
+  it "is raised on the main Thread by the default SIGINT handler" do
+    out = ruby_exe(<<-'RUBY', args: "2>&1")
+    begin
+      Process.kill :INT, Process.pid
+      sleep
+    rescue Interrupt => e
+      puts "Interrupt: #{e.signo}"
+    end
+    RUBY
+    out.should == "Interrupt: #{Signal.list["INT"]}\n"
+  end
+end

spec/tags/core/exception/interrupt_tags.txt

@@ -1 +1 @@
-fails:rescuing Interrupt raises an Interrupt when sent a signal SIGINT
+slow:Interrupt is raised on the main Thread by the default SIGINT handler

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

@@ -228,7 +228,7 @@ public static abstract class VMWatchSignalNode extends PrimitiveArrayArgumentsNo
 
         @TruffleBoundary
         @Specialization(guards = { "libSignalString.isRubyString(signalString)", "libAction.isRubyString(action)" })
-        protected boolean restoreDefault(Object signalString, Object action,
+        protected boolean watchSignalString(Object signalString, boolean isRubyDefaultHandler, Object action,
                 @CachedLibrary(limit = "2") RubyStringLibrary libSignalString,
                 @CachedLibrary(limit = "2") RubyStringLibrary libAction) {
             final String actionString = libAction.getJavaString(action);
@@ -248,7 +248,7 @@ protected boolean restoreDefault(Object signalString, Object action,
 
         @TruffleBoundary
         @Specialization(guards = "libSignalString.isRubyString(signalString)")
-        protected boolean watchSignalProc(Object signalString, RubyProc action,
+        protected boolean watchSignalProc(Object signalString, boolean isRubyDefaultHandler, RubyProc action,
                 @CachedLibrary(limit = "2") RubyStringLibrary libSignalString) {
             final RubyContext context = getContext();
 
@@ -265,7 +265,7 @@ protected boolean watchSignalProc(Object signalString, RubyProc action,
                         "Handling of signal " + signal,
                         SafepointPredicate.currentFiberOfThread(context, rootThread),
                         (rubyThread, currentNode) -> ProcOperations.rootCall(action, signal.getNumber()));
-            });
+            }, isRubyDefaultHandler);
         }
 
         @TruffleBoundary
@@ -277,7 +277,7 @@ private boolean restoreDefaultHandler(String signalName) {
             }
 
             try {
-                return Signals.restoreDefaultHandler(signalName);
+                return Signals.restoreRubyDefaultHandler(signalName);
             } catch (IllegalArgumentException e) {
                 throw new RaiseException(getContext(), coreExceptions().argumentError(e.getMessage(), this));
             }
@@ -316,15 +316,15 @@ private boolean registerIgnoreHandler(String signalName) {
         }
 
         @TruffleBoundary
-        private boolean registerHandler(String signalName, SignalHandler newHandler) {
+        private boolean registerHandler(String signalName, SignalHandler newHandler, boolean isRubyDefaultHandler) {
             if (getContext().getOptions().EMBEDDED) {
                 RubyLanguage.LOGGER.warning(
                         "trapping signal " + signalName +
                                 " in embedded mode may interfere with other embedded contexts or the host system");
             }
 
             try {
-                Signals.registerHandler(newHandler, signalName);
+                Signals.registerHandler(newHandler, signalName, isRubyDefaultHandler);
             } catch (IllegalArgumentException e) {
                 throw new RaiseException(getContext(), coreExceptions().argumentError(e.getMessage(), this));
             }

src/main/java/org/truffleruby/platform/Signals.java

@@ -24,12 +24,18 @@ public class Signals {
     };
 
     // Use String and not Signal as key to work around SVM not allowing new Signal("PROF")
+    /** Default SignalHandlers for the JVM */
     private static final ConcurrentMap<String, SignalHandler> DEFAULT_HANDLERS = new ConcurrentHashMap<>();
+    /** Default signal handlers for Ruby, only SIGINT and SIGALRM, see {@code core/main.rb} */
+    private static final ConcurrentMap<String, SignalHandler> RUBY_DEFAULT_HANDLERS = new ConcurrentHashMap<>();
 
-    public static void registerHandler(SignalHandler newHandler, String signalName) {
+    public static void registerHandler(SignalHandler newHandler, String signalName, boolean isRubyDefaultHandler) {
         final Signal signal = new Signal(signalName);
         final SignalHandler oldHandler = Signal.handle(signal, newHandler);
         DEFAULT_HANDLERS.putIfAbsent(signalName, oldHandler);
+        if (isRubyDefaultHandler) {
+            RUBY_DEFAULT_HANDLERS.putIfAbsent(signalName, newHandler);
+        }
     }
 
     public static void registerIgnoreHandler(String signalName) {
@@ -50,6 +56,22 @@ public static boolean restoreDefaultHandler(String signalName) {
         }
     }
 
+    public static boolean restoreRubyDefaultHandler(String signalName) {
+        SignalHandler defaultHandler = RUBY_DEFAULT_HANDLERS.get(signalName);
+        if (defaultHandler == null) {
+            defaultHandler = DEFAULT_HANDLERS.get(signalName);
+        }
+
+        if (defaultHandler == null) {
+            // it is already the default signal
+            return false;
+        } else {
+            final Signal signal = new Signal(signalName);
+            Signal.handle(signal, defaultHandler);
+            return true;
+        }
+    }
+
     public static void restoreSystemHandler(String signalName) {
         final Signal signal = new Signal(signalName);
         Signal.handle(signal, SignalHandler.SIG_DFL);

src/main/ruby/truffleruby/core/main.rb

@@ -70,10 +70,10 @@ def to_s
 }
 
 Truffle::Boot.delay do
-  # Use vm_watch_signal directly as those should be the 'DEFAULT' handlers
+  # Use vm_watch_signal directly as those should be the default Ruby handlers
 
   if Truffle::Boot.get_option('platform-handle-interrupt')
-    Primitive.vm_watch_signal 'INT', -> do
+    Primitive.vm_watch_signal 'INT', true, -> _signo do
       if Truffle::Boot.get_option('backtraces-on-interrupt')
         puts 'Interrupting...'
         show_backtraces.call
@@ -84,7 +84,7 @@ def to_s
   end
 
   if Truffle::Boot.get_option('backtraces-sigalrm')
-    Primitive.vm_watch_signal 'ALRM', -> do
+    Primitive.vm_watch_signal 'ALRM', true, -> _signo do
       show_backtraces.call
     end
   end

src/main/ruby/truffleruby/core/signal.rb

@@ -117,7 +117,7 @@ def self.trap(signal, handler=nil, &block)
     end
 
     if number != Names['EXIT']
-      ret = Primitive.vm_watch_signal(signame, handler || 'IGNORE')
+      ret = Primitive.vm_watch_signal(signame, false, handler || 'IGNORE')
       if handler == 'DEFAULT' && !ret
         return +'SYSTEM_DEFAULT'
       end