cleanup and refactor error handling

Make sure app errors get logged correctly, and we no longer
return a 500 response when a client EOFs the write end (but not
the read end) of a connection.

9 files changed, 67 insertions, 67 deletions

diff --git a/lib/rainbows/base.rb b/lib/rainbows/base.rb
index 9a653ff..4be37f4 100644
--- a/lib/rainbows/base.rb
+++ b/lib/rainbows/base.rb
@@ -10,10 +10,9 @@ module Rainbows
     include Rainbows::Const
     G = Rainbows::G
 
-    def listen_loop_error(e)
-      G.alive or return
-      logger.error "Unhandled listen loop exception #{e.inspect}."
-      logger.error e.backtrace.join("\n")
+    def handle_error(client, e)
+      msg = Error.response(e) and client.write_nonblock(msg)
+      rescue
     end
 
     def init_worker_process(worker)
@@ -40,7 +39,7 @@ module Rainbows
 
       begin # loop
         while ! hp.headers(env, buf)
-          IO.select([client], nil, nil, G.kato) or return client.close
+          IO.select([client], nil, nil, G.kato) or return
           buf << client.readpartial(CHUNK_SIZE)
         end
 
@@ -60,13 +59,14 @@ module Rainbows
         out = [ alive ? CONN_ALIVE : CONN_CLOSE ] if hp.headers?
         HttpResponse.write(client, response, out)
       end while alive and hp.reset.nil? and env.clear
-      client.close
     # if we get any error, try to write something back to the client
     # assuming we haven't closed the socket, but don't get hung up
     # if the socket is already closed or broken.  We'll always ensure
     # the socket is closed at the end of this function
     rescue => e
       handle_error(client, e)
+    ensure
+      client.close
     end
 
     def join_threads(threads)
diff --git a/lib/rainbows/error.rb b/lib/rainbows/error.rb
new file mode 100644
index 0000000..89b1187
--- /dev/null
+++ b/lib/rainbows/error.rb
@@ -0,0 +1,34 @@
+# -*- encoding: binary -*-
+module Rainbows
+
+  class Error
+    class << self
+
+      def app(e)
+        G.server.logger.error "app error: #{e.inspect}"
+        G.server.logger.error e.backtrace.join("\n")
+        rescue
+      end
+
+      def listen_loop(e)
+        G.alive or return
+        G.server.logger.error "listen loop error: #{e.inspect}."
+        G.server.logger.error e.backtrace.join("\n")
+        rescue
+      end
+
+      def response(e)
+        case e
+        when EOFError,Errno::ECONNRESET,Errno::EPIPE,Errno::EINVAL,Errno::EBADF
+          # swallow error if client shuts down one end or disconnects
+        when Unicorn::HttpParserError
+          Const::ERROR_400_RESPONSE # try to tell the client they're bad
+        else
+          app(e)
+          Const::ERROR_500_RESPONSE
+        end
+      end
+
+    end
+  end
+end
diff --git a/lib/rainbows/ev_core.rb b/lib/rainbows/ev_core.rb
index e0fb029..f61b33f 100644
--- a/lib/rainbows/ev_core.rb
+++ b/lib/rainbows/ev_core.rb
@@ -26,17 +26,7 @@ module Rainbows
     end
 
     def handle_error(e)
-      msg = case e
-      when EOFError,Errno::ECONNRESET,Errno::EPIPE,Errno::EINVAL,Errno::EBADF
-        ERROR_500_RESPONSE
-      when HttpParserError # try to tell the client they're bad
-        ERROR_400_RESPONSE
-      else
-        G.server.logger.error "Read error: #{e.inspect}"
-        G.server.logger.error e.backtrace.join("\n")
-        ERROR_500_RESPONSE
-      end
-      write(msg)
+      msg = Error.response(e) and write(msg)
       ensure
         quit
     end
@@ -79,7 +69,7 @@ module Rainbows
           @input.close if File === @input
         end
       end
-      rescue Object => e
+      rescue => e
         handle_error(e)
     end
 
diff --git a/lib/rainbows/fiber_pool.rb b/lib/rainbows/fiber_pool.rb
index c647676..25f086e 100644
--- a/lib/rainbows/fiber_pool.rb
+++ b/lib/rainbows/fiber_pool.rb
@@ -38,7 +38,7 @@ module Rainbows
           fib.resume(Fiber::IO.new(io, fib))
         end
       rescue => e
-        listen_loop_error(e)
+        Error.listen_loop(e)
       end while G.alive || G.cur > 0
     end
 
diff --git a/lib/rainbows/fiber_spawn.rb b/lib/rainbows/fiber_spawn.rb
index 83b64de..da3db01 100644
--- a/lib/rainbows/fiber_spawn.rb
+++ b/lib/rainbows/fiber_spawn.rb
@@ -30,7 +30,7 @@ module Rainbows
           ::Fiber.new { process_client(fio.new(io, ::Fiber.current)) }.resume
         end
       rescue => e
-        listen_loop_error(e)
+        Error.listen_loop(e)
       end while G.alive || G.cur > 0
     end
 
diff --git a/lib/rainbows/rev_thread_spawn.rb b/lib/rainbows/rev_thread_spawn.rb
index b6c5eca..f2ce822 100644
--- a/lib/rainbows/rev_thread_spawn.rb
+++ b/lib/rainbows/rev_thread_spawn.rb
@@ -9,7 +9,7 @@ module Rainbows
   # A combination of the Rev and ThreadSpawn models.  This allows Ruby
   # Thread-based concurrency for application processing.  It DOES NOT
   # expose a streamable "rack.input" for upload processing within the
-  # app.  DevFdResponse may be used with this class to proxy
+  # app.  DevFdResponse should be used with this class to proxy
   # asynchronous responses.  All network I/O between the client and
   # server are handled by the main thread and outside of the core
   # application dispatch.
@@ -60,17 +60,17 @@ module Rainbows
         end
       end
 
-      def app_error(e)
-        case e
-        when EOFError,Errno::ECONNRESET,Errno::EPIPE,Errno::EINVAL,Errno::EBADF
-        else
-          begin
-            G.server.logger.error "App error: #{e.inspect}"
-            G.server.logger.error e.backtrace.join("\n")
-          rescue
-          end
+      # fails-safe application dispatch, we absolutely cannot
+      # afford to fail or raise an exception (killing the thread)
+      # here because that could cause a deadlock and we'd leak FDs
+      def app_response
+        begin
+          @env[REMOTE_ADDR] = @remote_addr
+          APP.call(@env.update(RACK_DEFAULTS))
+        rescue => e
+          Error.app(e) # we guarantee this does not raise
+          [ 500, {}, [] ]
         end
-        [ 500, {}, [] ]
       end
 
       def app_call
@@ -78,18 +78,7 @@ module Rainbows
         disable
         @env[RACK_INPUT] = @input
         @input = nil # not sure why, @input seems to get closed otherwise...
-        Thread.new do
-          @env[REMOTE_ADDR] = @remote_addr
-          begin
-            response = begin
-              APP.call(@env.update(RACK_DEFAULTS))
-            rescue => e
-              app_error(e)
-            end
-          ensure
-            MASTER << [ client, response ]
-          end
-        end
+        Thread.new { MASTER << [ client, app_response ] }
       end
     end
 
diff --git a/lib/rainbows/revactor.rb b/lib/rainbows/revactor.rb
index 489236c..4e4b381 100644
--- a/lib/rainbows/revactor.rb
+++ b/lib/rainbows/revactor.rb
@@ -63,11 +63,11 @@ module Rainbows
         out = [ alive ? CONN_ALIVE : CONN_CLOSE ] if hp.headers?
         HttpResponse.write(client, response, out)
       end while alive and hp.reset.nil? and env.clear
-      client.close
     rescue ::Revactor::TCP::ReadError
-      client.close
     rescue => e
       handle_error(client, e)
+    ensure
+      client.close
     end
 
     # runs inside each forked worker, this sits around and waits
@@ -95,8 +95,8 @@ module Rainbows
             clients[actor.object_id] = actor
             root.link(actor)
           rescue Errno::EAGAIN, Errno::ECONNABORTED
-          rescue Object => e
-            listen_loop_error(e)
+          rescue => e
+            Error.listen_loop(e)
           end while G.alive
         end
       end
@@ -119,23 +119,10 @@ module Rainbows
     # if the socket is already closed or broken.  We'll always ensure
     # the socket is closed at the end of this function
     def handle_error(client, e)
-      msg = case e
-      when EOFError,Errno::ECONNRESET,Errno::EPIPE,Errno::EINVAL,Errno::EBADF
-        Const::ERROR_500_RESPONSE
-      when HttpParserError # try to tell the client they're bad
-        Const::ERROR_400_RESPONSE
-      else
-        logger.error "Read error: #{e.inspect}"
-        logger.error e.backtrace.join("\n")
-        Const::ERROR_500_RESPONSE
-      end
-      client.instance_eval do
-        # this is Revactor implementation dependent
-        @_io.write_nonblock(msg)
-        close
-      end
+      # this is Revactor implementation dependent
+      msg = Error.response(e) and
+        client.instance_eval { @_io.write_nonblock(msg) }
       rescue
-        nil
     end
 
     def revactorize_listeners!
diff --git a/lib/rainbows/thread_pool.rb b/lib/rainbows/thread_pool.rb
index 7934dc8..949db6f 100644
--- a/lib/rainbows/thread_pool.rb
+++ b/lib/rainbows/thread_pool.rb
@@ -59,8 +59,8 @@ module Rainbows
           rescue Errno::EBADF, TypeError
             break
           end
-        rescue Object => e
-          listen_loop_error(e)
+        rescue => e
+          Error.listen_loop(e)
         end while G.alive
       }
     end
diff --git a/lib/rainbows/thread_spawn.rb b/lib/rainbows/thread_spawn.rb
index a3068c9..73a4107 100644
--- a/lib/rainbows/thread_spawn.rb
+++ b/lib/rainbows/thread_spawn.rb
@@ -51,8 +51,8 @@ module Rainbows
           rescue Errno::EAGAIN, Errno::ECONNABORTED
           end
         end
-      rescue Object => e
-        listen_loop_error(e)
+      rescue => e
+        Error.listen_loop(e)
       end while true
       join_threads(threads.list)
     end