fix SIGCHLD w/o workers + PID file renames

We'll hit SIGCHLD if our reexec process fails on us, so the
non-MP server must handle it, too.  We discovered this bug
while porting the PID file renaming changes from unicorn.

3 files changed, 110 insertions, 39 deletions

diff --git a/lib/yahns/server.rb b/lib/yahns/server.rb
index ae3c1d7..8490c5a 100644
--- a/lib/yahns/server.rb
+++ b/lib/yahns/server.rb
@@ -4,7 +4,8 @@
 require_relative 'queue_quitter'
 
 class Yahns::Server # :nodoc:
-  QUEUE_SIGS = [ :WINCH, :QUIT, :INT, :TERM, :USR1, :USR2, :HUP, :TTIN, :TTOU ]
+  QUEUE_SIGS = [ :WINCH, :QUIT, :INT, :TERM, :USR1, :USR2, :HUP, :TTIN, :TTOU,
+                 :CHLD ]
   attr_accessor :daemon_pipe
   attr_accessor :logger
   attr_writer :worker_processes
@@ -52,12 +53,11 @@ class Yahns::Server # :nodoc:
     # setup signal handlers before writing pid file in case people get
     # trigger happy and send signals as soon as the pid file exists.
     QUEUE_SIGS.each { |sig| trap(sig) { sqwakeup(sig) } }
-    self.pid = @config.value(:pid) # write pid file
     bind_new_listeners!
+    self.pid = @config.value(:pid) # write pid file
     if @worker_processes
       require 'yahns/server_mp'
       extend Yahns::ServerMP
-      mp_init
     end
     self
   end
@@ -100,6 +100,21 @@ class Yahns::Server # :nodoc:
     (set_names - cur_names).each { |addr| listen(addr) }
   end
 
+  def clobber_pid(path)
+    unlink_pid_safe(@pid) if @pid
+    if path
+      fp = begin
+        tmp = "#{File.dirname(path)}/#{rand}.#$$"
+        File.open(tmp, File::RDWR|File::CREAT|File::EXCL, 0644)
+      rescue Errno::EEXIST
+        retry
+      end
+      fp.syswrite("#$$\n")
+      File.rename(fp.path, path)
+      fp.close
+    end
+  end
+
   # sets the path for the PID file of the master process
   def pid=(path)
     if path
@@ -114,18 +129,18 @@ class Yahns::Server # :nodoc:
                              "(or pid=#{path} is stale)"
       end
     end
-    unlink_pid_safe(@pid) if @pid
 
-    if path
-      fp = begin
-        tmp = "#{File.dirname(path)}/#{rand}.#$$"
-        File.open(tmp, File::RDWR|File::CREAT|File::EXCL, 0644)
-      rescue Errno::EEXIST
-        retry
+    # rename the old pid if possible
+    if @pid && path
+      begin
+        File.rename(@pid, path)
+      rescue Errno::ENOENT, Errno::EXDEV
+        # a user may have accidentally removed the original,
+        # obviously cross-FS renames don't work, either.
+        clobber_pid(path)
       end
-      fp.syswrite("#$$\n")
-      File.rename(fp.path, path)
-      fp.close
+    else
+      clobber_pid(path)
     end
     @pid = path
   end
@@ -369,6 +384,8 @@ class Yahns::Server # :nodoc:
     case sig = @sig_queue.shift
     when :QUIT, :TERM, :INT
       return quit_enter(alive)
+    when :CHLD
+      reap_all
     when :USR1
       usr1_reopen('')
     when :USR2
@@ -396,4 +413,25 @@ class Yahns::Server # :nodoc:
   ensure
     quit_finish
   end
+
+  # reaps all unreaped workers/reexec processes
+  def reap_all
+    begin
+      wpid, status = Process.waitpid2(-1, Process::WNOHANG)
+      wpid or return
+      if @reexec_pid == wpid
+        @logger.error "reaped #{status.inspect} exec()-ed"
+        @reexec_pid = 0
+        self.pid = @pid.chomp('.oldbin') if @pid
+        proc_name('master') if @worker_processes
+      else
+        worker = @workers.delete(wpid)
+        desc = worker ? "worker=#{worker.nr}" : "(unknown)"
+        m = "reaped #{status.inspect} #{desc}"
+        status.success? ? @logger.info(m) : @logger.error(m)
+      end
+    rescue Errno::ECHILD
+      return
+    end while true
+  end
 end
diff --git a/lib/yahns/server_mp.rb b/lib/yahns/server_mp.rb
index 640f1b2..c8e1989 100644
--- a/lib/yahns/server_mp.rb
+++ b/lib/yahns/server_mp.rb
@@ -4,31 +4,6 @@
 module Yahns::ServerMP # :nodoc:
   EXIT_SIGS = [ :QUIT, :TERM, :INT ]
 
-  def mp_init
-    trap(:CHLD) { @sev.sev_signal }
-  end
-
-  # reaps all unreaped workers
-  def reap_all_workers
-    begin
-      wpid, status = Process.waitpid2(-1, Process::WNOHANG)
-      wpid or return
-      if @reexec_pid == wpid
-        @logger.error "reaped #{status.inspect} exec()-ed"
-        @reexec_pid = 0
-        self.pid = @pid.chomp('.oldbin') if @pid
-        proc_name 'master'
-      else
-        worker = @workers.delete(wpid)
-        worker_id = worker ? worker.nr : "(unknown)"
-        m = "reaped #{status.inspect} worker=#{worker_id}"
-        status.success? ? @logger.info(m) : @logger.error(m)
-      end
-    rescue Errno::ECHILD
-      return
-    end while true
-  end
-
   def maintain_worker_count
     (off = @workers.size - @worker_processes) == 0 and return
     off < 0 and return spawn_missing_workers
@@ -111,7 +86,7 @@ module Yahns::ServerMP # :nodoc:
     begin
       @sev.kgio_wait_readable
       @sev.yahns_step
-      reap_all_workers
+      reap_all
       case @sig_queue.shift
       when *EXIT_SIGS # graceful shutdown (twice for non graceful)
         @listeners.each(&:close).clear
diff --git a/test/test_server.rb b/test/test_server.rb
index d10a70e..d34ed2a 100644
--- a/test/test_server.rb
+++ b/test/test_server.rb
@@ -402,4 +402,62 @@ class TestServer < Testcase
   ensure
     quit_wait(pid)
   end
+
+  def test_pidfile_usr2
+    tmpdir = Dir.mktmpdir
+    pidf = "#{tmpdir}/pid"
+    old = "#{pidf}.oldbin"
+    err = @err
+    cfg = Yahns::Config.new
+    host, port = @srv.addr[3], @srv.addr[1]
+    cfg.instance_eval do
+      GTL.synchronize {
+        app(:rack, lambda { |_| [ 200, {}, [] ] }) { listen "#{host}:#{port}" }
+        pid pidf
+      }
+      stderr_path err.path
+    end
+    pid = mkserver(cfg) do
+      Yahns::START[0] = "sh"
+      Yahns::START[:argv] = [ '-c', "echo $$ > #{pidf}; sleep 10" ]
+    end
+
+    # ensure server is running
+    c = get_tcp_client(host, port)
+    c.write("GET / HTTP/1.0\r\n\r\n")
+    buf = Timeout.timeout(10) { c.read }
+    assert_match(/Connection: close/, buf)
+    c.close
+
+    assert_equal pid, File.read(pidf).to_i
+    before = File.stat(pidf)
+
+    # start the upgrade
+    Process.kill(:USR2, pid)
+    Timeout.timeout(10) { sleep(0.01) until File.exist?(old) }
+    after = File.stat(old)
+    assert_equal after.ino, before.ino
+    Timeout.timeout(10) { sleep(0.01) until File.exist?(pidf) }
+    new = File.read(pidf).to_i
+    refute_equal pid, new
+
+    # abort the upgrade (just wait for it to finish)
+    Process.kill(:TERM, new)
+    poke_until_dead(new)
+
+    # ensure reversion is OK
+    Timeout.timeout(10) { sleep(0.01) while File.exist?(old) }
+    after = File.stat(pidf)
+    assert_equal before.ino, after.ino
+    assert_equal before.mtime, after.mtime
+    assert_equal pid, File.read(pidf).to_i
+
+    lines = File.readlines(err.path).grep(/ERROR/)
+    assert_equal 1, lines.size
+    assert_match(/reaped/, lines[0], lines)
+    File.truncate(err.path, 0)
+  ensure
+    quit_wait(pid)
+    FileUtils.rm_rf(tmpdir)
+  end
 end