[PATCH] rework master-to-worker signaling to use a pipe

2013-12-09 Thread Eric Wong
Signaling using normal kill(2) is preserved, but the master now
prefers to signal workers using a pipe rather than kill(2).
Non-graceful signals (:TERM/:KILL) are still sent using kill(2),
as they ask for immediate shutdown.

This change is necessary to avoid triggering the ubf (unblocking
function) for rb_thread_call_without_gvl (and similar) functions
extensions.  Most notably, this fixes compatibility with newer
versions of the 'pg' gem which will cancel a running DB query if
signaled[1].

This also has the nice side-effect of allowing a premature
master death (assuming preload_app didn't cause the master to
spawn off rogue child daemons).

Note: users should also refrain from using killall if using the
'pg' gem or something like it.

Unfortunately, this increases FD usage in the master as the writable
end of the pipe is preserved in the master.  This limit the number
of worker processes the master may run to the open file limit of the
master process.  Increasing the open file limit of the master
process may be needed.  However, the FD use on the workers is
reduced by one as the internal self-pipe is no longer used.  Thus,
overall pipe allocation for the kernel remains unchanged.

[1] - pg is correct to cancel a query, as it cannot know if
  the signal was for a) graceful unicorn shutdown or
  b) oh-noes-I-started-a-bad-query-ABORT-ABORT-ABORT!!
---
  Pushed to master on git://bogomips.org/unicorn.git
  commit 6f6e4115b4bb03e5e7c55def91527799190566f2

 SIGNALS|  6 
 lib/unicorn.rb |  5 +++
 lib/unicorn/http_server.rb | 85 +++---
 lib/unicorn/worker.rb  | 64 ++
 4 files changed, 118 insertions(+), 42 deletions(-)

diff --git a/SIGNALS b/SIGNALS
index 48c651e..ef0b0d9 100644
--- a/SIGNALS
+++ b/SIGNALS
@@ -45,6 +45,10 @@ http://unicorn.bogomips.org/examples/init.sh
 
 === Worker Processes
 
+Note: as of unicorn 4.8, the master uses a pipe to signal workers
+instead of kill(2) for most cases.  Using signals still (and works and
+remains supported for external tools/libraries), however.
+
 Sending signals directly to the worker processes should not normally be
 needed.  If the master process is running, any exited worker will be
 automatically respawned.
@@ -52,6 +56,8 @@ automatically respawned.
 * INT/TERM - Quick shutdown, immediately exit.
   Unless WINCH has been sent to the master (or the master is killed),
   the master process will respawn a worker to replace this one.
+  Immediate shutdown is still triggered using kill(2) and not the
+  internal pipe as of unicorn 4.8
 
 * QUIT - Gracefully exit after finishing the current request.
   Unless WINCH has been sent to the master (or the master is killed),
diff --git a/lib/unicorn.rb b/lib/unicorn.rb
index 2535159..638b846 100644
--- a/lib/unicorn.rb
+++ b/lib/unicorn.rb
@@ -97,6 +97,11 @@ module Unicorn
 logger.error #{prefix}: #{message} (#{exc.class})
 exc.backtrace.each { |line| logger.error(line) }
   end
+
+  # remove this when we only support Ruby = 2.0
+  def self.pipe # :nodoc:
+Kgio::Pipe.new.each { |io| io.fcntl(Fcntl::F_SETFD, Fcntl::FD_CLOEXEC) }
+  end
   # :startdoc:
 end
 # :enddoc:
diff --git a/lib/unicorn/http_server.rb b/lib/unicorn/http_server.rb
index f15c8a7..ae8ad13 100644
--- a/lib/unicorn/http_server.rb
+++ b/lib/unicorn/http_server.rb
@@ -42,16 +42,8 @@ class Unicorn::HttpServer
   # it to wake up the master from IO.select in exactly the same manner
   # djb describes in http://cr.yp.to/docs/selfpipe.html
   #
-  # * The workers immediately close the pipe they inherit from the
-  # master and replace it with a new pipe after forking.  This new
-  # pipe is also used to wakeup from IO.select from inside (worker)
-  # signal handlers.  However, workers *close* the pipe descriptors in
-  # the signal handlers to raise EBADF in IO.select instead of writing
-  # like we do in the master.  We cannot easily use the reader set for
-  # IO.select because LISTENERS is already that set, and it's extra
-  # work (and cycles) to distinguish the pipe FD from the reader set
-  # once IO.select returns.  So we're lazy and just close the pipe when
-  # a (rare) signal arrives in the worker and reinitialize the pipe later.
+  # * The workers immediately close the pipe they inherit.  See the
+  # Unicorn::Worker class for the pipe workers use.
   SELF_PIPE = []
 
   # signal queue used for self-piping
@@ -127,7 +119,7 @@ class Unicorn::HttpServer
 inherit_listeners!
 # this pipe is used to wake us up from select(2) in #join when signals
 # are trapped.  See trap_deferred.
-init_self_pipe!
+SELF_PIPE.replace(Unicorn.pipe)
 
 # setup signal handlers before writing pid file in case people get
 # trigger happy and send signals as soon as the pid file exists.
@@ -306,14 +298,14 @@ class Unicorn::HttpServer
 logger.info master reopening logs...
 Unicorn::Util.reopen_logs

Re: [PATCH] rework master-to-worker signaling to use a pipe

2013-12-09 Thread Eric Wong
Sam Saffron sam.saff...@gmail.com wrote:
 Bottom line is that your change is not really required.

OK.  However, the other benefit of the change is that master process
death can be detected much sooner.  (The per-process open file limit
issue doesn't really bother me with my change, since the overall kernel
pipe usage does not change).

I suppose ruby-pg users can do something like:

trap(:EXIT) { pg.cancel }

if they really want to be able to abort in-progress queries

 Thanks you so much for being super responsive here. Sorry if I caused
 you to go on a tangent you did need to go on.

No worries, I was already using a similar method to (only) detect master
process death in another master-worker server.  Anyways, there's nothing
pg-specific to it and I was always slightly worried signals would cause
some buggy code behave incorrectly in the face of EINTR (though most of
C Ruby + extensions seem well-behaved in that regard).

So I'm likely to go forward with it (if not for unicorn 4.8, then 5.0)

Thanks for bringing this issue to everyone's attention.
___
Unicorn mailing list - mongrel-unicorn@rubyforge.org
http://rubyforge.org/mailman/listinfo/mongrel-unicorn
Do not quote signatures (like this one) or top post when replying