Always try to send a valid HTTP response back

This reworks error handling throughout the entire stack to be
more Ruby-ish.  Exceptions are raised instead of forcing the
us to check return values.

  If a client is sending us a bad request, we send a 400.

  If unicorn or app breaks in an unexpected way, we'll
  send a 500.

Both of these last-resort error responses are sent using
IO#write_nonblock to avoid tying Unicorn up longer than
necessary and all exceptions raised are ignored.

Sending a valid HTTP response back should reduce the chance of
us from being marked as down or broken by a load balancer.
Previously, some load balancers would mark us as down if we close
a socket without sending back a valid response; so make a best
effort to send one.  If for some reason we cannot write a valid
response, we're still susceptible to being marked as down.

A successful HttpResponse.write() call will now close the socket
immediately (instead of doing it higher up the stack).  This
ensures the errors will never get written to the socket on a
successful response.

4 files changed, 21 insertions, 15 deletions

diff --git a/lib/unicorn.rb b/lib/unicorn.rb
index 271bdab..34189ef 100644
--- a/lib/unicorn.rb
+++ b/lib/unicorn.rb
@@ -378,12 +378,21 @@ module Unicorn
     # once a client is accepted, it is processed in its entirety here
     # in 3 easy steps: read request, call app, write app response
     def process_client(client)
-      env = @request.read(client) or return
+      client.nonblock = false
+      set_client_sockopt(client) if TCPSocket === client
+      env = @request.read(client)
       app_response = @app.call(env)
       HttpResponse.write(client, app_response)
+    # 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 EOFError,Errno::ECONNRESET,Errno::EPIPE,Errno::EINVAL,Errno::EBADF
-      client.closed? or client.close rescue nil
+      client.write_nonblock(Const::ERROR_500_RESPONSE) rescue nil
+    rescue HttpParserError # try to tell the client they're bad
+      client.write_nonblock(Const::ERROR_400_RESPONSE) rescue nil
     rescue Object => e
+      client.write_nonblock(Const::ERROR_500_RESPONSE) rescue nil
       logger.error "Read error: #{e.inspect}"
       logger.error e.backtrace.join("\n")
     ensure
@@ -466,14 +475,10 @@ module Unicorn
                 next
               end
               accepted = true
-              client.nonblock = false
-              set_client_sockopt(client) if TCPSocket === client
               process_client(client)
             rescue Errno::ECONNABORTED
               # client closed the socket even before accept
-              if client && !client.closed?
-                client.close rescue nil
-              end
+              client.close rescue nil
             end
             tempfile.chmod(nr += 1)
             break if reopen_logs
diff --git a/lib/unicorn/const.rb b/lib/unicorn/const.rb
index 4e78171..f2143bf 100644
--- a/lib/unicorn/const.rb
+++ b/lib/unicorn/const.rb
@@ -79,6 +79,10 @@ module Unicorn
     # Maximum request body size before it is moved out of memory and into a tempfile for reading.
     MAX_BODY=MAX_HEADER
 
+    # common errors we'll send back
+    ERROR_400_RESPONSE = "HTTP/1.1 400 Bad Request\r\n\r\n".freeze
+    ERROR_500_RESPONSE = "HTTP/1.1 500 Internal Server Error\r\n\r\n".freeze
+
     # A frozen format for this is about 15% faster
     CONTENT_LENGTH="CONTENT_LENGTH".freeze
     REMOTE_ADDR="REMOTE_ADDR".freeze
diff --git a/lib/unicorn/http_request.rb b/lib/unicorn/http_request.rb
index 7d69943..70378ef 100644
--- a/lib/unicorn/http_request.rb
+++ b/lib/unicorn/http_request.rb
@@ -75,13 +75,13 @@ module Unicorn
                           socket.unicorn_peeraddr}): #{e.inspect}"
         @logger.error "REQUEST DATA: #{data.inspect}\n---\n" \
                       "PARAMS: #{@params.inspect}\n---\n"
-        nil
+        raise e
     end
 
     private
 
     # Handles dealing with the rest of the request
-    # returns true if successful, false if not
+    # returns a Rack environment if successful, raises an exception if not
     def handle_body(socket)
       http_body = @params[Const::HTTP_BODY]
       content_length = @params[Const::CONTENT_LENGTH].to_i
@@ -101,9 +101,7 @@ module Unicorn
       # Some clients (like FF1.0) report 0 for body and then send a body.
       # This will probably truncate them but at least the request goes through
       # usually.
-      if remain > 0
-        read_body(socket, remain) or return nil # fail!
-      end
+      read_body(socket, remain) if remain > 0
       @body.rewind
       @body.sysseek(0) if @body.respond_to?(:sysseek)
 
@@ -153,16 +151,14 @@ module Unicorn
         # writes always write the requested amount on a POSIX filesystem
         remain -= @body.syswrite(read_socket(socket))
       end
-      true # success!
     rescue Object => e
       @logger.error "Error reading HTTP body: #{e.inspect}"
-      socket.closed? or socket.close rescue nil
 
       # Any errors means we should delete the file, including if the file
       # is dumped.  Truncate it ASAP to help avoid page flushes to disk.
       @body.truncate(0) rescue nil
       reset
-      false
+      raise e
     end
 
     # read(2) on "slow" devices like sockets can be interrupted by signals
diff --git a/lib/unicorn/http_response.rb b/lib/unicorn/http_response.rb
index f928baa..98759f1 100644
--- a/lib/unicorn/http_response.rb
+++ b/lib/unicorn/http_response.rb
@@ -49,6 +49,7 @@ module Unicorn
                    "Connection: close\r\n" \
                    "#{out.join("\r\n")}\r\n\r\n")
       body.each { |chunk| socket_write(socket, chunk) }
+      socket.close # uncorks the socket immediately
       ensure
         body.respond_to?(:close) and body.close rescue nil
     end