Improve specification and compatibility of implementation of Signal.trap

CHANGELOG.md

@@ -64,6 +64,7 @@ Compatibility:
 * Access to path and mode via `rb_io_t` from C has been changed to improve compatibility for io-console.
 * Implemented the `Time.at` `in:` parameter.
 * Implemented `Kernel#raise` `cause` parameter.
+* Improved compatibility of `Signal.trap` and `Kernel#trap` (#2287, @chrisseaton).
 
 Performance:
 

spec/ruby/core/kernel/trap_spec.rb

@@ -1,12 +1,9 @@
 require_relative '../../spec_helper'
-require_relative 'fixtures/classes'
 
 describe "Kernel#trap" do
   it "is a private method" do
     Kernel.should have_private_instance_method(:trap)
   end
-end
 
-describe "Kernel.trap" do
-  it "needs to be reviewed for spec completeness"
+  # Behaviour is specified for Signal.trap
 end

spec/ruby/core/signal/trap_spec.rb

@@ -1,11 +1,12 @@
 require_relative '../../spec_helper'
 
-platform_is_not :windows do
-  describe "Signal.trap" do
+describe "Signal.trap" do
+  platform_is_not :windows do
     before :each do
       ScratchPad.clear
       @proc = -> {}
       @saved_trap = Signal.trap(:HUP, @proc)
+      @hup_number = Signal.list["HUP"]
     end
 
     after :each do
@@ -16,10 +17,11 @@
       Signal.trap(:HUP, @saved_trap).should equal(@proc)
     end
 
-    it "accepts a block in place of a proc/command argument" do
+    it "accepts a block" do
       done = false
 
-      Signal.trap(:HUP) do
+      Signal.trap(:HUP) do |signo|
+        signo.should == @hup_number
         ScratchPad.record :block_trap
         done = true
       end
@@ -30,6 +32,94 @@
       ScratchPad.recorded.should == :block_trap
     end
 
+    it "accepts a proc" do
+      done = false
+
+      handler = ->(signo) {
+        signo.should == @hup_number
+        ScratchPad.record :proc_trap
+        done = true
+      }
+
+      Signal.trap(:HUP, handler)
+
+      Process.kill :HUP, Process.pid
+      Thread.pass until done
+
+      ScratchPad.recorded.should == :proc_trap
+    end
+
+    it "accepts a method" do
+      done = false
+
+      handler_class = Class.new
+      hup_number = @hup_number
+
+      handler_class.define_method :handler_method do |signo|
+        signo.should == hup_number
+        ScratchPad.record :method_trap
+        done = true
+      end
+
+      handler_method = handler_class.new.method(:handler_method)
+
+      Signal.trap(:HUP, handler_method)
+
+      Process.kill :HUP, Process.pid
+      Thread.pass until done
+
+      ScratchPad.recorded.should == :method_trap
+    end
+
+    it "accepts anything you can call" do
+      done = false
+
+      callable = Object.new
+      hup_number = @hup_number
+
+      callable.singleton_class.define_method :call do |signo|
+        signo.should == hup_number
+        ScratchPad.record :callable_trap
+        done = true
+      end
+
+      Signal.trap(:HUP, callable)
+
+      Process.kill :HUP, Process.pid
+      Thread.pass until done
+
+      ScratchPad.recorded.should == :callable_trap
+    end
+
+    it "raises an exception for a non-callable at the point of use" do
+      not_callable = Object.new
+      Signal.trap(:HUP, not_callable)
+      -> {
+        Process.kill :HUP, Process.pid
+        loop { Thread.pass }
+      }.should raise_error(NoMethodError)
+    end
+
+    it "accepts a non-callable that becomes callable when used" do
+      done = false
+
+      late_callable = Object.new
+      hup_number = @hup_number
+
+      Signal.trap(:HUP, late_callable)
+
+      late_callable.singleton_class.define_method :call do |signo|
+        signo.should == hup_number
+        ScratchPad.record :late_callable_trap
+        done = true
+      end
+
+      Process.kill :HUP, Process.pid
+      Thread.pass until done
+
+      ScratchPad.recorded.should == :late_callable_trap
+    end
+
     it "is possible to create a new Thread when the handler runs" do
       done = false
 
@@ -130,14 +220,12 @@
       Signal.trap :HUP, @proc
       Signal.trap(:HUP, @saved_trap).should equal(@proc)
     end
-  end
 
-  describe "Signal.trap" do
     # See man 2 signal
     %w[KILL STOP].each do |signal|
       it "raises ArgumentError or Errno::EINVAL for SIG#{signal}" do
         -> {
-          trap(signal, -> {})
+          Signal.trap(signal, -> {})
         }.should raise_error(StandardError) { |e|
           [ArgumentError, Errno::EINVAL].should include(e.class)
           e.message.should =~ /Invalid argument|Signal already used by VM or OS/
@@ -152,7 +240,7 @@
     end
 
     it "returns 'DEFAULT' for the initial SIGINT handler" do
-      ruby_exe('print trap(:INT) { abort }').should == 'DEFAULT'
+      ruby_exe("print Signal.trap(:INT) { abort }").should == 'DEFAULT'
     end
 
     it "returns SYSTEM_DEFAULT if passed DEFAULT and no handler was ever set" do
@@ -174,23 +262,22 @@
       Signal.signame(status.termsig).should == "PIPE"
     end
   end
-end
 
-describe "Signal.trap" do
   describe "the special EXIT signal code" do
     it "accepts the EXIT code" do
-      code = "trap(:EXIT, proc { print 1 })"
+      code = "Signal.trap(:EXIT, proc { print 1 })"
       ruby_exe(code).should == "1"
     end
 
     it "runs the proc before at_exit handlers" do
-      code = "at_exit {print 1}; trap(:EXIT, proc {print 2}); at_exit {print 3}"
+      code = "at_exit {print 1}; Signal.trap(:EXIT, proc {print 2}); at_exit {print 3}"
       ruby_exe(code).should == "231"
     end
 
     it "can unset the handler" do
-      code = "trap(:EXIT, proc { print 1 }); trap(:EXIT, 'DEFAULT')"
+      code = "Signal.trap(:EXIT, proc { print 1 }); Signal.trap(:EXIT, 'DEFAULT')"
       ruby_exe(code).should == ""
     end
   end
+
 end

spec/tags/core/signal/trap_tags.txt

@@ -1,8 +1,9 @@
+fails:Signal.trap raises an exception for a non-callable at the point of use
+fails:Signal.trap raises ArgumentError or Errno::EINVAL for SIGKILL
+fails:Signal.trap raises ArgumentError or Errno::EINVAL for SIGSTOP
 fails:Signal.trap the special EXIT signal code accepts the EXIT code
 fails:Signal.trap the special EXIT signal code runs the proc before at_exit handlers
-slow:Signal.trap the special EXIT signal code accepts the EXIT code
-slow:Signal.trap the special EXIT signal code runs the proc before at_exit handlers
-slow:Signal.trap the special EXIT signal code can unset the handler
+slow:Signal.trap allows to register a handler for all known signals, except reserved signals for which it raises ArgumentError
 slow:Signal.trap returns 'DEFAULT' for the initial SIGINT handler
 slow:Signal.trap accepts 'SYSTEM_DEFAULT' and uses the OS handler for SIGPIPE
-slow:Signal.trap allows to register a handler for all known signals, except reserved signals for which it raises ArgumentError
+slow:Signal.trap the special EXIT signal code can unset the handler

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

@@ -264,7 +264,7 @@ protected boolean watchSignalProc(Object signalString, RubyProc action,
                 context.getSafepointManager().pauseAllThreadsAndExecuteFromNonRubyThread(
                         "Handling of signal " + signal,
                         SafepointPredicate.currentFiberOfThread(context, rootThread),
-                        (rubyThread, currentNode) -> ProcOperations.rootCall(action));
+                        (rubyThread, currentNode) -> ProcOperations.rootCall(action, signal.getNumber()));
             });
         }
 

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

@@ -98,10 +98,13 @@ def self.trap(signal, handler=nil, &block)
       handler = proc { exit }
     when String
       raise ArgumentError, "Unsupported command '#{handler}'"
+    when Proc
+      # handler is already callable
     else
-      unless handler.respond_to? :call
-        raise ArgumentError, "Handler must respond to #call (was #{handler.class})"
-      end
+      underlying_handler = handler
+      handler = ->(signo) {
+        underlying_handler.call(signo)
+      }
     end
 
     had_old = @handlers.key?(number)