From 50c11036dd4898ccfed8b3e0552e88c67b6c63a9 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Fri, 27 Aug 2010 20:29:55 +0000 Subject: http_response: avoid singleton method 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. --- lib/unicorn.rb | 11 ++++++---- lib/unicorn/http_response.rb | 50 +++++++++++++------------------------------- test/unit/test_response.rb | 23 ++++++++++---------- 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]) -- cgit v1.2.3-24-ge0c7