net/http: request not rewound in certain scenario if HTTP/2 stack is used

[michaljemala]

What version of Go are you using (go version)?

go version go1.17.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/mije/Library/Caches/go-build"
GOENV="/Users/mije/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/mije/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/mije/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.17.2/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.17.2/libexec/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.17.2"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/k5/46gthz455w7bfkrqskj1p0f80000gn/T/go-build2644659669=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Our implementation is retrying http request in certain scenarios. When this happens we received a 404 response indicating no body has been sent. We fixed this issue by ensuring the request body reader is rewound upon each retry.

However, when trying to replicate the issue we found out a discrepancy between HTTP/1.1 and HTTP/2 stacks. the following unit test demonstrates the problem

func TestClient_RetrySameRequest(t *testing.T) {
	tt := []struct {
		HTTP2Enabled bool
	}{
		{HTTP2Enabled: true},
		{HTTP2Enabled: false},
	}

	for _, tc := range tt {
		t.Run("", func(t *testing.T) {
			s := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
				if n, _ := io.Copy(ioutil.Discard, r.Body); n == 0 {
					t.Error("body length is zero")
				}
			}))
			s.EnableHTTP2 = tc.HTTP2Enabled
			s.StartTLS()
			defer s.Close()

			c := &http.Client{Transport: http.DefaultTransport}
			c.Transport.(*http.Transport).TLSClientConfig = &tls.Config{InsecureSkipVerify: true}

			body := bytes.NewReader([]byte("something"))
			req, err := http.NewRequest("POST", s.URL, body)
			if err != nil {
				t.Fatalf("unable to create request: %v", err)
			}

			for i := 0; i < 2; i++ {
				resp, err := c.Do(req)
				if err != nil {
					t.Fatalf("unable to sent request#%d: %v", i, err)
				}
				if resp.StatusCode != http.StatusOK {
					t.Fatalf("invalid response status#%d: %d", i, resp.StatusCode)
				}
			}
		})
	}
}

The first test case fails, the second one succeeds.

What did you expect to see?

Maybe I am wrong, but I would expect both tests to behave the same way, i.e. either both succeed or fail. What is the reason the test cases fails if the HTTP/2 stack is enforced?

What did you see instead?

A discrepancy between HTTP/1.1 and HTTP/2 stacks (maybe a valid one).

[seankhliao]

seems like http1 is using GetBody when it shouldn't?

cc @neild