about summary refs log tree commit homepage
diff options
context:
space:
mode:
authorEric Wong <normalperson@yhbt.net>2009-03-18 02:55:29 -0700
committerEric Wong <normalperson@yhbt.net>2009-03-18 17:50:33 -0700
commitf55db91a7f9e97190028839a41131ce9c308a8cf (patch)
treed653413e5ff964cc74167f5b768483c1e9521aa2
parent29c6af908c2bd1109be175c34b89c45c8cc2f60b (diff)
downloadunicorn-f55db91a7f9e97190028839a41131ce9c308a8cf.tar.gz
Although I didn't like the idea initially, signal queueing
allows test_exec to run more reliably and the limited signal
queue size will prevent scary queued signal behavior.

Also, always wakeup the master immediately when CHLD is trapped
to reduce the performance impact of SIGHUP-based config
reloading.  Combined with an extra check in test_exec, this
should make test_exec run much more reliably than before.
-rw-r--r--lib/unicorn.rb68
-rw-r--r--test/exec/test_exec.rb18
2 files changed, 48 insertions, 38 deletions
diff --git a/lib/unicorn.rb b/lib/unicorn.rb
index 4f7d417..7b7c4bb 100644
--- a/lib/unicorn.rb
+++ b/lib/unicorn.rb
@@ -53,8 +53,7 @@ module Unicorn
       @start_ctx = DEFAULT_START_CTX.dup
       @start_ctx.merge!(start_ctx) if start_ctx
       @app = app
-      @alive = true
-      @mode = :idle
+      @sig_queue = []
       @master_pid = $$
       @workers = Hash.new
       @io_purgatory = [] # prevents IO objects in here from being GC-ed
@@ -161,15 +160,17 @@ module Unicorn
       # are trapped.  See trap_deferred
       @rd_sig, @wr_sig = IO.pipe unless (@rd_sig && @wr_sig)
       @rd_sig.nonblock = @wr_sig.nonblock = true
+      ready = mode = nil
 
-      reset_master
+      QUEUE_SIGS.each { |sig| trap_deferred(sig) }
+      trap('CHLD') { |sig_nr| awaken_master }
       $0 = "unicorn master"
-      logger.info "master process ready" # test relies on this message
+      logger.info "master process ready" # test_exec.rb relies on this message
       begin
-        while @alive
+        loop do
           reap_all_workers
-          case @mode
-          when :idle
+          case (mode = @sig_queue.shift)
+          when nil
             murder_lazy_workers
             spawn_missing_workers
           when 'QUIT' # graceful shutdown
@@ -177,17 +178,14 @@ module Unicorn
           when 'TERM', 'INT' # immediate shutdown
             stop(false)
             break
-          when 'USR1' # user-defined (probably something like log reopening)
+          when 'USR1' # rotate logs
             kill_each_worker('USR1')
             Unicorn::Util.reopen_logs
-            reset_master
           when 'USR2' # exec binary, stay alive in case something went wrong
             reexec
-            reset_master
           when 'HUP'
             if @config.config_file
               load_config!
-              reset_master
               redo # immediate reaping since we may have QUIT workers
             else # exec binary and exit if there's no config file
               logger.info "config_file not present, reexecuting binary"
@@ -195,8 +193,7 @@ module Unicorn
               break
             end
           else
-            logger.error "master process in unknown mode: #{@mode}, resetting"
-            reset_master
+            logger.error "master process in unknown mode: #{mode}"
           end
           reap_all_workers
 
@@ -205,9 +202,10 @@ module Unicorn
           rescue Errno::EINTR # next
           end
           ready[0] && ready[0][0] or next
-          begin # just consume the pipe when we're awakened, @mode is set
-            loop { @rd_sig.sysread(Const::CHUNK_SIZE) }
-          rescue Errno::EAGAIN, Errno::EINTR # next
+          begin
+            @rd_sig.sysread(1)
+          rescue Errno::EAGAIN, Errno::EINTR
+            # spurious wakeup? ignore it
           end
         end
       rescue Errno::EINTR
@@ -215,7 +213,6 @@ module Unicorn
       rescue Object => e
         logger.error "Unhandled master loop exception #{e.inspect}."
         logger.error e.backtrace.join("\n")
-        reset_master
         retry
       end
       stop # gracefully shutdown all workers on our way out
@@ -242,31 +239,27 @@ module Unicorn
     private
 
     # list of signals we care about and trap in master.
-    TRAP_SIGS = %w(QUIT INT TERM USR1 USR2 HUP).map { |x| x.freeze }.freeze
+    QUEUE_SIGS = %w(QUIT INT TERM USR1 USR2 HUP).map { |x| x.freeze }.freeze
 
     # defer a signal for later processing in #join (master process)
     def trap_deferred(signal)
       trap(signal) do |sig_nr|
-        # we only handle/defer one signal at a time and ignore all others
-        # until we're ready again.  Queueing signals can lead to more bugs,
-        # and simplicity is the most important thing
-        TRAP_SIGS.each { |sig| trap(sig, 'IGNORE') }
-        if Symbol === @mode
-          @mode = signal
-          begin
-            @wr_sig.syswrite('.') # wakeup master process from IO.select
-          rescue Errno::EAGAIN
-          rescue Errno::EINTR
-            retry
-          end
+        if @sig_queue.size < 5
+          @sig_queue << signal
+          awaken_master
+        else
+          logger.error "ignoring SIG#{signal}, queue=#{@sig_queue.inspect}"
         end
       end
     end
 
-
-    def reset_master
-      @mode = :idle
-      TRAP_SIGS.each { |sig| trap_deferred(sig) }
+    def awaken_master
+      begin
+        @wr_sig.syswrite('.') # wakeup master process from IO.select
+      rescue Errno::EAGAIN # pipe is full, master should wake up anyways
+      rescue Errno::EINTR
+        retry
+      end
     end
 
     # reaps all unreaped workers
@@ -357,7 +350,7 @@ module Unicorn
           Dir.chdir(@start_ctx[:cwd])
         rescue Errno::ENOENT => err
           logger.fatal "#{err.inspect} (#{@start_ctx[:cwd]})"
-          @alive = false
+          @sig_queue << 'QUIT' # forcibly emulate SIGQUIT
           return
         end
         tempfile = Tempfile.new('') # as short as possible to save dir space
@@ -397,7 +390,8 @@ module Unicorn
     # by the user.
     def init_worker_process(worker)
       build_app! unless @preload_app
-      TRAP_SIGS.each { |sig| trap(sig, 'IGNORE') }
+      @sig_queue.clear
+      QUEUE_SIGS.each { |sig| trap(sig, 'IGNORE') }
       trap('CHLD', 'DEFAULT')
       trap('USR1') do
         @logger.info "worker=#{worker.nr} rotating logs..."
@@ -411,7 +405,7 @@ module Unicorn
       @workers.values.each { |other| other.tempfile.close rescue nil }
       @workers.clear
       @start_ctx.clear
-      @mode = @start_ctx = @workers = @rd_sig = @wr_sig = nil
+      @start_ctx = @workers = @rd_sig = @wr_sig = nil
       @listeners.each { |sock| set_cloexec(sock) }
       ENV.delete('UNICORN_FD')
       @after_fork.call(self, worker.nr) if @after_fork
diff --git a/test/exec/test_exec.rb b/test/exec/test_exec.rb
index 712037c..fc8ac26 100644
--- a/test/exec/test_exec.rb
+++ b/test/exec/test_exec.rb
@@ -283,6 +283,7 @@ end
     results = retry_hit(["http://#{@addr}:#{@port}/"])
     assert_equal String, results[0].class
     wait_master_ready(COMMON_TMP.path)
+    wait_workers_ready(COMMON_TMP.path, 4)
     bf = File.readlines(COMMON_TMP.path).grep(/\bbefore_fork: worker=/)
     assert_equal 4, bf.size
     rotate = Tempfile.new('unicorn_rotate')
@@ -496,6 +497,21 @@ end
       assert status.success?, "exited successfully"
     end
 
+    def wait_workers_ready(path, nr_workers)
+      tries = DEFAULT_TRIES
+      lines = []
+      while (tries -= 1) > 0
+        begin
+          lines = File.readlines(path).grep(/worker=\d+ spawned/)
+          lines.size == nr_workers and return
+        rescue Errno::ENOENT
+        end
+        sleep DEFAULT_RES
+      end
+      raise "#{nr_workers} workers never became ready:" \
+            "\n\t#{lines.join("\n\t")}\n"
+    end
+
     def wait_master_ready(master_log)
       tries = DEFAULT_TRIES
       while (tries -= 1) > 0
@@ -555,7 +571,7 @@ end
       while (tries -= 1) > 0 && ! File.exist?(path)
         sleep DEFAULT_RES
       end
-      assert File.exist?(path), "path=#{path} exists"
+      assert File.exist?(path), "path=#{path} exists #{caller.inspect}"
     end
 
     def xfork(&block)