refactor RequestParseForm and achieve greatness in API design

http/httphi/bench_test.go

@@ -112,10 +112,13 @@ func BenchmarkRequestParseForm(b *testing.B) {
 	const request = "POST /f HTTP/1.1\r\nHost: tinygo.org\r\n" +
 		"Content-Type: application/x-www-form-urlencoded\r\nContent-Length: 27\r\n\r\n" +
 		"user=gopher&msg=hello+world"
-	buf := make([]byte, 64)
+	// The form owns its memory now, so pre-size it and forbid growth: an
+	// allocation on this path is the failure the benchmark is watching for.
+	benchForm.Reset(make([]byte, 0, 64), 2)
+	benchForm.EnableBufferGrowth(false)
 	var mux MuxSlice
 	mux.Handle("POST /f", func(ex *Exchange) {
-		err := ex.RequestParseForm(&benchForm, buf)
+		err := ex.RequestParseForm(&benchForm, false, false)
 		if err != nil || benchForm.Len() != 2 {
 			panic("invalid result")
 		}

http/httphi/example_test.go

@@ -77,10 +77,12 @@ func ExampleMuxSlice_query_forms_multipart() {
 	})
 
 	mux.Handle("GET /form", func(ex *httphi.Exchange) {
-		// Request Body Form.
-		formbuf := make([]byte, 1024)
+		// Request Body Form. The form owns the memory: hand it a buffer and
+		// forbid growth to bound what a request may spend.
 		var form httpraw.Form
-		err := ex.RequestParseForm(&form, formbuf)
+		form.Reset(make([]byte, 0, 1024), 8)
+		form.EnableBufferGrowth(false)
+		err := ex.RequestParseForm(&form, false, false)
 		if err != nil {
 			ex.WriteHeader(httphi.StatusInternalServerError)
 			return

http/httphi/exchange.go

@@ -36,6 +36,9 @@ type Exchange struct {
 	respHeaderLen uint16
 	reqHdr        httpraw.Header
 	pathValues    []PathValue
+	// bodyRW is the reader handed to [httpraw.Form.ReadLimited], kept here so
+	// boxing it into an io.Reader allocates nothing per request.
+	bodyRW ExchangeRW
 
 	hijacked bool
 	rw       conn
@@ -506,83 +509,64 @@ func (exch *Exchange) RequestContentLength() (_ int64, present bool, _ error) {
 	return exch.RequestHeaderRaw().ContentLength()
 }
 
-func (exch *Exchange) RequestParseForm(dst *httpraw.Form, parseURL, prioritizeURL bool) (err error) {
-	dst.Reset(nil, 0)
-	if parseURL {
-		err = dst.ReadFromBytes(exch.RequestQuery())
-
-	}
-	if !parseURL {
-		cl, ok, err := exch.RequestContentLength()
-		if cl == 0 {
-			return nil
+// RequestParseForm parses "application/x-www-form-urlencoded" pairs into dst
+// from the request body and, when parseURL is set, from the query string as
+// well. Pairs are stored as they arrived, call [httpraw.Form.Decode] to decode
+// them in place.
+//
+// dst owns the memory: both sources are read into its buffer and parsed together
+// once. Hand it a preallocated buffer with [httpraw.Form.Reset] and turn growth
+// off with [httpraw.Form.EnableBufferGrowth] to bound it, which then reports
+// [httpraw.ErrBufferExhausted] instead of allocating. It grows by default.
+//
+// prioritizeURL reads the query ahead of the body, so a key carried by both
+// resolves to the query's value: [httpraw.Form.Get] answers with the first pair
+// holding a key. Both stay readable in wire order through [httpraw.Form.Pair].
+// The body is consumed, so call this before [Exchange.ReadBody].
+//
+// A request with no Content-Length has no body, RFC 9112 6.3, and one with no
+// Content-Type declares no encoding to parse, RFC 9110 8.3. Neither is an error,
+// a bodiless POST being legal, and the query is still parsed when asked for. A
+// Content-Type that is present and not form encoded is [errNotFormEncoded].
+func (exch *Exchange) RequestParseForm(dst *httpraw.Form, parseURL, prioritizeURL bool) error {
+	dst.Reset(nil, 0) // Reuse whatever buffer dst holds, discarding old pairs.
+	if parseURL && prioritizeURL {
+		if err := exch.readQueryForm(dst); err != nil {
+			return err
 		}
-		var rw ExchangeRW
-		exch.ReadWriter(&rw)
-		_, err = dst.ReadLimited(&rw, int(cl))
-
 	}
-
+	if err := exch.readBodyForm(dst); err != nil {
+		return err
+	}
+	if parseURL && !prioritizeURL {
+		if err := exch.readQueryForm(dst); err != nil {
+			return err
+		}
+	}
+	return dst.Parse()
 }
 
-func (exch *Exchange) readQueryForm(dst *httpraw.Form) (n int, err error) {
+// formSeparator joins two sources inside one form buffer. Shared so appending it
+// converts no literal per call.
+var formSeparator = []byte{'&'}
+
+// readQueryForm appends the request's query string to dst's buffer.
+func (exch *Exchange) readQueryForm(dst *httpraw.Form) error {
 	query := exch.RequestQuery()
 	if len(query) == 0 {
-		return 0, nil
-	} else if dst.Len() > 0 {
-		err = dst.ReadFromBytes([]byte{'&'}) // Add separator.
-		if err != nil {
-			return 0, err
-		}
+		return nil
+	} else if err := separateForm(dst); err != nil {
+		return err
 	}
-	err = dst.ReadFromBytes(query)
-	if err != nil {
-		return 0, err
-	}
-	return len(query), nil
+	return dst.ReadFromBytes(query)
 }
 
-func (exch *Exchange) readBodyForm(dst *httpraw.Form) (n int, err error) {
-	cl, _, err := exch.RequestContentLength()
-	if cl <= 0 {
-		return 0, nil
-	}
-	if dst.Len() > 0 {
-		err = dst.ReadFromBytes([]byte{'&'}) // Add separator.
-		if err != nil {
-			return 0, err
-		}
-	}
-	var rw ExchangeRW
-	exch.ReadWriter(&rw)
-	n, err = dst.ReadLimited(&rw, int(cl))
-	if err != nil {
-		return n, err
-	}
-	return n, nil
-}
-
-// RequestParseForm reads the request body into buf and parses it as
-// "application/x-www-form-urlencoded" into dst. buf is the only storage used and
-// the only limit: a body longer than buf is refused with [lneto.ErrBufferFull]
-// before a single byte is read, leaving the caller free to answer 413. Pairs are
-// left as they arrived, call [httpraw.Form.Decode] to decode them in place.
-//
-// Unlike http.Request.ParseForm the query string is not folded in, reach it with
-// [Exchange.RequestQuery] or [Exchange.RequestQueryAppend]. The body is consumed, so
-// call this before [Exchange.ReadBody].
-//
-// A request with no Content-Length has no body, RFC 9112 6.3, and a request with
-// no Content-Type declares no encoding to parse, RFC 9110 8.3. Both yield an
-// empty form and a nil error, a bodiless POST being legal. Use
-// [Exchange.RequestContentLength] and [Exchange.RequestContentType] to tell
-// either apart from a body that arrived empty. A Content-Type that is present
-// and not form encoded is [errNotFormEncoded], an absent one never is.
-func (exch *Exchange) RequestParseForm(dst *httpraw.Form, buf []byte) error {
+// readBodyForm appends the request body to dst's buffer, reading until
+// Content-Length bytes have arrived.
+func (exch *Exchange) readBodyForm(dst *httpraw.Form) error {
 	contentType := exch.RequestContentType()
 	if contentType == nil {
-		dst.Reset(nil, 0)
-		return nil // No declared encoding is no form, as no length is no body.
+		return nil // No declared encoding is no form, RFC 9110 8.3.
 	} else if !httpraw.MediaTypeIs(contentType, "application/x-www-form-urlencoded") {
 		return errNotFormEncoded
 	} else if exch.RequestHeaderRaw().GetFold("Transfer-Encoding") != nil {
@@ -590,31 +574,44 @@ func (exch *Exchange) RequestParseForm(dst *httpraw.Form, buf []byte) error {
 		// wire would parse chunk sizes as form data. httpraw does not decode them.
 		return errUnsupportedTransferCoding
 	}
-
 	length, present, err := exch.RequestContentLength()
-	if !present {
-		dst.Reset(nil, 0)
-		return nil // No length is no body, RFC 9112 6.3.
-	} else if err != nil {
+	if err != nil {
 		return err
-	} else if length > int64(len(buf)) {
-		return lneto.ErrShortBuffer // Refuse before reading, caller may answer 413.
+	} else if !present || length == 0 {
+		return nil // No length is no body, RFC 9112 6.3.
 	}
-	buf = buf[:length]
-	for read := 0; read < len(buf); {
-		n, err := exch.ReadBody(buf[read:])
+	if err = separateForm(dst); err != nil {
+		return err
+	}
+	// Reuse the exchange's own handle: a local would escape when boxed into the
+	// io.Reader [httpraw.Form.ReadLimited] takes, costing an allocation a request.
+	exch.ReadWriter(&exch.bodyRW)
+	// A single read may fall short of the limit, the body arriving a TCP segment
+	// at a time, so read until the declared length is in hand.
+	for read := 0; read < int(length); {
+		n, err := dst.ReadLimited(&exch.bodyRW, int(length)-read)
 		read += n
 		if n == 0 {
 			if err == nil {
 				err = io.ErrNoProgress
 			} else if err == io.EOF {
-				break
+				break // Peer sent less than it declared.
 			}
 			return err
+		} else if err != nil && err != io.EOF {
+			return err
 		}
 	}
-	dst.Reset(buf, 0)
-	return dst.Parse()
+	return nil
+}
+
+// separateForm appends the '&' keeping two sources from merging into one pair,
+// doing nothing while dst holds no bytes yet.
+func separateForm(dst *httpraw.Form) error {
+	if dst.BufferUsed() == 0 {
+		return nil
+	}
+	return dst.ReadFromBytes(formSeparator)
 }
 
 // RequestMultipart returns a parser prepared from the boundary parameter of the

http/httphi/exchange_test.go

@@ -824,10 +824,12 @@ func TestExchangeRequestParseForm(t *testing.T) {
 			extraHeaders:    "Transfer-Encoding: chunked\r\n",
 			wantErr:         errUnsupportedTransferCoding,
 		}, {
+			// The form bounds itself now, so an oversized body is the form
+			// refusing to grow rather than a short buffer handed in.
 			name:     "body larger than buffer",
 			formVals: []formPair{{key: "a", value: "1"}, {key: "b", value: "2"}, {key: "c", value: "3"}},
 			bufsize:  4,
-			wantErr:  lneto.ErrShortBuffer,
+			wantErr:  httpraw.ErrBufferExhausted,
 		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
@@ -867,12 +869,16 @@ func TestExchangeRequestParseForm(t *testing.T) {
 			builder.WriteString("\r\n")
 			builder.Write(body)
 
+			// The form owns the memory: bufSize bounds it here, growth off so an
+			// oversized body is reported rather than allocated for.
 			var form httpraw.Form
+			form.Reset(make([]byte, 0, bufSize), 8)
+			form.EnableBufferGrowth(false)
 			var gotErr error
 			var sm MuxSlice
 			sm.Reset(1)
 			sm.Handle("/f", func(exch *Exchange) {
-				gotErr = exch.RequestParseForm(&form, make([]byte, bufSize))
+				gotErr = exch.RequestParseForm(&form, false, false)
 				if gotErr == nil && test.callDecode {
 					gotErr = form.Decode()
 				}
@@ -915,7 +921,7 @@ func TestExchangeRequestParseFormSplit(t *testing.T) {
 	var sm MuxSlice
 	sm.Reset(1)
 	sm.Handle("/f", func(exch *Exchange) {
-		gotErr = exch.RequestParseForm(&form, make([]byte, 64))
+		gotErr = exch.RequestParseForm(&form, false, false)
 	})
 	exch := newExchange(t, conn, ExchangeConfig{RawBuf: make([]byte, 2*1024), RequestBufferLim: 1024})
 	if err := Handle(exch, &sm, nopBackoff); err != nil {
@@ -935,7 +941,7 @@ func TestExchangeRequestParseFormDecode(t *testing.T) {
 	var sm MuxSlice
 	sm.Reset(1)
 	sm.Handle("/f", func(exch *Exchange) {
-		if err := exch.RequestParseForm(&form, make([]byte, 64)); err != nil {
+		if err := exch.RequestParseForm(&form, false, false); err != nil {
 			t.Error(err)
 		} else if err = form.Decode(); err != nil {
 			t.Error(err)
@@ -1435,7 +1441,7 @@ func TestExchangeRequestContentTypeFolded(t *testing.T) {
 			sm.Reset(1)
 			sm.Handle("/f", func(exch *Exchange) {
 				gotType = string(exch.RequestContentType())
-				gotErr = exch.RequestParseForm(&form, make([]byte, 64))
+				gotErr = exch.RequestParseForm(&form, false, false)
 			})
 			serve(t, "POST /f HTTP/1.1\r\nHost: h\r\n"+name+": "+formType+"\r\nContent-Length: 3\r\n\r\na=1", &sm)
 
@@ -1464,7 +1470,7 @@ func TestExchangeRequestParseFormFoldedTransferEncoding(t *testing.T) {
 			var sm MuxSlice
 			sm.Reset(1)
 			sm.Handle("/f", func(exch *Exchange) {
-				gotErr = exch.RequestParseForm(&form, make([]byte, 64))
+				gotErr = exch.RequestParseForm(&form, false, false)
 			})
 			serve(t, "POST /f HTTP/1.1\r\nHost: h\r\nContent-Type: application/x-www-form-urlencoded\r\n"+
 				name+": chunked\r\nContent-Length: "+strconv.Itoa(len(body))+"\r\n\r\n"+body, &sm)
@@ -1576,3 +1582,95 @@ func TestExchangeRespondReportsOverflow(t *testing.T) {
 		t.Error("want the failure recorded on the exchange too")
 	}
 }
+
+// Query and body are read into one form buffer and parsed together, so both
+// sources are present at once and read order decides which value a key resolves
+// to. A key carried by both keeps both pairs, in wire order.
+func TestExchangeRequestParseFormFoldsQuery(t *testing.T) {
+	const body = "cnt=body&only=b"
+	const target = "/f?cnt=query&page=2"
+	for _, test := range []struct {
+		name                    string
+		parseURL, prioritizeURL bool
+		wantCnt                 string
+		wantPage                string
+		wantRendered            string
+	}{
+		{
+			name: "body only", parseURL: false,
+			wantCnt: "body", wantPage: "", wantRendered: "cnt=body|only=b",
+		},
+		{
+			name: "query first wins", parseURL: true, prioritizeURL: true,
+			wantCnt: "query", wantPage: "2", wantRendered: "cnt=query|page=2|cnt=body|only=b",
+		},
+		{
+			name: "body first wins", parseURL: true, prioritizeURL: false,
+			wantCnt: "body", wantPage: "2", wantRendered: "cnt=body|only=b|cnt=query|page=2",
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			var form httpraw.Form
+			var gotErr error
+			var sm MuxSlice
+			sm.Reset(1)
+			sm.Handle("POST /f", func(exch *Exchange) {
+				gotErr = exch.RequestParseForm(&form, test.parseURL, test.prioritizeURL)
+			})
+			serve(t, "POST "+target+" HTTP/1.1\r\nHost: h\r\n"+
+				"Content-Type: application/x-www-form-urlencoded\r\n"+
+				"Content-Length: "+strconv.Itoa(len(body))+"\r\n\r\n"+body, &sm)
+			if gotErr != nil {
+				t.Fatalf("RequestParseForm: %s", gotErr)
+			}
+			if got := string(form.Get("cnt")); got != test.wantCnt {
+				t.Errorf("want cnt=%q, got %q", test.wantCnt, got)
+			}
+			if got := string(form.Get("page")); got != test.wantPage {
+				t.Errorf("want page=%q, got %q", test.wantPage, got)
+			}
+			if got := formString(&form); got != test.wantRendered {
+				t.Errorf("want pairs %q, got %q", test.wantRendered, got)
+			}
+		})
+	}
+}
+
+// A GET with a query and no body must fold the query alone: no Content-Type
+// means no body to parse, which is not an error.
+func TestExchangeRequestParseFormQueryWithoutBody(t *testing.T) {
+	var form httpraw.Form
+	var gotErr error
+	var sm MuxSlice
+	sm.Reset(1)
+	sm.Handle("GET /f", func(exch *Exchange) {
+		gotErr = exch.RequestParseForm(&form, true, true)
+	})
+	serve(t, "GET /f?a=1&b=2 HTTP/1.1\r\nHost: h\r\n\r\n", &sm)
+	if gotErr != nil {
+		t.Fatalf("want the query parsed with no body, got %s", gotErr)
+	}
+	if got := formString(&form); got != "a=1|b=2" {
+		t.Errorf("want a=1|b=2, got %q", got)
+	}
+}
+
+// The separator must not merge the two sources into one pair: without it the
+// last query pair and the first body pair run together.
+func TestExchangeRequestParseFormSourcesNotMerged(t *testing.T) {
+	var form httpraw.Form
+	var sm MuxSlice
+	sm.Reset(1)
+	sm.Handle("POST /f", func(exch *Exchange) {
+		if err := exch.RequestParseForm(&form, true, true); err != nil {
+			t.Fatal(err)
+		}
+	})
+	const body = "second=2"
+	serve(t, "POST /f?first=1 HTTP/1.1\r\nHost: h\r\n"+
+		"Content-Type: application/x-www-form-urlencoded\r\n"+
+		"Content-Length: "+strconv.Itoa(len(body))+"\r\n\r\n"+body, &sm)
+	if got := formString(&form); got != "first=1|second=2" {
+		t.Errorf("want first=1|second=2, got %q", got)
+	}
+}

http/httphi/httphi_fuzz_test.go

@@ -251,7 +251,7 @@ func FuzzQueryAndForm(f *testing.F) {
 
 			var form httpraw.Form
 			buf := make([]byte, scratchLen)
-			if err := exch.RequestParseForm(&form, buf); err != nil {
+			if err := exch.RequestParseForm(&form, false, false); err != nil {
 				return
 			}
 			total := 0