"unicorn -D" always returns 0 "success" (even when failed to load)

Eric Wong normalperson at yhbt.net
Sat Dec 26 20:31:15 EST 2009


Iñaki Baz Castillo <ibc at aliax.net> wrote:
> El Sábado, 26 de Diciembre de 2009, Iñaki Baz Castillo escribió:
> > El Sábado, 26 de Diciembre de 2009, Eric Wong escribió:
> > > Interesting, I could be tempted to accept this with a few changes...
> > >
> > > Process.detach() spawns a background Thread.  Having running threads
> > > before forking won't fly with certain OS threading libraries (some
> > > versions of FreeBSD, I believe, check MRI Redmine for some open bugs).
> > 
> > Yes, but there is a problem:
> > Imagine I don't use Process.detach and the master process ends when loads
> >  the rack application (due to a typo or due to an error in unicorn conf
> >  file). Then the master process would remain visible/alive but as zombie.
> > If so, when the controller process sends signal 0 to check the master
> >  process it will always receive 'true' (a zombie process is a process which
> >  can receive signals, it's does exist).
> 
> Hi, I've improved it a bit with your suggestions and also simplified
> it a lot.  Now there is no a "controller process". Instead, if
> "Unicorn.run" raises then master process rescues the exception and
> sends USR1 to parent process so it exists with 1.

Hi, I realized Unicorn.run inside the daemonize method doesn't work.
The daemonize method is also used by Rainbows! (and Zbatery).

However, I've reimplemented much of your idea here so it does not
involve sleeping at all, but rather shares a pipe with the grandparent
(started by the user) and Unicorn master process.  The added benefit of
using a pipe is that there's no fuzzy sleeping involved at or grace
periods to worry about, too.

Also pushed out to the "ready_pipe" branch of
git://git.bogomips.org/unicorn.git

Let me know how it goes.

If everything looks good, I'll consider merging for 0.96.0.

>From 5eea32764571b721cd1a89cf9ebfa853c621ac9e Mon Sep 17 00:00:00 2001
From: Eric Wong <normalperson at yhbt.net>
Date: Sat, 26 Dec 2009 17:04:57 -0800
Subject: [PATCH] exit with failure if master dies when daemonized
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This behavior change also means our grandparent (launched
from a controlling terminal or script) will wait until
the master process is ready before returning.

Thanks to Iñaki Baz Castillo for the initial implementations
and inspiration.
---
 bin/unicorn             |    2 +-
 bin/unicorn_rails       |    2 +-
 lib/unicorn.rb          |    9 +++++++--
 lib/unicorn/launcher.rb |   37 +++++++++++++++++++++++++++++--------
 test/exec/test_exec.rb  |    2 +-
 5 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/bin/unicorn b/bin/unicorn
index 651c2ff..5af021d 100755
--- a/bin/unicorn
+++ b/bin/unicorn
@@ -161,5 +161,5 @@ if $DEBUG
   })
 end
 
-Unicorn::Launcher.daemonize! if daemonize
+Unicorn::Launcher.daemonize!(options) if daemonize
 Unicorn.run(app, options)
diff --git a/bin/unicorn_rails b/bin/unicorn_rails
index 4a22a8c..b1458fc 100755
--- a/bin/unicorn_rails
+++ b/bin/unicorn_rails
@@ -202,6 +202,6 @@ end
 
 if daemonize
   options[:pid] = rails_pid
-  Unicorn::Launcher.daemonize!
+  Unicorn::Launcher.daemonize!(options)
 end
 Unicorn.run(app, options)
diff --git a/lib/unicorn.rb b/lib/unicorn.rb
index 71d5994..ae05f03 100644
--- a/lib/unicorn.rb
+++ b/lib/unicorn.rb
@@ -25,7 +25,8 @@ module Unicorn
 
   class << self
     def run(app, options = {})
-      HttpServer.new(app, options).start.join
+      ready_pipe = options.delete(:ready_pipe)
+      HttpServer.new(app, options).start.join(ready_pipe)
     end
   end
 
@@ -313,7 +314,7 @@ module Unicorn
     # (or until a termination signal is sent).  This handles signals
     # one-at-a-time time and we'll happily drop signals in case somebody
     # is signalling us too often.
-    def join
+    def join(ready_pipe = nil)
       # this pipe is used to wake us up from select(2) in #join when signals
       # are trapped.  See trap_deferred
       init_self_pipe!
@@ -324,6 +325,10 @@ module Unicorn
       trap(:CHLD) { |sig_nr| awaken_master }
       proc_name 'master'
       logger.info "master process ready" # test_exec.rb relies on this message
+      if ready_pipe
+        ready_pipe.syswrite($$.to_s)
+        ready_pipe.close
+      end
       begin
         loop do
           reap_all_workers
diff --git a/lib/unicorn/launcher.rb b/lib/unicorn/launcher.rb
index 1229b84..2d6ad97 100644
--- a/lib/unicorn/launcher.rb
+++ b/lib/unicorn/launcher.rb
@@ -19,21 +19,42 @@ class Unicorn::Launcher
   #     the directory it was started in when being re-executed
   #     to pickup code changes if the original deployment directory
   #     is a symlink or otherwise got replaced.
-  def self.daemonize!
+  def self.daemonize!(options = {})
     $stdin.reopen("/dev/null")
+    $stdin.sync = true # may not do anything...
 
     # We only start a new process group if we're not being reexecuted
     # and inheriting file descriptors from our parent
     unless ENV['UNICORN_FD']
-      exit if fork
-      Process.setsid
-      exit if fork
+      # grandparent - reads pipe, exits when master is ready
+      #  \_ parent  - exits immediately ASAP
+      #      \_ unicorn master - writes to pipe when ready
 
-      # $stderr/$stderr can/will be redirected separately in the Unicorn config
-      Unicorn::Configurator::DEFAULTS[:stderr_path] = "/dev/null"
-      Unicorn::Configurator::DEFAULTS[:stdout_path] = "/dev/null"
+      rd, wr = IO.pipe
+      grandparent = $$
+      if fork
+        wr.close # grandparent does not write
+      else
+        rd.close # unicorn master does not read
+        Process.setsid
+        exit if fork # parent dies now
+      end
+
+      if grandparent == $$
+        # this will block until HttpServer#join runs (or it dies)
+        master_pid = rd.sysread(16).to_i
+        exit!(1) unless master_pid > 1
+        exit 0
+      else # unicorn master process
+        options[:ready_pipe] = wr
+        # $stderr/$stderr can/will be redirected separately in the
+        # Unicorn config
+        Unicorn::Configurator::DEFAULTS[:stderr_path] = "/dev/null"
+        Unicorn::Configurator::DEFAULTS[:stdout_path] = "/dev/null"
+
+        # returns so Unicorn.run can happen
+      end
     end
-    $stdin.sync = $stdout.sync = $stderr.sync = true
   end
 
 end
diff --git a/test/exec/test_exec.rb b/test/exec/test_exec.rb
index fc0719b..24ba856 100644
--- a/test/exec/test_exec.rb
+++ b/test/exec/test_exec.rb
@@ -805,7 +805,7 @@ EOF
       exec($unicorn_bin, "-D", "-l#{@addr}:#{@port}", "-c#{ucfg.path}")
     end
     pid, status = Process.waitpid2(pid)
-    assert status.success?, "original process exited successfully"
+    assert ! status.success?, "original process exited successfully"
     sleep 1 # can't waitpid on a daemonized process :<
     assert err.stat.size > 0
   end
-- 
Eric Wong


More information about the mongrel-unicorn mailing list