Repository: go-resty/resty
Pull request #398 (2020-12-22 17:58:10)

feature: add OnError hook
Adds a new hook that is called when a rest request returns an error.
This is called when the err returned by client.execute is non-nil, and
it is only called after all retry attempts have been exhausted.

This feature is intended to support logging, metrics, tracing on request
errors that otherwise cannot be captured with OnAfterResponse, such as
TLS Connection errors. It also provides a way to report response errors
only once after retries have been attempted, to avoid emitting error
logs when a request ultimately succeeds.

justenwalker: This indirectly allows #356 since you can use OnAfterResponse to return an custom error that includes the response object, and therefore react to it from OnResponseError:

```go
type ResponseError struct {
	Response *resty.Response
}
func (r *ResponseError) Error() string {
	return r.Response.Status()
}

/// ...

	client.OnAfterResponse(func(client *resty.Client, response *resty.Response) error {
		if response.IsError() {
			return &ResponseError{Response: response}
		}
		return nil
	})
	client.OnResponseError(func(err error) {
		var re *ResponseError
		if errors.As(err, &re) {
			// Can use re.Response to get response body, status, etc...
		}
	})
```
codecov[bot]: # [Codecov](https://codecov.io/gh/go-resty/resty/pull/398?src=pr&el=h1) Report
> Merging [#398](https://codecov.io/gh/go-resty/resty/pull/398?src=pr&el=desc) (f654557) into [master](https://codecov.io/gh/go-resty/resty/commit/0de0d7e3240b249466241b87947fcdc87c543909?el=desc) (0de0d7e) will **increase** coverage by `0.22%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/go-resty/resty/pull/398/graphs/tree.svg?width=650&height=150&src=pr&token=9t4RCu3fVO)](https://codecov.io/gh/go-resty/resty/pull/398?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #398      +/-   ##
==========================================
+ Coverage   95.51%   95.73%   +0.22%     
==========================================
  Files          10       10              
  Lines        1003     1009       +6     
==========================================
+ Hits          958      966       +8     
+ Misses         25       24       -1     
+ Partials       20       19       -1     
```


| [Impacted Files](https://codecov.io/gh/go-resty/resty/pull/398?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [client.go](https://codecov.io/gh/go-resty/resty/pull/398/diff?src=pr&el=tree#diff-Y2xpZW50Lmdv) | `96.68% <100.00%> (+0.89%)` | :arrow_up: |
| [request.go](https://codecov.io/gh/go-resty/resty/pull/398/diff?src=pr&el=tree#diff-cmVxdWVzdC5nbw==) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/go-resty/resty/pull/398?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/go-resty/resty/pull/398?src=pr&el=footer). Last update [0de0d7e...f654557](https://codecov.io/gh/go-resty/resty/pull/398?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

justenwalker: One other thing I'm thinking about is if `OnResponseError` is a good name for this hook, since a response is not in the hook, and a request may not have even been executed (in the case of a TLS error). Maybe this should be renamed to just `OnError`
justenwalker: In the absence of any feedback, I've implemented the above enhancements:

1. Renamed OnResponseError to OnError (and related types/funcs)
2. The `err` received by the `ErrorHook` may now be wrapped in a `*ResponseError` which has the last `*Response` from the server. This can either be extracted using `errors.As` or a type assertion, so it should be backward compatible with all Go versions.

**Note**: I'm going the route of wrapping the error rather than changing the function signature so that it is more difficult for users of the library to unintentionally nil-dereference the response. Although the type assertion is arguably uglier to look at, it is more difficult to misuse, which I think is a good trade-off.

## type assertion

```go
client.OnError(func(err error) {
	if v, ok := err.(*resty.ResponseError); ok {
		resp := v.Response()
		// Do something with v.Response
	}
	// Log the error, increment a metric, etc...
})
```

## errors.As

```go
client.OnError(func(err error) {
	var v *resty.ResponseError
	if errors.As(err, &v) {
		// Do something with v.Response
	}
	// Log the error, increment a metric, etc...
})
```


justenwalker: One other thing that might warrant a signature change for the hook is to make the OnError also include the `*Request`, since the request is always available.

```go
ErrorHook func(*Request, error)
```
justenwalker: > One question: why allow multiple `OnError` hooks to be defined? What's the use case?

Don't have a particular use-case in mind for multiple on-error hooks, but using a slice/range makes it at least possible for one to arise without a code change to accommodate it. It doesn't make the implementation any less clean. (ranging over nil is the same as ranging over an empty list, so we don't have to do any nil checks)
moorereason: Thanks for all the hard work and iterating/improving your idea.

I approved this change, but that doesn't mean it will be merged.  We'll wait for @jeevatkm to chime in.  I consider myself a "junior" member of this project, so I defer to @jeevatkm for all merges.

Also, if @jeevatkm approves of this PR, we'll want to update the README where appropriate.

Patch:
diff --git a/README.md b/README.md
index 123c759..644dab8 100644
--- a/README.md
+++ b/README.md
@@ -514,6 +514,28 @@ client.OnAfterResponse(func(c *resty.Client, resp *resty.Response) error {
   })
 ```
 
+#### OnError Hooks
+
+Resty provides OnError hooks that may be called because:
+
+- The client failed to send the request due to connection timeout, TLS handshake failure, etc...
+- The request was retried the maximum amount of times, and still failed.
+
+If there was a response from the server, the original error will be wrapped in `*resty.ResponseError` which contains the last response received.
+
+```go
+// Create a Resty Client
+client := resty.New()
+
+client.OnError(func(req *resty.Request, err error) {
+  if v, ok := err.(*resty.ResponseError); ok {
+    // v.Response contains the last response from the server
+    // v.Err contains the original error
+  }
+  // Log the error, increment a metric, etc...
+})
+```
+
 #### Redirect Policy
 
 Resty provides few ready to use redirect policy(s) also it supports multiple policies together.
@@ -661,7 +683,7 @@ client.
 Above setup will result in resty retrying requests returned non nil error up to
 3 times with delay increased after each attempt.
 
-You can optionally provide client with custom retry conditions:
+You can optionally provide client with [custom retry conditions](https://pkg.go.dev/github.com/go-resty/resty/v2#RetryConditionFunc):
 
 ```go
 // Create a Resty Client
@@ -670,7 +692,7 @@ client := resty.New()
 client.AddRetryCondition(
     // RetryConditionFunc type is for retry condition function
     // input: non-nil Response OR request execution error
-    func(r *resty.Response) (bool, error) {
+    func(r *resty.Response, err error) bool {
         return r.StatusCode() == http.StatusTooManyRequests
     },
 )
diff --git a/client.go b/client.go
index 3bef2d6..9b96fdd 100644
--- a/client.go
+++ b/client.go
@@ -82,6 +82,9 @@ type (
 
 	// ResponseLogCallback type is for response logs, called before the response is logged
 	ResponseLogCallback func(*ResponseLog) error
+
+	// ErrorHook type is for reacting to request errors, called after all retries were attempted
+	ErrorHook func(*Request, error)
 )
 
 // Client struct is used to create Resty client with client level settings,
@@ -128,6 +131,7 @@ type Client struct {
 	afterResponse      []ResponseMiddleware
 	requestLog         RequestLogCallback
 	responseLog        ResponseLogCallback
+	errorHooks         []ErrorHook
 }
 
 // User type is to hold an username and password information
@@ -384,6 +388,22 @@ func (c *Client) OnAfterResponse(m ResponseMiddleware) *Client {
 	return c
 }
 
+// OnError method adds a callback that will be run whenever a request execution fails.
+// This is called after all retries have been attempted (if any).
+// If there was a response from the server, the error will be wrapped in *ResponseError
+// which has the last response received from the server.
+//
+//		client.OnError(func(req *resty.Request, err error) {
+//			if v, ok := err.(*resty.ResponseError); ok {
+//				// Do something with v.Response
+//			}
+//			// Log the error, increment a metric, etc...
+//		})
+func (c *Client) OnError(h ErrorHook) *Client {
+	c.errorHooks = append(c.errorHooks, h)
+	return c
+}
+
 // SetPreRequestHook method sets the given pre-request function into resty client.
 // It is called right before the request is fired.
 //
@@ -902,6 +922,35 @@ func (c *Client) outputLogTo(w io.Writer) *Client {
 	return c
 }
 
+// ResponseError is a wrapper for including the server response with an error.
+// Neither the err nor the response should be nil.
+type ResponseError struct {
+	Response *Response
+	Err      error
+}
+
+func (e *ResponseError) Error() string {
+	return e.Err.Error()
+}
+
+func (e *ResponseError) Unwrap() error {
+	return e.Err
+}
+
+// Helper to run onErrorHooks hooks.
+// It wraps the error in a ResponseError if the resp is not nil
+// so hooks can access it.
+func (c *Client) onErrorHooks(req *Request, resp *Response, err error) {
+	if err != nil {
+		if resp != nil { // wrap with ResponseError
+			err = &ResponseError{Response: resp, Err: err}
+		}
+		for _, h := range c.errorHooks {
+			h(req, err)
+		}
+	}
+}
+
 //‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾
 // File struct and its methods
 //_______________________________________________________________________
diff --git a/request.go b/request.go
index 46b24a6..48c71fe 100644
--- a/request.go
+++ b/request.go
@@ -680,12 +680,14 @@ func (r *Request) Execute(method, url string) (*Response, error) {
 	var err error
 
 	if r.isMultiPart && !(method == MethodPost || method == MethodPut || method == MethodPatch) {
+		// No OnError hook here since this is a request validation error
 		return nil, fmt.Errorf("multipart content is not allowed in HTTP verb [%v]", method)
 	}
 
 	if r.SRV != nil {
 		_, addrs, err = net.LookupSRV(r.SRV.Service, "tcp", r.SRV.Domain)
 		if err != nil {
+			r.client.onErrorHooks(r, nil, err)
 			return nil, err
 		}
 	}
@@ -696,6 +698,7 @@ func (r *Request) Execute(method, url string) (*Response, error) {
 	if r.client.RetryCount == 0 {
 		r.Attempt = 1
 		resp, err = r.client.execute(r)
+		r.client.onErrorHooks(r, resp, unwrapNoRetryErr(err))
 		return resp, unwrapNoRetryErr(err)
 	}
 
@@ -718,6 +721,8 @@ func (r *Request) Execute(method, url string) (*Response, error) {
 		RetryConditions(r.client.RetryConditions),
 	)
 
+	r.client.onErrorHooks(r, resp, unwrapNoRetryErr(err))
+
 	return resp, unwrapNoRetryErr(err)
 }
 
diff --git a/retry.go b/retry.go
index 0b7c6ff..7500b70 100644
--- a/retry.go
+++ b/retry.go
@@ -167,11 +167,8 @@ defaultCase:
 	capLevel := float64(max)
 
 	temp := math.Min(capLevel, base*math.Exp2(float64(attempt)))
-	ri := int(temp / 2)
-	if ri <= 0 {
-		ri = maxInt // max int for arch 386
-	}
-	result := time.Duration(math.Abs(float64(ri + rand.Intn(ri))))
+	ri := int64(temp / 2)
+	result := time.Duration(math.Abs(float64(ri + rand.Int63n(ri))))
 
 	if result < min {
 		result = min


Test patch:
diff --git a/client_test.go b/client_test.go
index 70d67b2..36e65e4 100644
--- a/client_test.go
+++ b/client_test.go
@@ -8,6 +8,7 @@ import (
 	"bytes"
 	"crypto/tls"
 	"errors"
+	"fmt"
 	"io/ioutil"
 	"net"
 	"net/http"
@@ -612,3 +613,117 @@ func TestNewWithLocalAddr(t *testing.T) {
 	assertEqual(t, resp.String(), "TestGet: text response")
 }
 
+func TestClientOnResponseError(t *testing.T) {
+	ts := createAuthServer(t)
+	defer ts.Close()
+
+	tests := []struct {
+		name        string
+		setup       func(*Client)
+		isError     bool
+		hasResponse bool
+	}{
+		{
+			name: "successful_request",
+		},
+		{
+			name: "http_status_error",
+			setup: func(client *Client) {
+				client.SetAuthToken("BAD")
+			},
+		},
+		{
+			name: "before_request_error",
+			setup: func(client *Client) {
+				client.OnBeforeRequest(func(client *Client, request *Request) error {
+					return fmt.Errorf("before request")
+				})
+			},
+			isError: true,
+		},
+		{
+			name: "before_request_error_retry",
+			setup: func(client *Client) {
+				client.SetRetryCount(3).OnBeforeRequest(func(client *Client, request *Request) error {
+					return fmt.Errorf("before request")
+				})
+			},
+			isError: true,
+		},
+		{
+			name: "after_response_error",
+			setup: func(client *Client) {
+				client.OnAfterResponse(func(client *Client, response *Response) error {
+					return fmt.Errorf("after response")
+				})
+			},
+			isError:     true,
+			hasResponse: true,
+		},
+		{
+			name: "after_response_error_retry",
+			setup: func(client *Client) {
+				client.SetRetryCount(3).OnAfterResponse(func(client *Client, response *Response) error {
+					return fmt.Errorf("after response")
+				})
+			},
+			isError:     true,
+			hasResponse: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			var assertErrorHook = func(r *Request, err error) {
+				assertNotNil(t, r)
+				v, ok := err.(*ResponseError)
+				assertEqual(t, test.hasResponse, ok)
+				if ok {
+					assertNotNil(t, v.Response)
+					assertNotNil(t, v.Err)
+				}
+			}
+			var hook1, hook2 int
+			c := New().outputLogTo(ioutil.Discard).
+				SetTLSClientConfig(&tls.Config{InsecureSkipVerify: true}).
+				SetAuthToken("004DDB79-6801-4587-B976-F093E6AC44FF").
+				SetRetryCount(0).
+				SetRetryMaxWaitTime(time.Microsecond).
+				AddRetryCondition(func(response *Response, err error) bool {
+					if err != nil {
+						return true
+					}
+					return response.IsError()
+				}).
+				OnError(func(r *Request, err error) {
+					assertErrorHook(r, err)
+					hook1++
+				}).
+				OnError(func(r *Request, err error) {
+					assertErrorHook(r, err)
+					hook2++
+				})
+			if test.setup != nil {
+				test.setup(c)
+			}
+			_, err := c.R().Get(ts.URL + "/profile")
+			if test.isError {
+				assertNotNil(t, err)
+				assertEqual(t, 1, hook1)
+				assertEqual(t, 1, hook2)
+			} else {
+				assertError(t, err)
+			}
+		})
+	}
+}
+
+func TestResponseError(t *testing.T) {
+	err := errors.New("error message")
+	re := &ResponseError{
+		Response: &Response{},
+		Err:      err,
+	}
+	assertNotNil(t, re.Unwrap())
+	assertEqual(t, err.Error(), re.Error())
+}