about summary refs log tree commit homepage
diff options
context:
space:
mode:
authorEric Wong <normalperson@yhbt.net>2009-03-27 17:26:03 -0700
committerEric Wong <normalperson@yhbt.net>2009-03-27 17:26:03 -0700
commitc83b5a903a076fda67c7d062da1ad6ff9337fdd1 (patch)
treec543d22f6227be5f373274a09a05b3ce237fe869
parent9509f414a88281c93f0a1cb28123b8ae7538ee7f (diff)
downloadunicorn-c83b5a903a076fda67c7d062da1ad6ff9337fdd1.tar.gz
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.
-rw-r--r--lib/unicorn.rb19
-rw-r--r--lib/unicorn/const.rb4
-rw-r--r--lib/unicorn/http_request.rb12
-rw-r--r--lib/unicorn/http_response.rb1
-rw-r--r--test/unit/test_response.rb7
-rw-r--r--test/unit/test_server.rb29
6 files changed, 54 insertions, 18 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
diff --git a/test/unit/test_response.rb b/test/unit/test_response.rb
index 4c7423b..62cabdf 100644
--- a/test/unit/test_response.rb
+++ b/test/unit/test_response.rb
@@ -22,7 +22,7 @@ class ResponseTest < Test::Unit::TestCase
     $, = "\f\v"
     out = StringIO.new
     HttpResponse.write(out,[200, {"X-Whatever" => "stuff"}, ["cool"]])
-    resp = out.read
+    resp = out.string
     assert ! resp.include?("\f\v"), "output didn't use $, ($OFS)"
     ensure
       $, = old_ofs
@@ -38,8 +38,9 @@ class ResponseTest < Test::Unit::TestCase
     code = 400
     io = StringIO.new
     HttpResponse.write(io, [code, {}, []])
-    io.rewind
-    assert_match(/.* #{HTTP_STATUS_CODES[code]}$/, io.readline.chomp, "wrong default reason phrase")
+    lines = io.string.split(/\r\n/)
+    assert_match(/.* #{HTTP_STATUS_CODES[code]}$/, lines.first,
+                 "wrong default reason phrase")
   end
 
   def test_rack_multivalue_headers
diff --git a/test/unit/test_server.rb b/test/unit/test_server.rb
index 356f946..eadeba4 100644
--- a/test/unit/test_server.rb
+++ b/test/unit/test_server.rb
@@ -35,6 +35,25 @@ class WebServerTest < Test::Unit::TestCase
     end
   end
 
+  def test_broken_app
+    teardown
+    port = unused_port
+    app = lambda { |env| raise RuntimeError, "hello" }
+    # [200, {}, []] }
+    redirect_test_io do
+      @server = HttpServer.new(app, :listeners => [ "127.0.0.1:#{port}"] )
+      @server.start
+    end
+    sock = nil
+    assert_nothing_raised do
+      sock = TCPSocket.new('127.0.0.1', port)
+      sock.syswrite("GET / HTTP/1.0\r\n\r\n")
+    end
+
+    assert_match %r{\AHTTP/1.[01] 500\b}, sock.sysread(4096)
+    assert_nothing_raised { sock.close }
+  end
+
   def test_simple_server
     results = hit(["http://localhost:#{@port}/test"])
     assert_equal 'hello!\n', results[0], "Handler didn't really run"
@@ -77,6 +96,16 @@ class WebServerTest < Test::Unit::TestCase
     end
   end
 
+  def test_bad_client_400
+    sock = nil
+    assert_nothing_raised do
+      sock = TCPSocket.new('127.0.0.1', @port)
+      sock.syswrite("GET / HTTP/1.0\r\nHost: foo\rbar\r\n\r\n")
+    end
+    assert_match %r{\AHTTP/1.[01] 400\b}, sock.sysread(4096)
+    assert_nothing_raised { sock.close }
+  end
+
   def test_header_is_too_long
     redirect_test_io do
       long = "GET /test HTTP/1.1\r\n" + ("X-Big: stuff\r\n" * 15000) + "\r\n"