summary refs log tree commit homepage
diff options
context:
space:
mode:
authorEric Wong <normalperson@yhbt.net>2010-08-27 20:29:55 +0000
committerEric Wong <normalperson@yhbt.net>2010-10-04 21:01:27 +0000
commit50c11036dd4898ccfed8b3e0552e88c67b6c63a9 (patch)
tree21cae94fbbfcc52f7c247e4942b51e5e47007d81
parent7a3efe8a03f85c1f2957130986c24ef7931ff44a (diff)
There's no need for a response class or object since Rack just
uses an array as the response.  So use a procedural style which
allows for easier understanding.

We shall also support keepalive/pipelining in the future, too.
-rw-r--r--lib/unicorn.rb11
-rw-r--r--lib/unicorn/http_response.rb50
-rw-r--r--test/unit/test_response.rb23
3 files changed, 34 insertions, 50 deletions
diff --git a/lib/unicorn.rb b/lib/unicorn.rb
index 7f91352..2a5f493 100644
--- a/lib/unicorn.rb
+++ b/lib/unicorn.rb
@@ -90,6 +90,7 @@ module Unicorn
                                 :reexec_pid, :orig_app, :init_listeners,
                                 :master_pid, :config, :ready_pipe, :user)
     include ::Unicorn::SocketHelper
+    include ::Unicorn::HttpResponse
 
     # prevents IO objects in here from being GC-ed
     IO_PURGATORY = []
@@ -626,14 +627,16 @@ module Unicorn
     # in 3 easy steps: read request, call app, write app response
     def process_client(client)
       client.fcntl(Fcntl::F_SETFD, Fcntl::FD_CLOEXEC)
-      response = app.call(env = REQUEST.read(client))
+      r = app.call(env = REQUEST.read(client))
 
-      if 100 == response[0].to_i
+      if 100 == r[0].to_i
         client.write(Const::EXPECT_100_RESPONSE)
         env.delete(Const::HTTP_EXPECT)
-        response = app.call(env)
+        r = app.call(env)
       end
-      HttpResponse.write(client, response, HttpRequest::PARSER.headers?)
+      # r may be frozen or const, so don't modify it
+      HttpRequest::PARSER.headers? or r = [ r[0], nil, r[2] ]
+      http_response_write(client, r)
     rescue => e
       handle_error(client, e)
     end
diff --git a/lib/unicorn/http_response.rb b/lib/unicorn/http_response.rb
index 6f1cd48..5725e25 100644
--- a/lib/unicorn/http_response.rb
+++ b/lib/unicorn/http_response.rb
@@ -5,19 +5,12 @@ require 'time'
 # You use it by simply doing:
 #
 #   status, headers, body = rack_app.call(env)
-#   HttpResponse.write(socket, [ status, headers, body ])
+#   http_response_write(socket, [ status, headers, body ])
 #
 # Most header correctness (including Content-Length and Content-Type)
-# is the job of Rack, with the exception of the "Connection: close"
-# and "Date" headers.
+# is the job of Rack, with the exception of the "Date" and "Status" header.
 #
-# A design decision was made to force the client to not pipeline or
-# keepalive requests.  HTTP/1.1 pipelining really kills the
-# performance due to how it has to be handled and how unclear the
-# standard is.  To fix this the HttpResponse always gives a
-# "Connection: close" header which forces the client to close right
-# away.  The bonus for this is that it gives a pretty nice speed boost
-# to most clients since they can close their connection immediately.
+# TODO: allow keepalive
 module Unicorn::HttpResponse
 
   # Every standard HTTP code mapped to the appropriate message.
@@ -25,41 +18,28 @@ module Unicorn::HttpResponse
     hash[code] = "#{code} #{msg}"
     hash
   }
-
-  # Rack does not set/require a Date: header.  We always override the
-  # Connection: and Date: headers no matter what (if anything) our
-  # Rack application sent us.
-  SKIP = { 'connection' => true, 'date' => true, 'status' => true }
+  CRLF = "\r\n"
 
   # writes the rack_response to socket as an HTTP response
-  def self.write(socket, rack_response, have_header = true)
+  def http_response_write(socket, rack_response)
     status, headers, body = rack_response
+    status = CODES[status.to_i] || status
 
-    if have_header
-      status = CODES[status.to_i] || status
-      out = []
-
-      # Don't bother enforcing duplicate supression, it's a Hash most of
-      # the time anyways so just hope our app knows what it's doing
+    if headers
+      buf = "HTTP/1.1 #{status}\r\n" \
+            "Date: #{Time.now.httpdate}\r\n" \
+            "Status: #{status}\r\n" \
+            "Connection: close\r\n"
       headers.each do |key, value|
-        next if SKIP.include?(key.downcase)
+        next if %r{\A(?:Date\z|Status\z|Connection\z)}i =~ key
         if value =~ /\n/
           # avoiding blank, key-only cookies with /\n+/
-          out.concat(value.split(/\n+/).map! { |v| "#{key}: #{v}\r\n" })
+          buf << value.split(/\n+/).map! { |v| "#{key}: #{v}\r\n" }.join('')
         else
-          out << "#{key}: #{value}\r\n"
+          buf << "#{key}: #{value}\r\n"
         end
       end
-
-      # Rack should enforce Content-Length or chunked transfer encoding,
-      # so don't worry or care about them.
-      # Date is required by HTTP/1.1 as long as our clock can be trusted.
-      # Some broken clients require a "Status" header so we accomodate them
-      socket.write("HTTP/1.1 #{status}\r\n" \
-                   "Date: #{Time.now.httpdate}\r\n" \
-                   "Status: #{status}\r\n" \
-                   "Connection: close\r\n" \
-                   "#{out.join('')}\r\n")
+      socket.write(buf << CRLF)
     end
 
     body.each { |chunk| socket.write(chunk) }
diff --git a/test/unit/test_response.rb b/test/unit/test_response.rb
index f9eda8e..e5245e8 100644
--- a/test/unit/test_response.rb
+++ b/test/unit/test_response.rb
@@ -11,10 +11,11 @@ require 'test/test_helper'
 include Unicorn
 
 class ResponseTest < Test::Unit::TestCase
-  
+  include Unicorn::HttpResponse
+
   def test_response_headers
     out = StringIO.new
-    HttpResponse.write(out,[200, {"X-Whatever" => "stuff"}, ["cool"]])
+    http_response_write(out,[200, {"X-Whatever" => "stuff"}, ["cool"]])
     assert out.closed?
 
     assert out.length > 0, "output didn't have data"
@@ -22,7 +23,7 @@ class ResponseTest < Test::Unit::TestCase
 
   def test_response_string_status
     out = StringIO.new
-    HttpResponse.write(out,['200', {}, []])
+    http_response_write(out,['200', {}, []])
     assert out.closed?
     assert out.length > 0, "output didn't have data"
     assert_equal 1, out.string.split(/\r\n/).grep(/^Status: 200 OK/).size
@@ -32,7 +33,7 @@ class ResponseTest < Test::Unit::TestCase
     old_ofs = $,
     $, = "\f\v"
     out = StringIO.new
-    HttpResponse.write(out,[200, {"X-k" => "cd","X-y" => "z"}, ["cool"]])
+    http_response_write(out,[200, {"X-k" => "cd","X-y" => "z"}, ["cool"]])
     assert out.closed?
     resp = out.string
     assert ! resp.include?("\f\v"), "output didn't use $, ($OFS)"
@@ -42,7 +43,7 @@ class ResponseTest < Test::Unit::TestCase
 
   def test_response_200
     io = StringIO.new
-    HttpResponse.write(io, [200, {}, []])
+    http_response_write(io, [200, {}, []])
     assert io.closed?
     assert io.length > 0, "output didn't have data"
   end
@@ -50,7 +51,7 @@ class ResponseTest < Test::Unit::TestCase
   def test_response_with_default_reason
     code = 400
     io = StringIO.new
-    HttpResponse.write(io, [code, {}, []])
+    http_response_write(io, [code, {}, []])
     assert io.closed?
     lines = io.string.split(/\r\n/)
     assert_match(/.* Bad Request$/, lines.first,
@@ -59,7 +60,7 @@ class ResponseTest < Test::Unit::TestCase
 
   def test_rack_multivalue_headers
     out = StringIO.new
-    HttpResponse.write(out,[200, {"X-Whatever" => "stuff\nbleh"}, []])
+    http_response_write(out,[200, {"X-Whatever" => "stuff\nbleh"}, []])
     assert out.closed?
     assert_match(/^X-Whatever: stuff\r\nX-Whatever: bleh\r\n/, out.string)
   end
@@ -68,7 +69,7 @@ class ResponseTest < Test::Unit::TestCase
   # some broken clients still rely on it
   def test_status_header_added
     out = StringIO.new
-    HttpResponse.write(out,[200, {"X-Whatever" => "stuff"}, []])
+    http_response_write(out,[200, {"X-Whatever" => "stuff"}, []])
     assert out.closed?
     assert_equal 1, out.string.split(/\r\n/).grep(/^Status: 200 OK/i).size
   end
@@ -79,7 +80,7 @@ class ResponseTest < Test::Unit::TestCase
   def test_status_header_ignores_app_hash
     out = StringIO.new
     header_hash = {"X-Whatever" => "stuff", 'StaTus' => "666" }
-    HttpResponse.write(out,[200, header_hash, []])
+    http_response_write(out,[200, header_hash, []])
     assert out.closed?
     assert_equal 1, out.string.split(/\r\n/).grep(/^Status: 200 OK/i).size
     assert_equal 1, out.string.split(/\r\n/).grep(/^Status:/i).size
@@ -90,7 +91,7 @@ class ResponseTest < Test::Unit::TestCase
     body = StringIO.new(expect_body)
     body.rewind
     out = StringIO.new
-    HttpResponse.write(out,[200, {}, body])
+    http_response_write(out,[200, {}, body])
     assert out.closed?
     assert body.closed?
     assert_match(expect_body, out.string.split(/\r\n/).last)
@@ -98,7 +99,7 @@ class ResponseTest < Test::Unit::TestCase
 
   def test_unknown_status_pass_through
     out = StringIO.new
-    HttpResponse.write(out,["666 I AM THE BEAST", {}, [] ])
+    http_response_write(out,["666 I AM THE BEAST", {}, [] ])
     assert out.closed?
     headers = out.string.split(/\r\n\r\n/).first.split(/\r\n/)
     assert %r{\AHTTP/\d\.\d 666 I AM THE BEAST\z}.match(headers[0])