From 06bf73975864b8e16ef1ee977f8424a0e5517fd6 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Wed, 1 Jul 2009 13:59:40 -0700 Subject: Force streaming input onto apps by default This change gives applications full control to deny clients from uploading unwanted message bodies. This also paves the way for doing things like upload progress notification within applications in a Rack::Lint-compatible manner. Since we don't support HTTP keepalive, so we have more freedom here by being able to close TCP connections and deny clients the ability to write to us (and thus wasting our bandwidth). While I could've left this feature off by default indefinitely for maximum backwards compatibility (for arguably broken applications), Unicorn is not and has never been about supporting the lowest common denominator. --- TODO | 4 ---- examples/echo.ru | 1 - lib/unicorn/app/inetd.rb | 2 -- lib/unicorn/configurator.rb | 27 --------------------------- lib/unicorn/const.rb | 1 - lib/unicorn/http_request.rb | 3 +-- lib/unicorn/tee_input.rb | 18 ++++++++---------- test/unit/test_configurator.rb | 2 -- test/unit/test_upload.rb | 19 +++++++++---------- 9 files changed, 18 insertions(+), 59 deletions(-) diff --git a/TODO b/TODO index 7e36cc2..df0d06e 100644 --- a/TODO +++ b/TODO @@ -1,7 +1,3 @@ -* Support HTTP/1.1 keepalive if (and probably only if) pipelining. - We can do this by testing readability of socket immediately after - the response is written. - * integration tests with nginx including bad client handling * manpages (why do so few Ruby executables come with proper manpages?) diff --git a/examples/echo.ru b/examples/echo.ru index e13721a..ebc4d9d 100644 --- a/examples/echo.ru +++ b/examples/echo.ru @@ -7,7 +7,6 @@ # # Then type random stuff in your terminal to watch it get echoed back! -Unicorn::HttpRequest::DEFAULTS["unicorn.stream_input"] = true class EchoBody def initialize(input) @input = input diff --git a/lib/unicorn/app/inetd.rb b/lib/unicorn/app/inetd.rb index c3b8bbc..e22b308 100644 --- a/lib/unicorn/app/inetd.rb +++ b/lib/unicorn/app/inetd.rb @@ -91,8 +91,6 @@ module Unicorn::App end def initialize(*cmd) - # enable streaming input mode in Unicorn - Unicorn::HttpRequest::DEFAULTS["unicorn.stream_input"] = true @cmd = cmd end diff --git a/lib/unicorn/configurator.rb b/lib/unicorn/configurator.rb index 1f18515..bd0a198 100644 --- a/lib/unicorn/configurator.rb +++ b/lib/unicorn/configurator.rb @@ -44,7 +44,6 @@ module Unicorn :preload_app => false, :stderr_path => nil, :stdout_path => nil, - :stream_input => false, } attr_reader :config_file #:nodoc: @@ -65,10 +64,6 @@ module Unicorn def commit!(server, options = {}) #:nodoc: skip = options[:skip] || [] - stream_input = @set.delete(:stream_input) - unless stream_input.nil? - Unicorn::HttpRequest::DEFAULTS[Const::STREAM_INPUT] = stream_input - end @set.each do |key, value| value == :unset and next skip.include?(key) and next @@ -276,28 +271,6 @@ module Unicorn end end - # Allow applications to stream input as it is being read from the - # network directly to the application. Enabling this can allow - # real-time processing of request bodies as it is being sent by - # the client, useful for things like upload progress notification - # and tunneling arbitrary stream protocols via bidirectional chunked - # transfer encoding. - # This may not work with all applications because some broken - # applications assume env['rack.input'].read(size) always returns - # the requested amount of data. This causes env['rack.input']#read - # to provide IO#readpartial semantics instead. Some applications - # may also fully receive an input and never attempt to process it, - # causing clients confusion when they receive a response after - # only a partial request has been sent. - def stream_input(bool) - case bool - when TrueClass, FalseClass - @set[:stream_input] = bool - else - raise ArgumentError, "stream_input=#{bool.inspect} not a boolean" - end - end - # Allow redirecting $stderr to a given path. Unlike doing this from # the shell, this allows the unicorn process to know the path its # writing to and rotate the file if it is used for logging. The diff --git a/lib/unicorn/const.rb b/lib/unicorn/const.rb index be69753..be23bb4 100644 --- a/lib/unicorn/const.rb +++ b/lib/unicorn/const.rb @@ -34,7 +34,6 @@ module Unicorn HTTP_EXPECT="HTTP_EXPECT".freeze HTTP_TRAILER="HTTP_TRAILER".freeze RACK_INPUT="rack.input".freeze - STREAM_INPUT="unicorn.stream_input".freeze end end diff --git a/lib/unicorn/http_request.rb b/lib/unicorn/http_request.rb index ad1e23f..779cd32 100644 --- a/lib/unicorn/http_request.rb +++ b/lib/unicorn/http_request.rb @@ -98,8 +98,7 @@ module Unicorn end end - inp = TeeInput.new(socket, length, body) - DEFAULTS[Const::STREAM_INPUT] ? inp : inp.consume + TeeInput.new(socket, length, body) else NULL_IO.closed? ? NULL_IO.reopen(Z) : NULL_IO end diff --git a/lib/unicorn/tee_input.rb b/lib/unicorn/tee_input.rb index 8dd8954..06028a6 100644 --- a/lib/unicorn/tee_input.rb +++ b/lib/unicorn/tee_input.rb @@ -29,22 +29,20 @@ module Unicorn @size = size # nil if chunked end - def consume - @input or return - buf = Z.dup - while tee(Const::CHUNK_SIZE, buf) - end - @tmp.rewind - self - end - # returns the size of the input. This is what the Content-Length # header value should be, and how large our input is expected to be. # For TE:chunked, this requires consuming all of the input stream # before returning since there's no other way def size @size and return @size - @input and consume + + if @input + buf = Z.dup + while tee(Const::CHUNK_SIZE, buf) + end + @tmp.rewind + end + @size = @tmp.stat.size end diff --git a/test/unit/test_configurator.rb b/test/unit/test_configurator.rb index f836647..bcdd2f5 100644 --- a/test/unit/test_configurator.rb +++ b/test/unit/test_configurator.rb @@ -53,7 +53,6 @@ class TestConfigurator < Test::Unit::TestCase cfg = Unicorn::Configurator.new(:use_defaults => true) assert_nothing_raised { cfg.commit!(self) } Unicorn::Configurator::DEFAULTS.each do |key,value| - next if key == :stream_input assert_equal value, instance_variable_get("@#{key.to_s}") end end @@ -65,7 +64,6 @@ class TestConfigurator < Test::Unit::TestCase @logger = nil Unicorn::Configurator::DEFAULTS.each do |key,value| next if skip.include?(key) - next if key == :stream_input assert_equal value, instance_variable_get("@#{key.to_s}") end assert_nil @logger diff --git a/test/unit/test_upload.rb b/test/unit/test_upload.rb index 37baa30..dad5825 100644 --- a/test/unit/test_upload.rb +++ b/test/unit/test_upload.rb @@ -142,20 +142,19 @@ class UploadTest < Test::Unit::TestCase end def test_put_excessive_overwrite_closed - start_server(lambda { |env| [ 200, @hdr, [] ] }) + start_server(lambda { |env| + while env['rack.input'].read(65536); end + [ 200, @hdr, [] ] + }) sock = TCPSocket.new(@addr, @port) buf = ' ' * @bs sock.syswrite("PUT / HTTP/1.0\r\nContent-Length: #{length}\r\n\r\n") - if Unicorn::HttpRequest::DEFAULTS['unicorn.stream_input'] - assert_raise(Errno::ECONNRESET, Errno::EPIPE) do - @count.times { sock.syswrite(buf) } - end - else - @count.times { sock.syswrite(buf) } - assert_raise(Errno::ECONNRESET, Errno::EPIPE) do - ::Unicorn::Const::CHUNK_SIZE.times { sock.syswrite(buf) } - end + + @count.times { sock.syswrite(buf) } + assert_raise(Errno::ECONNRESET, Errno::EPIPE) do + ::Unicorn::Const::CHUNK_SIZE.times { sock.syswrite(buf) } end + assert_equal "HTTP/1.1 200 OK\r\n", sock.gets end # Despite reading numerous articles and inspecting the 1.9.1-p0 C -- cgit v1.2.3-24-ge0c7