Revert "net/http: improve performance for parsePostForm"

This reverts commit 59320c3.

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 <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>

Author: dsnet

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

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