diff --git a/httpclient/client.go b/httpclient/client.go index ed873ca..fe17d23 100644 --- a/httpclient/client.go +++ b/httpclient/client.go @@ -154,12 +154,13 @@ func (c *Client) Do(request *http.Request) (*http.Response, error) { break } - request, err = internal.CloneRequest(request, reqGetBody) // Clone the request to reset the body for retry + clonedRequest, err := internal.CloneRequest(request, reqGetBody) if err != nil { errs = append(errs, err) c.reportError(request, err) break } + request = clonedRequest } c.reportRequestStart(request) diff --git a/httpclient/client_test.go b/httpclient/client_test.go index 4af03c7..36ebfc7 100644 --- a/httpclient/client_test.go +++ b/httpclient/client_test.go @@ -14,6 +14,7 @@ import ( "time" "github.com/gojek/heimdall/v8" + "github.com/gojek/heimdall/v8/plugins" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -551,6 +552,113 @@ func TestPluginErrorMethodCalled(t *testing.T) { assert.Contains(t, err.Error(), "unsupported protocol scheme") } +func TestHTTPClientRetryBodyErrorReportsRequest(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + failedClone int + transportFail bool + }{ + {name: "first retry after transport error", failedClone: 1, transportFail: true}, + {name: "later retry after transport error", failedClone: 2, transportFail: true}, + {name: "first retry after server error", failedClone: 1}, + {name: "later retry after server error", failedClone: 2}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + bodyErr := errors.New("cannot reopen request body") + transportErr := errors.New("transport failed") + body := &closeTrackingBody{Reader: strings.NewReader("payload")} + ctx := t.Context() + req, err := http.NewRequestWithContext(ctx, http.MethodPost, "http://example.com/upload", body) + require.NoError(t, err) + req.Header.Set("X-Request-ID", "retry-body-error") + cloneCalls := 0 + req.GetBody = func() (io.ReadCloser, error) { + cloneCalls++ + if cloneCalls == tc.failedClone { + return nil, bodyErr + } + return io.NopCloser(strings.NewReader("payload")), nil + } + + var lastRequest *http.Request + doCalls := 0 + client := NewClient( + WithRetryCount(3), + WithHTTPClient(httpDoerFunc(func(request *http.Request) (*http.Response, error) { + lastRequest = request + doCalls++ + payload, err := io.ReadAll(request.Body) + require.NoError(t, err) + assert.Equal(t, "payload", string(payload)) + require.NoError(t, request.Body.Close()) + if tc.transportFail { + return nil, transportErr + } + return &http.Response{ + StatusCode: http.StatusServiceUnavailable, + Body: io.NopCloser(strings.NewReader("unavailable")), + }, nil + })), + ) + mockPlugin := &MockPlugin{} + mockPlugin.On("OnRequestStart", mock.Anything) + mockPlugin.On("OnRequestEnd", mock.Anything, mock.Anything) + mockPlugin.On("OnError", mock.Anything, mock.Anything).Run(func(args mock.Arguments) { + pluginRequest, ok := args.Get(0).(*http.Request) + assert.True(t, ok) + assert.Same(t, lastRequest, pluginRequest) + if assert.NotNil(t, pluginRequest) { + assert.Equal(t, req.URL.String(), pluginRequest.URL.String()) + assert.Equal(t, "retry-body-error", pluginRequest.Header.Get("X-Request-ID")) + assert.Equal(t, ctx.Done(), pluginRequest.Context().Done()) + } + }) + client.AddPlugin(mockPlugin) + var errorLog bytes.Buffer + client.AddPlugin(plugins.NewRequestLogger(io.Discard, &errorLog)) + + var resultErr error + require.NotPanics(t, func() { + _, resultErr = client.Do(req) + }) + require.ErrorIs(t, resultErr, bodyErr) + if tc.transportFail { + assert.ErrorIs(t, resultErr, transportErr) + } + assert.Equal(t, tc.failedClone, doCalls) + assert.Equal(t, tc.failedClone, cloneCalls) + assert.True(t, body.closed) + mockPlugin.AssertCalled(t, "OnError", lastRequest, bodyErr) + mockPlugin.AssertNumberOfCalls(t, "OnRequestStart", doCalls) + assert.Contains(t, errorLog.String(), req.URL.String()) + assert.Contains(t, errorLog.String(), bodyErr.Error()) + if tc.failedClone > 1 { + assert.NotSame(t, req, lastRequest) + } + }) + } +} + +type httpDoerFunc func(*http.Request) (*http.Response, error) + +func (f httpDoerFunc) Do(request *http.Request) (*http.Response, error) { + return f(request) +} + +type closeTrackingBody struct { + io.Reader + closed bool +} + +func (b *closeTrackingBody) Close() error { + b.closed = true + return nil +} + type myHTTPClient struct { client http.Client }