Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
michaljemala opened this issue Nov 16, 2021 · 1 comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@michaljemala
Copy link

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 seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 16, 2021
@seankhliao
Copy link
Member

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

cc @neild

@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

2 participants