Support errors.Is for returned errors. (#114)

* replace valkyrie.MultiError with custom MultiError

- supports errors.Is and errors.Cause
- improved performance (no mutex required)

* simplify & move mutli error, remove Cause method

---------

Co-authored-by: Anmol Chopra <anmol.chopra@gojek.com>

Author: peterzeller

go.mod

@@ -4,7 +4,6 @@ go 1.24
 
 require (
 	github.com/afex/hystrix-go v0.0.0-20180502004556-fa1af6a1f4f5
-	github.com/gojek/valkyrie v0.0.0-20180215180059-6aee720afcdf
 	github.com/pkg/errors v0.9.1
 	github.com/stretchr/testify v1.11.1
 )

go.sum

@@ -6,8 +6,6 @@ github.com/cactus/go-statsd-client/statsd v0.0.0-20200423205355-cb0885a1018c h1:
 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.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

@@ -7,7 +7,6 @@ import (
 
 	"github.com/gojek/heimdall/v7"
 	"github.com/gojek/heimdall/v7/internal"
-	"github.com/gojek/valkyrie"
 	"github.com/pkg/errors"
 )
 
@@ -138,7 +137,7 @@ func (c *Client) Do(request *http.Request) (*http.Response, error) {
 		reqGetBody = request.GetBody
 	}
 
-	multiErr := &valkyrie.MultiError{}
+	var errs []error
 	var response *http.Response
 
 	for i := 0; i <= c.retryCount; i++ {
@@ -148,15 +147,17 @@ func (c *Client) Do(request *http.Request) (*http.Response, error) {
 		}
 		if i > 0 {
 			if err := internal.SleepInterruptible(request.Context(), c.retrier.NextInterval(i-1)); err != nil {
-				multiErr.Push(err.Error())
+				errs = append(errs, err)
 				c.reportError(request, err)
 				// no point of retrying after context has been cancelled
 				break
 			}
 
 			request, err = internal.CloneRequest(request, reqGetBody) // Clone the request to reset the body for retry
 			if err != nil {
-				return nil, err
+				errs = append(errs, err)
+				c.reportError(request, err)
+				break
 			}
 		}
 
@@ -165,7 +166,7 @@ func (c *Client) Do(request *http.Request) (*http.Response, error) {
 		response, err = c.client.Do(request)
 
 		if err != nil {
-			multiErr.Push(err.Error())
+			errs = append(errs, err)
 			c.reportError(request, err)
 			if internal.IsCtxDone(request.Context()) {
 				break
@@ -182,11 +183,11 @@ func (c *Client) Do(request *http.Request) (*http.Response, error) {
 			continue
 		}
 
-		multiErr = &valkyrie.MultiError{} // Clear errors if any iteration succeeds
+		errs = nil // Clear errors if any iteration succeeds
 		break
 	}
 
-	return response, multiErr.HasError()
+	return response, internal.BuildMultiError(errs)
 }
 
 func (c *Client) reportRequestStart(request *http.Request) {

httpclient/client_test.go

@@ -3,9 +3,11 @@ package httpclient
 import (
 	"bytes"
 	"context"
+	"errors"
 	"io"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"strings"
 	"sync/atomic"
 	"testing"
@@ -510,6 +512,36 @@ func TestCustomHTTPClientHeaderSuccess(t *testing.T) {
 	assert.Equal(t, "{ \"response\": \"ok\" }", string(body))
 }
 
+func TestHTTPClientContextTimeout(t *testing.T) {
+	client := NewClient(WithHTTPTimeout(1000 * time.Millisecond))
+
+	dummyHandler := func(w http.ResponseWriter, r *http.Request) {
+		assert.Equal(t, http.MethodGet, r.Method)
+		assert.Equal(t, "application/json", r.Header.Get("Content-Type"))
+		assert.Equal(t, "en", r.Header.Get("Accept-Language"))
+
+		time.Sleep(100 * time.Millisecond)
+
+		w.WriteHeader(http.StatusOK)
+		w.Write([]byte(`{ "response": "ok" }`))
+	}
+
+	server := httptest.NewServer(http.HandlerFunc(dummyHandler))
+	defer server.Close()
+
+	ctxt, cancel := context.WithTimeout(context.Background(), 1*time.Millisecond)
+	defer cancel()
+
+	req, err := http.NewRequestWithContext(ctxt, http.MethodGet, server.URL, nil)
+	require.NoError(t, err)
+	req.Header.Set("Content-Type", "application/json")
+	req.Header.Set("Accept-Language", "en")
+	response, err := client.Do(req)
+	require.ErrorIs(t, err, context.DeadlineExceeded)
+	assert.Equal(t, &url.Error{Op: "Get", URL: server.URL, Err: context.DeadlineExceeded}, err)
+	require.Nil(t, response)
+}
+
 func respBody(t *testing.T, response *http.Response) string {
 	if response.Body != nil {
 		defer response.Body.Close()
@@ -555,7 +587,7 @@ func TestHTTPClientDoContextCancelledDuringRetry(t *testing.T) {
 
 	_, err = client.Do(req)
 	require.Error(t, err)
-	assert.Contains(t, err.Error(), context.Canceled.Error())
+	assert.Equal(t, context.Canceled, err)
 	assert.Less(t, count.Load(), int32(noOfRetries+1), "should not have completed all retries due to context cancellation")
 }
 
@@ -624,3 +656,41 @@ func TestHTTPClientDoContextTimeoutDuringRetry(t *testing.T) {
 	assert.Contains(t, err.Error(), context.DeadlineExceeded.Error())
 	assert.Less(t, count.Load(), int32(noOfRetries+1), "should not have completed all retries due to context timeout")
 }
+
+func TestHTTPClientMultiRetryOnTimeout(t *testing.T) {
+	noOfRetries := 3
+	backoffInterval := 4 * time.Millisecond
+	maximumJitterInterval := 2 * time.Millisecond
+
+	client := NewClient(
+		WithHTTPTimeout(5*time.Millisecond),
+		WithRetryCount(noOfRetries),
+		WithRetrier(heimdall.NewRetrier(heimdall.NewConstantBackoff(backoffInterval, maximumJitterInterval))),
+	)
+
+	dummyHandler := func(w http.ResponseWriter, r *http.Request) {
+		time.Sleep(10 * time.Millisecond)
+		w.WriteHeader(http.StatusOK)
+	}
+
+	server := httptest.NewServer(http.HandlerFunc(dummyHandler))
+	defer server.Close()
+
+	req, err := http.NewRequest(http.MethodGet, server.URL, nil)
+	require.NoError(t, err)
+
+	_, err = client.Do(req)
+	require.Error(t, err)
+	assert.ErrorIs(t, err, context.DeadlineExceeded)
+	mutliErr, ok := err.(interface{ Unwrap() []error })
+	require.True(t, ok)
+	errs := mutliErr.Unwrap()
+	assert.Len(t, errs, noOfRetries+1)
+	for _, e := range errs {
+		var urlErr *url.Error
+		require.True(t, errors.As(e, &urlErr))
+		assert.Equal(t, "Get", urlErr.Op)
+		assert.Equal(t, server.URL, urlErr.URL)
+		assert.ErrorIs(t, urlErr.Err, context.DeadlineExceeded)
+	}
+}

internal/multi_error.go

@@ -0,0 +1,34 @@
+package internal
+
+import "strings"
+
+func BuildMultiError(errors []error) error {
+	if len(errors) == 0 {
+		return nil
+	}
+	if len(errors) == 1 {
+		return errors[0]
+	}
+
+	return &multiError{errors: errors}
+}
+
+type multiError struct {
+	errors []error
+}
+
+func (m multiError) Error() string {
+	var sb strings.Builder
+	for i, e := range m.errors {
+		sb.WriteString(e.Error())
+		if i < len(m.errors)-1 {
+			sb.WriteString(", ")
+		}
+	}
+
+	return sb.String()
+}
+
+func (m multiError) Unwrap() []error {
+	return m.errors
+}

internal/multi_error_test.go

@@ -0,0 +1,34 @@
+package internal_test
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/gojek/heimdall/v7/internal"
+	"github.com/stretchr/testify/assert"
+)
+
+var err1 = errors.New("err1")
+var err2 = errors.New("err2")
+var err3 = errors.New("err3")
+var err4 = errors.New("err4")
+
+func TestBuildMultiError(t *testing.T) {
+	assert.Nil(t, internal.BuildMultiError(nil))
+
+	assert.Equal(t, err1, internal.BuildMultiError([]error{err1}))
+
+	errs := internal.BuildMultiError([]error{err1, err2})
+	assert.Equal(t, "err1, err2", errs.Error())
+	assert.ErrorIs(t, errs, err1)
+	assert.ErrorIs(t, errs, err2)
+	assert.NotErrorIs(t, errs, err3)
+	assert.NotErrorIs(t, errs, err4)
+
+	errs = internal.BuildMultiError([]error{err1, err2, err3, err4})
+	assert.Equal(t, "err1, err2, err3, err4", errs.Error())
+	assert.ErrorIs(t, errs, err1)
+	assert.ErrorIs(t, errs, err2)
+	assert.ErrorIs(t, errs, err3)
+	assert.ErrorIs(t, errs, err4)
+}