From 5ab08c608bd93ddeed814bab6705ba83b2ec188e Mon Sep 17 00:00:00 2001 From: Michael Granger Date: Mon, 4 Mar 2013 10:53:42 -0800 Subject: [PATCH 1/2] Add deferred signal-handling (fixes #332). This uses a thread-local queue and a self-pipe so the code in the trap blocks is properly re-entrant. For details, see: * http://cr.yp.to/docs/selfpipe.html * http://blog.rubybestpractices.com/posts/ewong/016-Implementing-Signal-Handlers.html --- lib/foreman/engine.rb | 104 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 99 insertions(+), 5 deletions(-) diff --git a/lib/foreman/engine.rb b/lib/foreman/engine.rb index b62ec3f..0fd7da5 100644 --- a/lib/foreman/engine.rb +++ b/lib/foreman/engine.rb @@ -9,6 +9,10 @@ require "thread" class Foreman::Engine + # The signals that the engine cares about. + # + HANDLED_SIGNALS = [ :TERM, :INT, :HUP ] + attr_reader :env attr_reader :options attr_reader :processes @@ -33,6 +37,16 @@ class Foreman::Engine @processes = [] @running = {} @readers = {} + + # Self-pipe for deferred signal-handling (ala djb: http://cr.yp.to/docs/selfpipe.html) + reader, writer = create_pipe + reader.close_on_exec = true if reader.respond_to?(:close_on_exec) + writer.close_on_exec = true if writer.respond_to?(:close_on_exec) + @selfpipe = { :reader => reader, :writer => writer } + + # Set up a global signal queue + # http://blog.rubybestpractices.com/posts/ewong/016-Implementing-Signal-Handlers.html + Thread.main[:signal_queue] = [] end # Start the processes registered to this +Engine+ @@ -41,10 +55,7 @@ class Foreman::Engine # Make sure foreman is the process group leader. Process.setpgrp unless Foreman.windows? - trap("TERM") { puts "SIGTERM received"; terminate_gracefully } - trap("INT") { puts "SIGINT received"; terminate_gracefully } - trap("HUP") { puts "SIGHUP received"; terminate_gracefully } if ::Signal.list.keys.include? 'HUP' - + register_signal_handlers startup spawn_processes watch_for_output @@ -53,6 +64,74 @@ class Foreman::Engine shutdown end + # Set up deferred signal handlers + # + def register_signal_handlers + HANDLED_SIGNALS.each do |sig| + if ::Signal.list.include? sig.to_s + trap(sig) { Thread.main[:signal_queue] << sig ; notice_signal } + end + end + end + + # Unregister deferred signal handlers + # + def restore_default_signal_handlers + HANDLED_SIGNALS.each do |sig| + trap(sig, :DEFAULT) if ::Signal.list.include? sig.to_s + end + end + + # Wake the main thread up via the selfpipe when there's a signal + # + def notice_signal + @selfpipe[:writer].write_nonblock( '.' ) + rescue Errno::EAGAIN + # Ignore writes that would block + rescue Errno::EINT + # Retry if another signal arrived while writing + retry + end + + # Invoke the real handler for signal +sig+. This shouldn't be called directly + # by signal handlers, as it might invoke code which isn't re-entrant. + # + # @param [Symbol] sig the name of the signal to be handled + # + def handle_signal(sig) + case sig + when :TERM + handle_term_signal + when :INT + handle_interrupt + when :HUP + handle_hangup + else + system "unhandled signal #{sig}" + end + end + + # Handle a TERM signal + # + def handle_term_signal + puts "SIGTERM received" + terminate_gracefully + end + + # Handle an INT signal + # + def handle_interrupt + puts "SIGINT received" + terminate_gracefully + end + + # Handle a HUP signal + # + def handle_hangup + puts "SIGHUP received" + terminate_gracefully + end + # Register a process to be run by this +Engine+ # # @param [String] name A name for this process @@ -277,8 +356,22 @@ private Thread.new do begin loop do - io = IO.select(@readers.values, nil, nil, 30) + io = IO.select([@selfpipe[:reader]] + @readers.values, nil, nil, 30) + + begin + @selfpipe[:reader].read_nonblock(11) + rescue Errno::EAGAIN, Errno::EINTR => err + # ignore + end + + # Look for any signals that arrived and handle them + while sig = Thread.main[:signal_queue].shift + self.handle_signal(sig) + end + (io.nil? ? [] : io.first).each do |reader| + next if reader == @selfpipe[:reader] + if reader.eof? @readers.delete_if { |key, value| value == reader } else @@ -305,6 +398,7 @@ private def terminate_gracefully return if @terminating + restore_default_signal_handlers @terminating = true if Foreman.windows? system "sending SIGKILL to all processes" From 169188376baa5b5d1b59c4636fbad51d461912ac Mon Sep 17 00:00:00 2001 From: Michael Granger Date: Mon, 4 Mar 2013 14:05:53 -0800 Subject: [PATCH 2/2] Try to allow children to shut down gracefully Since signals will no longer be handled once foreman goes into `terminate_gracefully`, default signal handlers are restored so as not to cause it to get stuck in an unTERMable state. This necessitates not using the process group for signalling except as a last resort, as foreman itself will receive the signals it sends. This splits `killall` into two methods, one which signals only processes foreman itself has started, and one which signals all processes in the process group to try to clean up more aggressively, and then reworks `terminate_gracefully` to use them. --- lib/foreman/engine.rb | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/lib/foreman/engine.rb b/lib/foreman/engine.rb index 0fd7da5..7463e53 100644 --- a/lib/foreman/engine.rb +++ b/lib/foreman/engine.rb @@ -177,11 +177,11 @@ class Foreman::Engine end end - # Send a signal to all processesstarted by this +Engine+ + # Send a signal to all processes started by this +Engine+ # # @param [String] signal The signal to send to each process # - def killall(signal="SIGTERM") + def kill_children(signal="SIGTERM") if Foreman.windows? @running.each do |pid, (process, index)| system "sending #{signal} to #{name_for(pid)} at pid #{pid}" @@ -190,6 +190,21 @@ class Foreman::Engine rescue Errno::ESRCH, Errno::EPERM end end + else + begin + Process.kill signal, *@running.keys unless @running.empty? + rescue Errno::ESRCH, Errno::EPERM + end + end + end + + # Send a signal to the whole process group. + # + # @param [String] signal The signal to send + # + def killall(signal="SIGTERM") + if Foreman.windows? + kill_children(signal) else begin Process.kill "-#{signal}", Process.getpgrp @@ -402,10 +417,10 @@ private @terminating = true if Foreman.windows? system "sending SIGKILL to all processes" - killall "SIGKILL" + kill_children "SIGKILL" else system "sending SIGTERM to all processes" - killall "SIGTERM" + kill_children "SIGTERM" end Timeout.timeout(options[:timeout]) do watch_for_termination while @running.length > 0