unicorn Ruby/Rack server user+dev discussion/patches/pulls/bugs/help
 help / color / Atom feed
Search results ordered by [date|relevance]  view[summary|nested|Atom feed]
						download: 
* [PATCH] rework master-to-worker signaling to use a pipe
@ 2013-12-09  9:52 Eric Wong
  2013-12-10  0:22 ` Sam Saffron
  0 siblings, 1 reply; 5+ results
From: Eric Wong @ 2013-12-09  9:52 UTC (permalink / raw)
  To: mongrel-unicorn

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
         logger.info "master done reopening logs"
-        kill_each_worker(:USR1)
+        soft_kill_each_worker(:USR1)
       when :USR2 # exec binary, stay alive in case something went wrong
         reexec
       when :WINCH
         if Unicorn::Configurator::RACKUP[:daemonized]
           respawn = false
           logger.info "gracefully stopping all workers"
-          kill_each_worker(:QUIT)
+          soft_kill_each_worker(:QUIT)
           self.worker_processes = 0
         else
           logger.info "SIGWINCH ignored because we're not daemonized"
@@ -345,7 +337,11 @@ class Unicorn::HttpServer
     self.listeners = []
     limit = Time.now + timeout
     until WORKERS.empty? || Time.now > limit
-      kill_each_worker(graceful ? :QUIT : :TERM)
+      if graceful
+        soft_kill_each_worker(:QUIT)
+      else
+        kill_each_worker(:TERM)
+      end
       sleep(0.1)
       reap_all_workers
     end
@@ -498,6 +494,7 @@ class Unicorn::HttpServer
   end
 
   def after_fork_internal
+    SELF_PIPE.each { |io| io.close }.clear # this is master-only, now
     @ready_pipe.close if @ready_pipe
     Unicorn::Configurator::RACKUP.clear
     @ready_pipe = @init_listeners = @before_exec = @before_fork = nil
@@ -517,6 +514,7 @@ class Unicorn::HttpServer
       before_fork.call(self, worker)
       if pid = fork
         WORKERS[pid] = worker
+        worker.atfork_parent
       else
         after_fork_internal
         worker_loop(worker)
@@ -531,9 +529,7 @@ class Unicorn::HttpServer
   def maintain_worker_count
     (off = WORKERS.size - worker_processes) == 0 and return
     off < 0 and return spawn_missing_workers
-    WORKERS.dup.each_pair { |wpid,w|
-      w.nr >= worker_processes and kill_worker(:QUIT, wpid) rescue nil
-    }
+    WORKERS.each_value { |w| w.nr >= worker_processes and w.soft_kill(:QUIT) }
   end
 
   # if we get any error, try to write something back to the client
@@ -600,6 +596,7 @@ class Unicorn::HttpServer
   # traps for USR1, USR2, and HUP may be set in the after_fork Proc
   # by the user.
   def init_worker_process(worker)
+    worker.atfork_child
     # we'll re-trap :QUIT later for graceful shutdown iff we accept clients
     EXIT_SIGS.each { |sig| trap(sig) { exit!(0) } }
     exit!(0) if (SIG_QUEUE & EXIT_SIGS)[0]
@@ -608,23 +605,27 @@ class Unicorn::HttpServer
     SIG_QUEUE.clear
     proc_name "worker[#{worker.nr}]"
     START_CTX.clear
-    init_self_pipe!
     WORKERS.clear
+
+    after_fork.call(self, worker) # can drop perms and create listeners
     LISTENERS.each { |sock| sock.fcntl(Fcntl::F_SETFD, Fcntl::FD_CLOEXEC) }
-    after_fork.call(self, worker) # can drop perms
+
     worker.user(*user) if user.kind_of?(Array) && ! worker.switched
     self.timeout /= 2.0 # halve it for select()
     @config = nil
     build_app! unless preload_app
     ssl_enable!
     @after_fork = @listener_opts = @orig_app = nil
+    readers = LISTENERS.dup
+    readers << worker
+    trap(:QUIT) { readers.each { |io| io.close }.replace([false]) }
+    readers
   end
 
   def reopen_worker_logs(worker_nr)
     logger.info "worker=#{worker_nr} reopening logs..."
     Unicorn::Util.reopen_logs
     logger.info "worker=#{worker_nr} done reopening logs"
-    init_self_pipe!
     rescue => e
       logger.error(e) rescue nil
       exit!(77) # EX_NOPERM in sysexits.h
@@ -635,22 +636,24 @@ class Unicorn::HttpServer
   # given a INT, QUIT, or TERM signal)
   def worker_loop(worker)
     ppid = master_pid
-    init_worker_process(worker)
+    readers = init_worker_process(worker)
     nr = 0 # this becomes negative if we need to reopen logs
-    l = LISTENERS.dup
-    ready = l.dup
 
-    # closing anything we IO.select on will raise EBADF
-    trap(:USR1) { nr = -65536; SELF_PIPE[0].close rescue nil }
-    trap(:QUIT) { worker = nil; LISTENERS.each { |s| s.close rescue nil }.clear }
-    logger.info "worker=#{worker.nr} ready"
+    # this only works immediately if the master sent us the signal
+    # (which is the normal case)
+    trap(:USR1) { nr = -65536 }
+
+    ready = readers.dup
+    @logger.info "worker=#{worker.nr} ready"
 
     begin
       nr < 0 and reopen_worker_logs(worker.nr)
       nr = 0
-
       worker.tick = Time.now.to_i
-      while sock = ready.shift
+      tmp = ready.dup
+      while sock = tmp.shift
+        # Unicorn::Worker#kgio_tryaccept is not like accept(2) at all,
+        # but that will return false
         if client = sock.kgio_tryaccept
           process_client(client)
           nr += 1
@@ -663,8 +666,8 @@ class Unicorn::HttpServer
       # we're probably reasonably busy, so avoid calling select()
       # and do a speculative non-blocking accept() on ready listeners
       # before we sleep again in select().
-      unless nr == 0 # (nr < 0) => reopen logs (unlikely)
-        ready = l.dup
+      unless nr == 0
+        tmp = ready.dup
         redo
       end
 
@@ -672,11 +675,11 @@ class Unicorn::HttpServer
 
       # timeout used so we can detect parent death:
       worker.tick = Time.now.to_i
-      ret = IO.select(l, nil, SELF_PIPE, @timeout) and ready = ret[0]
+      ret = IO.select(readers, nil, nil, @timeout) and ready = ret[0]
     rescue => e
-      redo if nr < 0 && (Errno::EBADF === e || IOError === e) # reopen logs
-      Unicorn.log_error(@logger, "listen loop error", e) if worker
-    end while worker
+      redo if nr < 0
+      Unicorn.log_error(@logger, "listen loop error", e) if readers[0]
+    end while readers[0]
   end
 
   # delivers a signal to a worker and fails gracefully if the worker
@@ -692,6 +695,10 @@ class Unicorn::HttpServer
     WORKERS.keys.each { |wpid| kill_worker(signal, wpid) }
   end
 
+  def soft_kill_each_worker(signal)
+    WORKERS.each_value { |worker| worker.soft_kill(signal) }
+  end
+
   # unlinks a PID file at given +path+ if it contains the current PID
   # still potentially racy without locking the directory (which is
   # non-portable and may interact badly with other programs), but the
@@ -720,7 +727,7 @@ class Unicorn::HttpServer
     config[:listeners].replace(@init_listeners)
     config.reload
     config.commit!(self)
-    kill_each_worker(:QUIT)
+    soft_kill_each_worker(:QUIT)
     Unicorn::Util.reopen_logs
     self.app = orig_app
     build_app! if preload_app
@@ -756,12 +763,6 @@ class Unicorn::HttpServer
     io.sync = true
   end
 
-  def init_self_pipe!
-    SELF_PIPE.each { |io| io.close rescue nil }
-    SELF_PIPE.replace(Kgio::Pipe.new)
-    SELF_PIPE.each { |io| io.fcntl(Fcntl::F_SETFD, Fcntl::FD_CLOEXEC) }
-  end
-
   def inherit_listeners!
     # inherit sockets from parents, they need to be plain Socket objects
     # before they become Kgio::UNIXServer or Kgio::TCPServer
diff --git a/lib/unicorn/worker.rb b/lib/unicorn/worker.rb
index 1fb6a4a..e74a1c9 100644
--- a/lib/unicorn/worker.rb
+++ b/lib/unicorn/worker.rb
@@ -12,6 +12,7 @@ class Unicorn::Worker
   # :stopdoc:
   attr_accessor :nr, :switched
   attr_writer :tmp
+  attr_reader :to_io # IO.select-compatible
 
   PER_DROP = Raindrops::PAGE_SIZE / Raindrops::SIZE
   DROPS = []
@@ -23,6 +24,66 @@ class Unicorn::Worker
     @raindrop[@offset] = 0
     @nr = nr
     @tmp = @switched = false
+    @to_io, @master = Unicorn.pipe
+  end
+
+  def atfork_child # :nodoc:
+    # we _must_ close in child, parent just holds this open to signal
+    @master = @master.close
+  end
+
+  # master fakes SIGQUIT using this
+  def quit # :nodoc:
+    @master = @master.close if @master
+  end
+
+  # parent does not read
+  def atfork_parent # :nodoc:
+    @to_io = @to_io.close
+  end
+
+  # call a signal handler immediately without triggering EINTR
+  # We do not use the more obvious Process.kill(sig, $$) here since
+  # that signal delivery may be deferred.  We want to avoid signal delivery
+  # while the Rack app.call is running because some database drivers
+  # (e.g. ruby-pg) may cancel pending requests.
+  def fake_sig(sig) # :nodoc:
+    old_cb = trap(sig, "IGNORE")
+    old_cb.call
+  ensure
+    trap(sig, old_cb)
+  end
+
+  # master sends fake signals to children
+  def soft_kill(sig) # :nodoc:
+    case sig
+    when Integer
+      signum = sig
+    else
+      signum = Signal.list[sig.to_s] or
+          raise ArgumentError, "BUG: bad signal: #{sig.inspect}"
+    end
+    # writing and reading 4 bytes on a pipe is atomic on all POSIX platforms
+    # Do not care in the odd case the buffer is full, here.
+    @master.kgio_trywrite([signum].pack('l'))
+  rescue Errno::EPIPE
+    # worker will be reaped soon
+  end
+
+  # this only runs when the Rack app.call is not running
+  # act like a listener
+  def kgio_tryaccept # :nodoc:
+    case buf = @to_io.kgio_tryread(4)
+    when String
+      # unpack the buffer and trigger the signal handler
+      signum = buf.unpack('l')
+      fake_sig(signum[0])
+      # keep looping, more signals may be queued
+    when nil # EOF: master died, but we are at a safe place to exit
+      fake_sig(:QUIT)
+    when :wait_readable # keep waiting
+      return false
+    end while true # loop, as multiple signals may be sent
   end
 
   # worker objects may be compared to just plain Integers
@@ -49,8 +110,11 @@ class Unicorn::Worker
     end
   end
 
+  # called in both the master (reaping worker) and worker (SIGQUIT handler)
   def close # :nodoc:
     @tmp.close if @tmp
+    @master.close if @master
+    @to_io.close if @to_io
   end
 
   # :startdoc:
-- 
Eric Wong
_______________________________________________
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

^ permalink raw reply	[relevance 1%]

* [ANN] unicorn 4.8.0.pre1 prerelease gem
@ 2013-12-09  9:54 Eric Wong
  2014-01-09 21:50 ` Eric Wong
  0 siblings, 1 reply; 5+ results
From: Eric Wong @ 2013-12-09  9:54 UTC (permalink / raw)
  To: mongrel-unicorn

Changes since 4.7.0:

Eric Wong (6):
      tests: fix SO_REUSEPORT tests for old Linux and non-Linux
      stream_input: avoid IO#close on client disconnect
      t0300: kill off stray processes in test
      always write PID file early for compatibility
      doc: clarify SIGNALS and reference init example
      rework master-to-worker signaling to use a pipe

gem install --pre unicorn

git://bogomips.org/unicorn
_______________________________________________
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

^ permalink raw reply	[relevance 7%]

* Re: [PATCH] rework master-to-worker signaling to use a pipe
  2013-12-09  9:52 [PATCH] rework master-to-worker signaling to use a pipe Eric Wong
@ 2013-12-10  0:22 ` Sam Saffron
  2013-12-10  1:58   ` Eric Wong
  0 siblings, 1 reply; 5+ results
From: Sam Saffron @ 2013-12-10  0:22 UTC (permalink / raw)
  To: unicorn list

Eric,

Your work on unicorn is exemplary and responsiveness second to none. I
did want to take a full circle back though on the issue.

After discussing with tmm1 and ko1 the conclusion is that pg is using
ubfs incorrectly. They are just there to unblock io, not as a
termination hook, once io is unblocked you still need to be able to
re-acquire. pg was using ubfs as termination hooks. See full
discussion at: https://groups.google.com/forum/#!topic/ruby-pg/5_ylGmog1S4

The end result is that I suggested Lars open a ticket for redmine if
we need a ubfs to act as termination hooks, the change to pg was
reverted.

Bottom line is that your change is not really required.

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.

Sam

On Mon, Dec 9, 2013 at 8:52 PM, Eric Wong <normalperson@yhbt.net> wrote:
> 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
>          logger.info "master done reopening logs"
> -        kill_each_worker(:USR1)
> +        soft_kill_each_worker(:USR1)
>        when :USR2 # exec binary, stay alive in case something went wrong
>          reexec
>        when :WINCH
>          if Unicorn::Configurator::RACKUP[:daemonized]
>            respawn = false
>            logger.info "gracefully stopping all workers"
> -          kill_each_worker(:QUIT)
> +          soft_kill_each_worker(:QUIT)
>            self.worker_processes = 0
>          else
>            logger.info "SIGWINCH ignored because we're not daemonized"
> @@ -345,7 +337,11 @@ class Unicorn::HttpServer
>      self.listeners = []
>      limit = Time.now + timeout
>      until WORKERS.empty? || Time.now > limit
> -      kill_each_worker(graceful ? :QUIT : :TERM)
> +      if graceful
> +        soft_kill_each_worker(:QUIT)
> +      else
> +        kill_each_worker(:TERM)
> +      end
>        sleep(0.1)
>        reap_all_workers
>      end
> @@ -498,6 +494,7 @@ class Unicorn::HttpServer
>    end
>
>    def after_fork_internal
> +    SELF_PIPE.each { |io| io.close }.clear # this is master-only, now
>      @ready_pipe.close if @ready_pipe
>      Unicorn::Configurator::RACKUP.clear
>      @ready_pipe = @init_listeners = @before_exec = @before_fork = nil
> @@ -517,6 +514,7 @@ class Unicorn::HttpServer
>        before_fork.call(self, worker)
>        if pid = fork
>          WORKERS[pid] = worker
> +        worker.atfork_parent
>        else
>          after_fork_internal
>          worker_loop(worker)
> @@ -531,9 +529,7 @@ class Unicorn::HttpServer
>    def maintain_worker_count
>      (off = WORKERS.size - worker_processes) == 0 and return
>      off < 0 and return spawn_missing_workers
> -    WORKERS.dup.each_pair { |wpid,w|
> -      w.nr >= worker_processes and kill_worker(:QUIT, wpid) rescue nil
> -    }
> +    WORKERS.each_value { |w| w.nr >= worker_processes and w.soft_kill(:QUIT) }
>    end
>
>    # if we get any error, try to write something back to the client
> @@ -600,6 +596,7 @@ class Unicorn::HttpServer
>    # traps for USR1, USR2, and HUP may be set in the after_fork Proc
>    # by the user.
>    def init_worker_process(worker)
> +    worker.atfork_child
>      # we'll re-trap :QUIT later for graceful shutdown iff we accept clients
>      EXIT_SIGS.each { |sig| trap(sig) { exit!(0) } }
>      exit!(0) if (SIG_QUEUE & EXIT_SIGS)[0]
> @@ -608,23 +605,27 @@ class Unicorn::HttpServer
>      SIG_QUEUE.clear
>      proc_name "worker[#{worker.nr}]"
>      START_CTX.clear
> -    init_self_pipe!
>      WORKERS.clear
> +
> +    after_fork.call(self, worker) # can drop perms and create listeners
>      LISTENERS.each { |sock| sock.fcntl(Fcntl::F_SETFD, Fcntl::FD_CLOEXEC) }
> -    after_fork.call(self, worker) # can drop perms
> +
>      worker.user(*user) if user.kind_of?(Array) && ! worker.switched
>      self.timeout /= 2.0 # halve it for select()
>      @config = nil
>      build_app! unless preload_app
>      ssl_enable!
>      @after_fork = @listener_opts = @orig_app = nil
> +    readers = LISTENERS.dup
> +    readers << worker
> +    trap(:QUIT) { readers.each { |io| io.close }.replace([false]) }
> +    readers
>    end
>
>    def reopen_worker_logs(worker_nr)
>      logger.info "worker=#{worker_nr} reopening logs..."
>      Unicorn::Util.reopen_logs
>      logger.info "worker=#{worker_nr} done reopening logs"
> -    init_self_pipe!
>      rescue => e
>        logger.error(e) rescue nil
>        exit!(77) # EX_NOPERM in sysexits.h
> @@ -635,22 +636,24 @@ class Unicorn::HttpServer
>    # given a INT, QUIT, or TERM signal)
>    def worker_loop(worker)
>      ppid = master_pid
> -    init_worker_process(worker)
> +    readers = init_worker_process(worker)
>      nr = 0 # this becomes negative if we need to reopen logs
> -    l = LISTENERS.dup
> -    ready = l.dup
>
> -    # closing anything we IO.select on will raise EBADF
> -    trap(:USR1) { nr = -65536; SELF_PIPE[0].close rescue nil }
> -    trap(:QUIT) { worker = nil; LISTENERS.each { |s| s.close rescue nil }.clear }
> -    logger.info "worker=#{worker.nr} ready"
> +    # this only works immediately if the master sent us the signal
> +    # (which is the normal case)
> +    trap(:USR1) { nr = -65536 }
> +
> +    ready = readers.dup
> +    @logger.info "worker=#{worker.nr} ready"
>
>      begin
>        nr < 0 and reopen_worker_logs(worker.nr)
>        nr = 0
> -
>        worker.tick = Time.now.to_i
> -      while sock = ready.shift
> +      tmp = ready.dup
> +      while sock = tmp.shift
> +        # Unicorn::Worker#kgio_tryaccept is not like accept(2) at all,
> +        # but that will return false
>          if client = sock.kgio_tryaccept
>            process_client(client)
>            nr += 1
> @@ -663,8 +666,8 @@ class Unicorn::HttpServer
>        # we're probably reasonably busy, so avoid calling select()
>        # and do a speculative non-blocking accept() on ready listeners
>        # before we sleep again in select().
> -      unless nr == 0 # (nr < 0) => reopen logs (unlikely)
> -        ready = l.dup
> +      unless nr == 0
> +        tmp = ready.dup
>          redo
>        end
>
> @@ -672,11 +675,11 @@ class Unicorn::HttpServer
>
>        # timeout used so we can detect parent death:
>        worker.tick = Time.now.to_i
> -      ret = IO.select(l, nil, SELF_PIPE, @timeout) and ready = ret[0]
> +      ret = IO.select(readers, nil, nil, @timeout) and ready = ret[0]
>      rescue => e
> -      redo if nr < 0 && (Errno::EBADF === e || IOError === e) # reopen logs
> -      Unicorn.log_error(@logger, "listen loop error", e) if worker
> -    end while worker
> +      redo if nr < 0
> +      Unicorn.log_error(@logger, "listen loop error", e) if readers[0]
> +    end while readers[0]
>    end
>
>    # delivers a signal to a worker and fails gracefully if the worker
> @@ -692,6 +695,10 @@ class Unicorn::HttpServer
>      WORKERS.keys.each { |wpid| kill_worker(signal, wpid) }
>    end
>
> +  def soft_kill_each_worker(signal)
> +    WORKERS.each_value { |worker| worker.soft_kill(signal) }
> +  end
> +
>    # unlinks a PID file at given +path+ if it contains the current PID
>    # still potentially racy without locking the directory (which is
>    # non-portable and may interact badly with other programs), but the
> @@ -720,7 +727,7 @@ class Unicorn::HttpServer
>      config[:listeners].replace(@init_listeners)
>      config.reload
>      config.commit!(self)
> -    kill_each_worker(:QUIT)
> +    soft_kill_each_worker(:QUIT)
>      Unicorn::Util.reopen_logs
>      self.app = orig_app
>      build_app! if preload_app
> @@ -756,12 +763,6 @@ class Unicorn::HttpServer
>      io.sync = true
>    end
>
> -  def init_self_pipe!
> -    SELF_PIPE.each { |io| io.close rescue nil }
> -    SELF_PIPE.replace(Kgio::Pipe.new)
> -    SELF_PIPE.each { |io| io.fcntl(Fcntl::F_SETFD, Fcntl::FD_CLOEXEC) }
> -  end
> -
>    def inherit_listeners!
>      # inherit sockets from parents, they need to be plain Socket objects
>      # before they become Kgio::UNIXServer or Kgio::TCPServer
> diff --git a/lib/unicorn/worker.rb b/lib/unicorn/worker.rb
> index 1fb6a4a..e74a1c9 100644
> --- a/lib/unicorn/worker.rb
> +++ b/lib/unicorn/worker.rb
> @@ -12,6 +12,7 @@ class Unicorn::Worker
>    # :stopdoc:
>    attr_accessor :nr, :switched
>    attr_writer :tmp
> +  attr_reader :to_io # IO.select-compatible
>
>    PER_DROP = Raindrops::PAGE_SIZE / Raindrops::SIZE
>    DROPS = []
> @@ -23,6 +24,66 @@ class Unicorn::Worker
>      @raindrop[@offset] = 0
>      @nr = nr
>      @tmp = @switched = false
> +    @to_io, @master = Unicorn.pipe
> +  end
> +
> +  def atfork_child # :nodoc:
> +    # we _must_ close in child, parent just holds this open to signal
> +    @master = @master.close
> +  end
> +
> +  # master fakes SIGQUIT using this
> +  def quit # :nodoc:
> +    @master = @master.close if @master
> +  end
> +
> +  # parent does not read
> +  def atfork_parent # :nodoc:
> +    @to_io = @to_io.close
> +  end
> +
> +  # call a signal handler immediately without triggering EINTR
> +  # We do not use the more obvious Process.kill(sig, $$) here since
> +  # that signal delivery may be deferred.  We want to avoid signal delivery
> +  # while the Rack app.call is running because some database drivers
> +  # (e.g. ruby-pg) may cancel pending requests.
> +  def fake_sig(sig) # :nodoc:
> +    old_cb = trap(sig, "IGNORE")
> +    old_cb.call
> +  ensure
> +    trap(sig, old_cb)
> +  end
> +
> +  # master sends fake signals to children
> +  def soft_kill(sig) # :nodoc:
> +    case sig
> +    when Integer
> +      signum = sig
> +    else
> +      signum = Signal.list[sig.to_s] or
> +          raise ArgumentError, "BUG: bad signal: #{sig.inspect}"
> +    end
> +    # writing and reading 4 bytes on a pipe is atomic on all POSIX platforms
> +    # Do not care in the odd case the buffer is full, here.
> +    @master.kgio_trywrite([signum].pack('l'))
> +  rescue Errno::EPIPE
> +    # worker will be reaped soon
> +  end
> +
> +  # this only runs when the Rack app.call is not running
> +  # act like a listener
> +  def kgio_tryaccept # :nodoc:
> +    case buf = @to_io.kgio_tryread(4)
> +    when String
> +      # unpack the buffer and trigger the signal handler
> +      signum = buf.unpack('l')
> +      fake_sig(signum[0])
> +      # keep looping, more signals may be queued
> +    when nil # EOF: master died, but we are at a safe place to exit
> +      fake_sig(:QUIT)
> +    when :wait_readable # keep waiting
> +      return false
> +    end while true # loop, as multiple signals may be sent
>    end
>
>    # worker objects may be compared to just plain Integers
> @@ -49,8 +110,11 @@ class Unicorn::Worker
>      end
>    end
>
> +  # called in both the master (reaping worker) and worker (SIGQUIT handler)
>    def close # :nodoc:
>      @tmp.close if @tmp
> +    @master.close if @master
> +    @to_io.close if @to_io
>    end
>
>    # :startdoc:
> --
> Eric Wong
> _______________________________________________
> 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
_______________________________________________
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

^ permalink raw reply	[relevance 4%]

* Re: [PATCH] rework master-to-worker signaling to use a pipe
  2013-12-10  0:22 ` Sam Saffron
@ 2013-12-10  1:58   ` Eric Wong
  0 siblings, 0 replies; 5+ results
From: Eric Wong @ 2013-12-10  1:58 UTC (permalink / raw)
  To: unicorn list

Sam Saffron <sam.saffron@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

^ permalink raw reply	[relevance 4%]

* Re: [ANN] unicorn 4.8.0.pre1 prerelease gem
  2013-12-09  9:54 [ANN] unicorn 4.8.0.pre1 prerelease gem Eric Wong
@ 2014-01-09 21:50 ` Eric Wong
  0 siblings, 0 replies; 5+ results
From: Eric Wong @ 2014-01-09 21:50 UTC (permalink / raw)
  To: mongrel-unicorn

Eric Wong <normalperson@yhbt.net> wrote:
>       tests: fix SO_REUSEPORT tests for old Linux and non-Linux
>       stream_input: avoid IO#close on client disconnect
>       t0300: kill off stray processes in test
>       always write PID file early for compatibility
>       doc: clarify SIGNALS and reference init example
>       rework master-to-worker signaling to use a pipe

Btw, has anybody tried this?  I haven't noticed any issues, and I'm
thinking about releasing this as 4.8.0 as-is (with some minor doc
updates)
_______________________________________________
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

^ permalink raw reply	[relevance 0%]

Results 1-5 of 5 | reverse results
2013-12-09  9:52 [PATCH] rework master-to-worker signaling to use a pipe Eric Wong
2013-12-10  0:22 ` Sam Saffron
2013-12-10  1:58   ` Eric Wong
2013-12-09  9:54 [ANN] unicorn 4.8.0.pre1 prerelease gem Eric Wong
2014-01-09 21:50 ` Eric Wong


unicorn Ruby/Rack server user+dev discussion/patches/pulls/bugs/help

Archives are clonable:
	git clone --mirror https://bogomips.org/unicorn-public
	git clone --mirror http://ou63pmih66umazou.onion/unicorn-public

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.lang.ruby.unicorn
	nntp://ou63pmih66umazou.onion/inbox.comp.lang.ruby.unicorn

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git