fix bug & some optimize

README.md

@@ -333,7 +333,7 @@ A plugin is an interface whose methods get called during key events in a request
 
 - `OnRequestStart` is called just before the request is made
 - `OnRequestEnd` is called once the request has successfully executed
-- `OnError` is called is the request failed
+- `OnError` is called when the request failed
 
 Each method is called with the request object as an argument, with `OnRequestEnd`, and `OnError` additionally being called with the response and error instances respectively.
 For a simple example on how to write plugins, look at the [request logger plugin](/plugins/request_logger.go).

go.mod

@@ -6,7 +6,6 @@ require (
 	github.com/DataDog/datadog-go v3.7.1+incompatible // indirect
 	github.com/afex/hystrix-go v0.0.0-20180502004556-fa1af6a1f4f5
 	github.com/cactus/go-statsd-client/statsd v0.0.0-20200423205355-cb0885a1018c // indirect
-	github.com/gojek/valkyrie v0.0.0-20180215180059-6aee720afcdf
 	github.com/pkg/errors v0.9.1
 	github.com/rcrowley/go-metrics v0.0.0-20201227073835-cf1acfcdf475 // indirect
 	github.com/smartystreets/goconvey v1.6.4 // indirect

go.sum

@@ -4,12 +4,9 @@ github.com/afex/hystrix-go v0.0.0-20180502004556-fa1af6a1f4f5 h1:rFw4nCn9iMW+Vaj
 github.com/afex/hystrix-go v0.0.0-20180502004556-fa1af6a1f4f5/go.mod h1:SkGFH1ia65gfNATL8TAiHDNxPzPdmEL5uirI2Uyuz6c=
 github.com/cactus/go-statsd-client/statsd v0.0.0-20200423205355-cb0885a1018c h1:HIGF0r/56+7fuIZw2V4isE22MK6xpxWx7BbV8dJ290w=
 github.com/cactus/go-statsd-client/statsd v0.0.0-20200423205355-cb0885a1018c/go.mod h1:l/bIBLeOl9eX+wxJAzxS4TveKRtAqlyDpHjhkfO0MEI=
-github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
-github.com/gojek/valkyrie v0.0.0-20180215180059-6aee720afcdf h1:5xRGbUdOmZKoDXkGx5evVLehuCMpuO1hl701bEQqXOM=
-github.com/gojek/valkyrie v0.0.0-20180215180059-6aee720afcdf/go.mod h1:QzhUKaYKJmcbTnCYCAVQrroCOY7vOOI8cSQ4NbuhYf0=
 github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1 h1:EGx4pi6eqNxGaHF6qqu48+N2wcFQ5qg5FXgOdqsJ5d8=
 github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1/go.mod h1:wJfORRmW1u3UXTncJ5qlYoELFm8eSnnEO6hX4iZ3EWY=
 github.com/jtolds/gls v4.20.0+incompatible h1:xdiiI2gbIgH/gLH7ADydsJ1uDOEzR8yvV7C0MuV77Wo=

httpclient/client.go

@@ -2,14 +2,15 @@ package httpclient
 
 import (
 	"bytes"
+	"context"
 	"io"
 	"io/ioutil"
 	"net/http"
 	"time"
 
-	"github.com/gojek/heimdall/v7"
-	"github.com/gojek/valkyrie"
 	"github.com/pkg/errors"
+
+	heimdall "github.com/gojek/heimdall/v7"
 )
 
 // Client is the http client implementation
@@ -135,43 +136,71 @@ func (c *Client) Do(request *http.Request) (*http.Response, error) {
 		request.Body = ioutil.NopCloser(bodyReader) // prevents closing the body between retries
 	}
 
-	multiErr := &valkyrie.MultiError{}
+	var err error
+	var needRetry bool
 	var response *http.Response
 
-	for i := 0; i <= c.retryCount; i++ {
+	for i := 0; ; i++ {
 		if response != nil {
 			response.Body.Close()
 		}
 
 		c.reportRequestStart(request)
-		var err error
+
 		response, err = c.client.Do(request)
 		if bodyReader != nil {
 			// Reset the body reader after the request since at this point it's already read
 			// Note that it's safe to ignore the error here since the 0,0 position is always valid
 			_, _ = bodyReader.Seek(0, 0)
 		}
 
+		needRetry, err = c.checkRetry(request.Context(), response, err)
+
 		if err != nil {
-			multiErr.Push(err.Error())
 			c.reportError(request, err)
-			backoffTime := c.retrier.NextInterval(i)
-			time.Sleep(backoffTime)
-			continue
+		} else {
+			c.reportRequestEnd(request, response)
+		}
+
+		if !needRetry {
+			break
+		}
+
+		if c.retryCount-i <= 0 {
+			break
 		}
-		c.reportRequestEnd(request, response)
 
-		if response.StatusCode >= http.StatusInternalServerError {
-			backoffTime := c.retrier.NextInterval(i)
-			time.Sleep(backoffTime)
-			continue
+		// Cancel the retry sleep if the request context is cancelled or deadline exceeded
+		timer := time.NewTimer(c.retrier.NextInterval(i))
+		select {
+		case <-request.Context().Done():
+			timer.Stop()
+			break
+		case <-timer.C:
 		}
+	}
+
+	return response, err
+}
+
+func (c *Client) checkRetry(ctx context.Context, resp *http.Response, err error) (bool, error) {
+	// do not retry on context.Canceled or context.DeadlineExceeded
+	if ctx.Err() != nil {
+		return false, ctx.Err()
+	}
+
+	if err != nil {
+		return true, err
+	}
 
-		multiErr = &valkyrie.MultiError{} // Clear errors if any iteration succeeds
-		break
+	// 429 Too Many Requests is recoverable. Sometimes the server puts
+	// a Retry-After response header to indicate when the server is
+	// available to start processing request from client.
+	if resp.StatusCode == http.StatusTooManyRequests || resp.StatusCode >= http.StatusInternalServerError {
+		return true, nil
 	}
 
-	return response, multiErr.HasError()
+	return false, nil
 }
 
 func (c *Client) reportRequestStart(request *http.Request) {

httpclient/client_test.go

@@ -2,6 +2,7 @@ package httpclient
 
 import (
 	"bytes"
+	"context"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
@@ -15,6 +16,37 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+func TestHTTPRequestWithContextDoSuccess(t *testing.T) {
+	noOfRetries := 3
+	backoffInterval := 1 * time.Millisecond
+	maximumJitterInterval := 10 * time.Millisecond
+
+	client := NewClient(
+		WithRetryCount(noOfRetries),
+		WithRetrier(heimdall.NewRetrier(heimdall.NewConstantBackoff(backoffInterval, maximumJitterInterval))),
+	)
+
+	dummyHandler := func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+		w.Write([]byte(`{ "response": "ok" }`))
+		time.Sleep(100 * time.Millisecond)
+	}
+
+	server := httptest.NewServer(http.HandlerFunc(dummyHandler))
+	defer server.Close()
+
+	req, err := http.NewRequest(http.MethodGet, server.URL, nil)
+	require.NoError(t, err)
+
+	// define some user case context
+	subCtx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond)
+	defer cancel()
+
+	req = req.WithContext(subCtx)
+	_, err = client.Do(req)
+	require.Contains(t, err.Error(), "context deadline exceeded")
+}
+
 func TestHTTPClientDoSuccess(t *testing.T) {
 	client := NewClient(WithHTTPTimeout(10 * time.Millisecond))
 
@@ -201,6 +233,33 @@ func TestHTTPClientPatchSuccess(t *testing.T) {
 	assert.Equal(t, "{ \"response\": \"ok\" }", respBody(t, response))
 }
 
+func TestHTTPClientGetRetriesOnTimeout(t *testing.T) {
+	count := 0
+	noOfRetries := 3
+	noOfCalls := noOfRetries + 1
+	backoffInterval := 1 * time.Millisecond
+	maximumJitterInterval := 10 * time.Millisecond
+
+	client := NewClient(
+		WithHTTPTimeout(3*time.Millisecond),
+		WithRetryCount(noOfRetries),
+		WithRetrier(heimdall.NewRetrier(heimdall.NewConstantBackoff(backoffInterval, maximumJitterInterval))),
+	)
+
+	dummyHandler := func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusInternalServerError)
+		count++
+		time.Sleep(100 * time.Millisecond)
+	}
+
+	server := httptest.NewServer(http.HandlerFunc(dummyHandler))
+	defer server.Close()
+
+	_, err := client.Get(server.URL, http.Header{})
+	require.Contains(t, err.Error(), "context deadline exceeded")
+	assert.Equal(t, noOfCalls, count)
+}
+
 func TestHTTPClientGetRetriesOnFailure(t *testing.T) {
 	count := 0
 	noOfRetries := 3
@@ -406,7 +465,6 @@ func TestHTTPClientGetReturnsNoErrorOn5xxFailure(t *testing.T) {
 	response, err := client.Get(server.URL, http.Header{})
 	require.NoError(t, err)
 	require.Equal(t, http.StatusInternalServerError, response.StatusCode)
-
 }
 
 func TestHTTPClientGetReturnsErrorOnFailure(t *testing.T) {
@@ -484,7 +542,8 @@ func TestCustomHTTPClientHeaderSuccess(t *testing.T) {
 	client := NewClient(
 		WithHTTPTimeout(10*time.Millisecond),
 		WithHTTPClient(&myHTTPClient{
-			client: http.Client{Timeout: 25 * time.Millisecond}}),
+			client: http.Client{Timeout: 25 * time.Millisecond},
+		}),
 	)
 
 	dummyHandler := func(w http.ResponseWriter, r *http.Request) {

httpclient/options_test.go

@@ -128,6 +128,5 @@ func ExampleWithRetrier() {
 	// Output: retry attempt 0
 	// retry attempt 1
 	// retry attempt 2
-	// retry attempt 3
 	// error
 }