From: Joe Tsai Date: Wed, 5 Oct 2016 21:23:56 +0000 (+0000) Subject: Revert "net/http: improve performance for parsePostForm" X-Git-Tag: go1.8beta1~1010 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=f6b4c88941f88380caf59a050c1e9805664aa2fa;p=gostls13.git 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 TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick --- diff --git a/src/net/http/request.go b/src/net/http/request.go index 82a918c22e..c29af7fbe5 100644 --- 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 a6c90d09a4..a4c88c0291 100644 --- 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