net/http: data race in HTTP/2 body close

[neild]

This is pulling #60041 (comment) out into a separate issue.

Reproduction case:

package tmpgbudljeu3i_test

import (
	"bufio"
	"bytes"
	"fmt"
	"io"
	"net/http"
	"net/http/httptest"
	"testing"
)

func writeChunk(w io.Writer, b []byte) {
	fmt.Fprintf(w, "%x\r\n", len(b))
	w.Write(b)
	fmt.Fprint(w, "\r\n")
}

func startRequestGenerator(url string) io.ReadCloser {
	r, w := io.Pipe()
	chunk := bytes.Repeat([]byte("A"), 64)
	go func() {
		for {
			_, err := fmt.Fprintf(w, "POST %s HTTP/1.1\r\n", url)
			if err != nil {
				return
			}
			fmt.Fprint(w, "Host: localhost\r\n")
			fmt.Fprint(w, "Transfer-Encoding: chunked\r\n")
			fmt.Fprint(w, "\r\n")
			writeChunk(w, chunk)
			writeChunk(w, nil)
		}
	}()

	return r
}

func TestHTTPRace(t *testing.T) {
	svr := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		// cause cs.abortRequestBodyWrite()
		// https://github.com/golang/go/blob/go1.20.6/src/net/http/h2_bundle.go#L8205-L8216
		w.WriteHeader(http.StatusBadRequest)
	}))
	svr.EnableHTTP2 = true
	svr.StartTLS()

	r := startRequestGenerator("https://" + svr.Listener.Addr().String() + "/")
	defer r.Close()
	b := bufio.NewReader(r)
	for i := 0; i < 3; i++ {
		req, err := http.ReadRequest(b)
		if err != nil {
			t.Fatal(err)
		}
		req.RequestURI = ""
		_, err = svr.Client().Transport.RoundTrip(req)
		if err != nil {
			t.Log(err)
		}
	}
}

[neild]

This turns out to not be a bug.

The test sends three requests. Each request is created with http.ReadRequest on the same io.Reader, so each request shares the same reader for its request body.

Closing the request body in the request returned by ReadRequest consumes the remaining body. This isn't documented, but it's probably behavior you can count on.

RoundTrip closes the request body. However, the docs state:

	// RoundTrip must always close the body, including on errors,
	// but depending on the implementation may do so in a separate
	// goroutine even after RoundTrip returns. This means that
	// callers wanting to reuse the body for subsequent requests
	// must arrange to wait for the Close call before doing so.

So what is happening is a request is sent, RoundTrip returns prior to consuming the body (because the server rejected the request without reading the body), and RoundTrip closing the body in the background then races with the next request being read.

If I insert a req.Body.Close at the end of the loop in the test, the race goes away. (The body is then double-closed by RoundTrip and the test, but it handles that fine.)

[WGH-]

Thank you for the analysis! Totally missed that note, my bad.