Introduce the request.body stream. Lazy-read to parse parameters rather than always setting RAW_POST_DATA. Reduces the memory footprint of large binary PUT requests.

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@6740 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

actionpack/CHANGELOG

@@ -1,5 +1,7 @@
 *SVN*
 
+* Introduce the request.body stream. Lazy-read to parse parameters rather than always setting RAW_POST_DATA. Reduces the memory footprint of large binary PUT requests.  [Jeremy Kemper]
+
 * Add some performance enhancements to ActionView.
 
   * Cache base_paths in @@cached_base_paths

actionpack/lib/action_controller/cgi_ext.rb

@@ -1,8 +1,18 @@
+require 'action_controller/cgi_ext/stdinput'
 require 'action_controller/cgi_ext/parameters'
 require 'action_controller/cgi_ext/query_extension'
 require 'action_controller/cgi_ext/cookie'
 require 'action_controller/cgi_ext/session'
 
 class CGI #:nodoc:
+  include ActionController::CgiExt::Stdinput
   include ActionController::CgiExt::Parameters
+
+  class << self
+    alias :escapeHTML_fail_on_nil :escapeHTML
+
+    def escapeHTML(string)
+      escapeHTML_fail_on_nil(string) unless string.nil?
+    end
+  end
 end

actionpack/lib/action_controller/cgi_ext/parameters.rb

@@ -1,16 +1,6 @@
 require 'cgi'
 require 'strscan'
 
-class CGI #:nodoc:
-  class << self
-    alias :escapeHTML_fail_on_nil :escapeHTML
-
-    def escapeHTML(string)
-      escapeHTML_fail_on_nil(string) unless string.nil?
-    end
-  end
-end
-
 module ActionController
   module CgiExt
     module Parameters
@@ -72,18 +62,18 @@ module ActionController
           parser.result
         end
 
-        def parse_formatted_request_parameters(mime_type, raw_post_data)
+        def parse_formatted_request_parameters(mime_type, body)
           case strategy = ActionController::Base.param_parsers[mime_type]
             when Proc
-              strategy.call(raw_post_data)
+              strategy.call(body)
             when :xml_simple, :xml_node
-              raw_post_data.blank? ? {} : Hash.from_xml(raw_post_data).with_indifferent_access
+              body.blank? ? {} : Hash.from_xml(body).with_indifferent_access
             when :yaml
-              YAML.load(raw_post_data)
+              YAML.load(body)
           end
         rescue Exception => e # YAML, XML or Ruby code block errors
           { "exception" => "#{e.message} (#{e.class})", "backtrace" => e.backtrace,
-            "raw_post_data" => raw_post_data, "format" => mime_type }
+            "body" => body, "format" => mime_type }
         end
 
         private

actionpack/lib/action_controller/cgi_ext/query_extension.rb

@@ -31,18 +31,14 @@ class CGI #:nodoc:
       # Set multipart to false by default.
       @multipart = false
 
-      # POST and PUT may have params in entity body. If content type is
-      # missing for POST, assume urlencoded. If content type is missing
-      # for PUT, don't assume anything and don't parse the parameters:
-      # it's likely binary data.
-      #
-      # The other HTTP methods have their params in the query string.
+      # POST and PUT may have params in entity body. If content type is missing
+      # or non-urlencoded, don't read the body or parse parameters: assume it's
+      # binary data.
       if method == :post || method == :put
         if boundary = extract_multipart_form_boundary(content_type)
           @multipart = true
           @params = read_multipart(boundary, content_length)
         elsif content_type.blank? || content_type !~ %r{application/x-www-form-urlencoded}i
-          read_params(method, content_length)
           @params = {}
         end
       end

actionpack/lib/action_controller/cgi_ext/stdinput.rb

@@ -0,0 +1,23 @@
+require 'cgi'
+
+module ActionController
+  module CgiExt
+    # Publicize the CGI's internal input stream so we can lazy-read
+    # request.body. Make it writable so we don't have to play $stdin games.
+    module Stdinput
+      def self.included(base)
+        base.class_eval do
+          remove_method :stdinput
+          attr_accessor :stdinput
+        end
+
+        base.alias_method_chain :initialize, :stdinput
+      end
+
+      def initialize_with_stdinput(type = nil, stdinput = $stdin)
+        @stdinput = stdinput
+        initialize_without_stdinput(type || 'query')
+      end
+    end
+  end
+end

actionpack/lib/action_controller/cgi_process.rb

@@ -58,6 +58,16 @@ module ActionController #:nodoc:
       end
     end
 
+    # The request body is an IO input stream. If the RAW_POST_DATA environment
+    # variable is already set, wrap it in a StringIO.
+    def body
+      if raw_post = env['RAW_POST_DATA']
+        StringIO.new(raw_post)
+      else
+        @cgi.stdinput
+      end
+    end
+
     def query_parameters
       @query_parameters ||=
         (qs = self.query_string).empty? ? {} : CGI.parse_query_parameters(qs)
@@ -66,7 +76,7 @@ module ActionController #:nodoc:
     def request_parameters
       @request_parameters ||=
         if ActionController::Base.param_parsers.has_key?(content_type)
-          CGI.parse_formatted_request_parameters(content_type, @env['RAW_POST_DATA'])
+          CGI.parse_formatted_request_parameters(content_type, body.read)
         else
           CGI.parse_request_parameters(@cgi.params)
         end

actionpack/lib/action_controller/request.rb

@@ -1,23 +1,26 @@
 module ActionController
-  # Subclassing AbstractRequest makes these methods available to the request objects used in production and testing,
-  # CgiRequest and TestRequest
+  # CgiRequest and TestRequest provide concrete implementations.
   class AbstractRequest
     cattr_accessor :relative_url_root
     remove_method :relative_url_root
 
-    # Returns the hash of environment variables for this request,
+    # The hash of environment variables for this request,
     # such as { 'RAILS_ENV' => 'production' }.
     attr_reader :env
 
-    attr_accessor :format
+    # The requested content type, such as :html or :xml.
+    attr_writer :format
 
-    # Returns the HTTP request method as a lowercase symbol (:get, for example). Note, HEAD is returned as :get
-    # since the two are supposedly to be functionaly equivilent for all purposes except that HEAD won't return a response
-    # body (which Rails also takes care of elsewhere).
+    # The HTTP request method as a lowercase symbol, such as :get.
+    # Note, HEAD is returned as :get since the two are functionally
+    # equivalent from the application's perspective.
     def method
-      @request_method ||= (!parameters[:_method].blank? && @env['REQUEST_METHOD'] == 'POST') ?
-        parameters[:_method].to_s.downcase.to_sym :
-        @env['REQUEST_METHOD'].downcase.to_sym
+      @request_method ||=
+        if @env['REQUEST_METHOD'] == 'POST' && !parameters[:_method].blank?
+          parameters[:_method].to_s.downcase.to_sym
+        else
+          @env['REQUEST_METHOD'].downcase.to_sym
+        end
 
       @request_method == :head ? :get : @request_method
     end
@@ -42,8 +45,8 @@ module ActionController
       method == :delete
     end
 
-    # Is this a HEAD request?  HEAD is mapped as :get for request.method, so here we ask the 
-    # REQUEST_METHOD header directly. Thus, for head, both get? and head? will return true.
+    # Is this a HEAD request? request.method sees HEAD as :get, so check the
+    # HTTP method directly.
     def head?
       @env['REQUEST_METHOD'].downcase.to_sym == :head
     end
@@ -269,6 +272,11 @@ module ActionController
     #--
     # Must be implemented in the concrete request
     #++
+
+    # The request body is an IO input stream.
+    def body
+    end
+
     def query_parameters #:nodoc:
     end
 

actionpack/lib/action_controller/test_process.rb

@@ -40,18 +40,15 @@ module ActionController #:nodoc:
       @session = TestSession.new
     end
 
-    def raw_post
-      if raw_post = env['RAW_POST_DATA']
-        raw_post
-      else
-        params = self.request_parameters.dup
-        %w(controller action only_path).each do |k|
-          params.delete(k)
-          params.delete(k.to_sym)
-        end
+    # Wraps raw_post in a StringIO.
+    def body
+      StringIO.new(raw_post)
+    end
 
-        params.map { |k,v| [ CGI.escape(k.to_s), CGI.escape(v.to_s) ].join('=') }.sort.join('&')
-      end
+    # Either the RAW_POST_DATA environment variable or the URL-encoded request
+    # parameters.
+    def raw_post
+      env['RAW_POST_DATA'] ||= url_encoded_request_parameters
     end
 
     def port=(number)
@@ -140,6 +137,17 @@ module ActionController #:nodoc:
         @env["SERVER_PORT"]      = 80
         @env['REQUEST_METHOD']   = "GET"
       end
+
+      def url_encoded_request_parameters
+        params = self.request_parameters.dup
+
+        %w(controller action only_path).each do |k|
+          params.delete(k)
+          params.delete(k.to_sym)
+        end
+
+        params.to_query
+      end
   end
 
   # A refactoring of TestResponse to allow the same behavior to be applied

actionpack/test/abstract_unit.rb

@@ -3,8 +3,10 @@ $:.unshift(File.dirname(__FILE__) + '/../../activesupport/lib/active_support')
 $:.unshift(File.dirname(__FILE__) + '/fixtures/helpers')
 
 require 'yaml'
+require 'stringio'
 require 'test/unit'
 require 'action_controller'
+require 'action_controller/cgi_ext'
 require 'action_controller/test_process'
 
 # Show backtraces for deprecated behavior for quicker cleanup.

actionpack/test/controller/raw_post_test.rb

@@ -1,6 +1,4 @@
 require "#{File.dirname(__FILE__)}/../abstract_unit"
-require 'stringio'
-require 'action_controller/cgi_ext/query_extension'
 
 class RawPostDataTest < Test::Unit::TestCase
   def setup
@@ -11,57 +9,66 @@ class RawPostDataTest < Test::Unit::TestCase
   def test_post_with_urlencoded_body
     ENV['REQUEST_METHOD'] = 'POST'
     ENV['CONTENT_TYPE'] = ' apPlication/x-Www-form-urlEncoded; charset=utf-8'
-    assert_equal ['1'], cgi_params['a']
-    assert_has_raw_post_data
+    assert_equal ['1'], cgi.params['a']
+    assert_raw_post_data
   end
 
   def test_post_with_empty_content_type_treated_as_urlencoded
     ENV['REQUEST_METHOD'] = 'POST'
     ENV['CONTENT_TYPE'] = ''
-    assert_equal ['1'], cgi_params['a']
-    assert_has_raw_post_data
+    assert_equal ['1'], cgi.params['a']
+    assert_raw_post_data
   end
 
-  def test_post_with_unrecognized_content_type_reads_body_but_doesnt_parse_params
+  def test_post_with_unrecognized_content_type_ignores_body
     ENV['REQUEST_METHOD'] = 'POST'
     ENV['CONTENT_TYPE'] = 'foo/bar'
-    assert cgi_params.empty?
-    assert_has_raw_post_data
+    assert cgi.params.empty?
+    assert_no_raw_post_data
   end
 
   def test_put_with_urlencoded_body
     ENV['REQUEST_METHOD'] = 'PUT'
     ENV['CONTENT_TYPE'] = 'application/x-www-form-urlencoded'
-    assert_equal ['1'], cgi_params['a']
-    assert_has_raw_post_data
+    assert_equal ['1'], cgi.params['a']
+    assert_raw_post_data
   end
 
   def test_put_with_empty_content_type_ignores_body
     ENV['REQUEST_METHOD'] = 'PUT'
     ENV['CONTENT_TYPE'] = ''
-    assert cgi_params.empty?
-    assert_has_raw_post_data
+    assert cgi.params.empty?
+    assert_no_raw_post_data
   end
 
   def test_put_with_unrecognized_content_type_ignores_body
     ENV['REQUEST_METHOD'] = 'PUT'
     ENV['CONTENT_TYPE'] = 'foo/bar'
-    assert cgi_params.empty?
-    assert_has_raw_post_data
+    assert cgi.params.empty?
+    assert_no_raw_post_data
   end
 
   private
-    def cgi_params
-      old_stdin, $stdin = $stdin, StringIO.new(@request_body.dup)
-      ENV['CONTENT_LENGTH'] = $stdin.size.to_s
-      CGI.new.params
-    ensure
-      $stdin = old_stdin
+    def cgi
+      unless defined? @cgi
+        ENV['CONTENT_LENGTH'] = @request_body.size.to_s
+        @cgi = CGI.new('query', StringIO.new(@request_body.dup))
+      end
+
+      @cgi
     end
 
-    def assert_has_raw_post_data(expected_body = @request_body)
+    def assert_raw_post_data
       assert_not_nil ENV['RAW_POST_DATA']
       assert ENV['RAW_POST_DATA'].frozen?
-      assert_equal expected_body, ENV['RAW_POST_DATA']
+      assert_equal @request_body, ENV['RAW_POST_DATA']
+
+      assert_equal '', cgi.stdinput.read
+    end
+
+    def assert_no_raw_post_data
+      assert_nil ENV['RAW_POST_DATA']
+
+      assert_equal @request_body, cgi.stdinput.read
     end
 end

actionpack/test/controller/test_test.rb

@@ -13,6 +13,10 @@ class TestTest < Test::Unit::TestCase
       render :text => request.raw_post
     end
 
+    def render_body
+      render :text => request.body.read
+    end
+
     def test_params
       render :text => params.inspect
     end
@@ -93,8 +97,15 @@ HTML
     params = {:page => {:name => 'page name'}, 'some key' => 123}
     get :render_raw_post, params.dup
 
-    raw_post = params.map {|k,v| [CGI::escape(k.to_s), CGI::escape(v.to_s)].join('=')}.sort.join('&')
-    assert_equal raw_post, @response.body
+    assert_equal params.to_query, @response.body
+  end
+
+  def test_body_stream
+    params = { :page => { :name => 'page name' }, 'some key' => 123 }
+
+    get :render_body, params.dup
+
+    assert_equal params.to_query, @response.body
   end
 
   def test_process_without_flash

actionpack/test/controller/webservice_test.rb

@@ -1,20 +1,16 @@
 require File.dirname(__FILE__) + '/../abstract_unit'
-require 'stringio'
 
 class WebServiceTest < Test::Unit::TestCase
-
   class MockCGI < CGI #:nodoc:
-    attr_accessor :stdinput, :stdoutput, :env_table
+    attr_accessor :stdoutput, :env_table
 
     def initialize(env, data = '')
       self.env_table = env
-      self.stdinput = StringIO.new(data)
       self.stdoutput = StringIO.new
-      super()
+      super(nil, StringIO.new(data))
     end
   end
 
-
   class TestController < ActionController::Base
     session :off