Cannot retry err [...] after Request.Body was written; define Request.GetBody to avoid this error (affects h2 goaway and 307/308 redirects etc)

[timbunce]

The go-retryablehttp package was recently updated to handle retries with a body (hashicorp/go-retryablehttp#207 to fix hashicorp/go-retryablehttp#121).

A similar fix is needed for rehttp.

[mna]

Hey Tim,

Thanks for the report, would you be able to add an example program to reproduce the issue? Can also be in the form of a failing test case. Would make it clearer what the issue is and whether an eventual fix properly fixes this.

To anyone interested and contributing the fix for that, I'd be happy to help review and merge the PR.

Thanks,
Martin

[timbunce]

Here's a patch that adds a test that appears to cover the related "POST response is a redirect" case. It also sets up a framework to add related test cases.

(The http.NewRequest() function automatically sets request.GetBody if the Body type is bytes.Buffer, bytes.Reader, or strings.Reader, and has done for years.)

Uncomment the 307-redirect test to enable it.

diff --git a/rehttp_mock_test.go b/rehttp_mock_test.go
index 8ddcd8b..b8a8377 100644
--- a/rehttp_mock_test.go
+++ b/rehttp_mock_test.go
@@ -117,24 +117,56 @@ func TestMockClientPreventRetryWithBody(t *testing.T) {
 }
 
 func TestMockClientRetryWithBody(t *testing.T) {
-	retFn := func(att int, req *http.Request) (*http.Response, error) {
-		return nil, tempErr{}
-	}
-	mock := &mockRoundTripper{t: t, retFn: retFn}
 
-	tr := NewTransport(mock, RetryAll(RetryMaxRetries(1), RetryTemporaryErr()), ConstDelay(0))
+	newRequest := func(body io.Reader) *http.Request {
+		req, err := http.NewRequest("POST", "http://example.com", body)
+		assert.NoError(t, err)
+		req.Header.Set("Content-Type", "text/plain")
+		return req
+	}
 
-	client := &http.Client{
-		Transport: tr,
+	tests := []struct {
+		name  string
+		req   *http.Request
+		retFn func(att int, req *http.Request) (*http.Response, error)
+		err   error
+	}{
+		{name: "temp-error",
+			req: newRequest(strings.NewReader("hello")),
+			retFn: func(att int, req *http.Request) (*http.Response, error) {
+				return nil, tempErr{}
+			},
+			err: tempErr{},
+		},
+		/* uncomment to test reproduce https://github.com/PuerkitoBio/rehttp/issues/9
+		{name: "307-redirect",
+			req: newRequest(strings.NewReader("hello")),
+			retFn: func(att int, req *http.Request) (*http.Response, error) {
+				if att >= 1 {
+					return &http.Response{StatusCode: 200}, nil
+				}
+				return &http.Response{StatusCode: 307}, nil
+			},
+		},
+		*/
 	}
-	_, err := client.Post("http://example.com", "text/plain", strings.NewReader("hello"))
-	if assert.NotNil(t, err) {
-		uerr, ok := err.(*url.Error)
-		require.True(t, ok)
-		assert.Equal(t, tempErr{}, uerr.Err)
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			mock := &mockRoundTripper{t: t, retFn: tt.retFn}
+
+			tr := NewTransport(mock, RetryAll(RetryMaxRetries(1), RetryTemporaryErr()), ConstDelay(0))
+
+			client := &http.Client{
+				Transport: tr,
+			}
+			_, err := client.Do(tt.req)
+			assert.ErrorIs(t, err, tt.err)
+			assert.Equal(t, 2, mock.Calls())
+			assert.Equal(t, []string{"hello", "hello"}, mock.Bodies())
+		})
 	}
-	assert.Equal(t, 2, mock.Calls())
-	assert.Equal(t, []string{"hello", "hello"}, mock.Bodies())
 }
 
 func TestMockClientRetryTimeout(t *testing.T) {

[mna]

@timbunce Hey Tim, following some back and forth discussion on the PR from Mohan who tried to address this issue, he made me realize that rehttp doens't need a fix because it does not create new Requests, it always uses the one provided by the caller. If a GetBody is set on the caller, the redirections will work properly (it is the caller's responsibility to properly setup the Request). I added some tests that cover these cases: https://github.com/PuerkitoBio/rehttp/pull/11/files

Note that there's still this issue that could improve rehttp by not buffering the body if a GetBody function is available, but I haven't gotten to implement this change yet and that's a different matter anyway, so unless you have more to add to this issue that we may have misunderstood, I'll close it in a bit.

Thanks,
Martin

[timbunce]

All sounds good. Many thanks to you and @openmohan for the investigation and analysis.