Revert "net/http: improve performance for parsePostForm"

This reverts commit 59320c396e6448132a52cb5a5d96491eee1e0ad8.

Reasons:
This CL was causing failures on a large regression test that we run
within Google. The issues arises from two bugs in the CL:
* The CL dropped support for ';' as a delimiter (see https://golang.org/issue/2210)
* The handling of an empty string caused an empty record to be added when
no record was added (see https://golang.org/cl/30454 for my attempted fix)

The logic being added is essentially a variation of url.ParseQuery,
but altered to accept an io.Reader instead of a string.
Since it is duplicated (but modified) logic, there needs to be good
tests to ensure that it's implementation doesn't drift in functionality
from url.ParseQuery. Fixing the above issues and adding the associated
regression tests leads to >100 lines of codes.
For a 4% reduction in CPU time, I think this complexity and duplicated
logic is not worth the effort.

As such, I am abandoning my efforts to fix the existing issues and
believe that reverting CL/20301 is the better course of action.

Updates #14655

Change-Id: Ibb5be0a5b48a16c46337e213b79467fcafee69df
Reviewed-on: https://go-review.googlesource.com/30470
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

diff --git a/src/net/http/request.go b/src/net/http/request.go
index 82a918c22e5fef0bd30d7d2bfb69c975f7341dc2..c29af7fbe52fa3adbf53327d89d6f8122c7caf10 100644 (file)
--- a/src/net/http/request.go
+++ b/src/net/http/request.go
@@ -1015,8 +1015,18 @@ func parsePostForm(r *Request) (vs url.Values, err error) {
                        maxFormSize = int64(10 << 20) // 10 MB is a lot of text.
                        reader = io.LimitReader(r.Body, maxFormSize+1)
                }
-               vs = make(url.Values)
-               e := parsePostFormURLEncoded(vs, reader, maxFormSize)
+               b, e := ioutil.ReadAll(reader)
+               if e != nil {
+                       if err == nil {
+                               err = e
+                       }
+                       break
+               }
+               if int64(len(b)) > maxFormSize {
+                       err = errors.New("http: POST too large")
+                       return
+               }
+               vs, e = url.ParseQuery(string(b))
                if err == nil {
                        err = e
                }
@@ -1031,52 +1041,6 @@ func parsePostForm(r *Request) (vs url.Values, err error) {
        return
 }
 
-// parsePostFormURLEncoded reads from r, the reader of a POST form to populate vs which is a url-type values.
-// maxFormSize indicates the maximum number of bytes that will be read from r.
-func parsePostFormURLEncoded(vs url.Values, r io.Reader, maxFormSize int64) error {
-       br := newBufioReader(r)
-       defer putBufioReader(br)
-
-       var readSize int64
-       for {
-               // Read next "key=value&" or "justkey&".
-               // If this is the last pair, b will contain just "key=value" or "justkey".
-               b, err := br.ReadBytes('&')
-               if err != nil && err != io.EOF && err != bufio.ErrBufferFull {
-                       return err
-               }
-               isEOF := err == io.EOF
-               readSize += int64(len(b))
-               if readSize >= maxFormSize {
-                       return errors.New("http: POST too large")
-               }
-
-               // Remove last delimiter
-               if len(b) > 0 && b[len(b)-1] == '&' {
-                       b = b[:len(b)-1]
-               }
-
-               // Parse key and value
-               k := string(b)
-               var v string
-               if i := strings.Index(k, "="); i > -1 {
-                       k, v = k[:i], k[i+1:]
-               }
-               if k, err = url.QueryUnescape(k); err != nil {
-                       return err
-               }
-               if v, err = url.QueryUnescape(v); err != nil {
-                       return err
-               }
-
-               // Populate vs
-               vs[k] = append(vs[k], v)
-               if isEOF {
-                       return nil
-               }
-       }
-}
-
 // ParseForm parses the raw query from the URL and updates r.Form.
 //
 // For POST or PUT requests, it also parses the request body as a form and

diff --git a/src/net/http/request_test.go b/src/net/http/request_test.go
index a6c90d09a423e9c786f61457dc7bfdf124ae1c49..a4c88c02915ccdf3ada229bda2b6524b1f1623c0 100644 (file)
--- a/src/net/http/request_test.go
+++ b/src/net/http/request_test.go
@@ -30,8 +30,8 @@ func TestQuery(t *testing.T) {
 }
 
 func TestPostQuery(t *testing.T) {
-       req, _ := NewRequest("POST", "http://www.google.com/search?q=foo&q=bar&both=x&prio=1&empty=not&orphan=nope",
-               strings.NewReader("z=post&both=y&prio=2&orphan&empty="))
+       req, _ := NewRequest("POST", "http://www.google.com/search?q=foo&q=bar&both=x&prio=1&empty=not",
+               strings.NewReader("z=post&both=y&prio=2&empty="))
        req.Header.Set("Content-Type", "application/x-www-form-urlencoded; param=value")
 
        if q := req.FormValue("q"); q != "foo" {
@@ -58,26 +58,11 @@ func TestPostQuery(t *testing.T) {
        if empty := req.FormValue("empty"); empty != "" {
                t.Errorf(`req.FormValue("empty") = %q, want "" (from body)`, empty)
        }
-       if orphan := req.FormValue("orphan"); orphan != "" {
-               t.Errorf(`req.FormValue("orphan") = %q, want "" (from body)`, orphan)
-       }
-}
-
-func BenchmarkPostQuery(b *testing.B) {
-       req, _ := NewRequest("POST", "http://www.google.com/search?q=foo&q=bar&both=x&prio=1&empty=not&orphan=nope",
-               strings.NewReader("z=post&both=y&prio=2&orphan&empty="))
-       req.Header.Set("Content-Type", "application/x-www-form-urlencoded; param=value")
-       b.ReportAllocs()
-       b.ResetTimer()
-       for i := 0; i < b.N; i++ {
-               req.PostForm = nil
-               req.ParseForm()
-       }
 }
 
 func TestPatchQuery(t *testing.T) {
-       req, _ := NewRequest("PATCH", "http://www.google.com/search?q=foo&q=bar&both=x&prio=1&empty=not&orphan=nope",
-               strings.NewReader("z=post&both=y&prio=2&orphan&empty="))
+       req, _ := NewRequest("PATCH", "http://www.google.com/search?q=foo&q=bar&both=x&prio=1&empty=not",
+               strings.NewReader("z=post&both=y&prio=2&empty="))
        req.Header.Set("Content-Type", "application/x-www-form-urlencoded; param=value")
 
        if q := req.FormValue("q"); q != "foo" {
@@ -104,9 +89,6 @@ func TestPatchQuery(t *testing.T) {
        if empty := req.FormValue("empty"); empty != "" {
                t.Errorf(`req.FormValue("empty") = %q, want "" (from body)`, empty)
        }
-       if orphan := req.FormValue("orphan"); orphan != "" {
-               t.Errorf(`req.FormValue("orphan") = %q, want "" (from body)`, orphan)
-       }
 }
 
 type stringMap map[string][]string