From 284cd52433fd343f982a04a9afee22332f55be5a Mon Sep 17 00:00:00 2001 From: Justin Ricks Date: Fri, 22 Sep 2023 13:47:04 -0600 Subject: [PATCH] Promote the use of Timeout() over Temporary() --- context_post17.go | 1 + context_pre17.go | 1 + rehttp.go | 31 ++++++++++++++++++++++--------- rehttp_mock_test.go | 25 +++++++++++++++++++++++++ rehttp_retryfn_test.go | 31 +++++++++++++++++++++++++++++++ timeouterr_post113.go | 18 ++++++++++++++++++ timeouterr_pre113.go | 15 +++++++++++++++ timeouterr_test.go | 42 ++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 155 insertions(+), 9 deletions(-) create mode 100644 timeouterr_post113.go create mode 100644 timeouterr_pre113.go create mode 100644 timeouterr_test.go diff --git a/context_post17.go b/context_post17.go index e4c27a2..dc5eb1c 100644 --- a/context_post17.go +++ b/context_post17.go @@ -1,3 +1,4 @@ +//go:build go1.7 // +build go1.7 package rehttp diff --git a/context_pre17.go b/context_pre17.go index e4aa70d..41ca441 100644 --- a/context_pre17.go +++ b/context_pre17.go @@ -1,3 +1,4 @@ +//go:build !go1.7 // +build !go1.7 package rehttp diff --git a/rehttp.go b/rehttp.go index 44f0e48..62f64a9 100644 --- a/rehttp.go +++ b/rehttp.go @@ -9,16 +9,17 @@ // // The package offers common delay strategies as ready-made functions that // return a DelayFn: -// - ConstDelay(delay time.Duration) DelayFn -// - ExpJitterDelay(base, max time.Duration) DelayFn +// - ConstDelay(delay time.Duration) DelayFn +// - ExpJitterDelay(base, max time.Duration) DelayFn // // It also provides common retry helpers that return a RetryFn: -// - RetryIsErr(func(error) bool) RetryFn -// - RetryHTTPMethods(methods ...string) RetryFn -// - RetryMaxRetries(max int) RetryFn -// - RetryStatuses(statuses ...int) RetryFn -// - RetryStatusInterval(fromStatus, toStatus int) RetryFn -// - RetryTemporaryErr() RetryFn +// - RetryIsErr(func(error) bool) RetryFn +// - RetryHTTPMethods(methods ...string) RetryFn +// - RetryMaxRetries(max int) RetryFn +// - RetryStatuses(statuses ...int) RetryFn +// - RetryStatusInterval(fromStatus, toStatus int) RetryFn +// - RetryTimeoutErr() RetryFn +// - RetryTemporaryErr() RetryFn // // Those can be combined with RetryAny or RetryAll as needed. RetryAny // enables retries if any of the RetryFn return true, while RetryAll @@ -173,6 +174,8 @@ func RetryIsErr(fn func(error) bool) RetryFn { // is a temporary error. A temporary error is one that implements // the Temporary() bool method. Most errors from the net package implement // this. +// This interface was deprecated in go 1.18. Favor RetryTimeoutErr. +// https://github.com/golang/go/issues/45729 func RetryTemporaryErr() RetryFn { return RetryIsErr(func(err error) bool { if terr, ok := err.(temporaryer); ok { @@ -182,6 +185,16 @@ func RetryTemporaryErr() RetryFn { }) } +// RetryTimeoutErr returns a RetryFn that retries if the Attempt's error +// is a timeout error. Before go 1.13, a timeout error is one that implements +// the Timeout() bool method. Most errors from the net package implement this. +// After go 1.13, a timeout error is one that implemts the net.Error interface +// which includes both Timeout() and Temporary() to make it less likely to +// falsely identify errors that occurred outside of the net package. +func RetryTimeoutErr() RetryFn { + return RetryIsErr(isTimeoutErr) +} + // RetryStatusInterval returns a RetryFn that retries if the response's // status code is in the provided half-closed interval [fromStatus, toStatus) // (that is, it retries if fromStatus <= Response.StatusCode < toStatus, so @@ -213,7 +226,7 @@ func RetryStatuses(statuses ...int) RetryFn { // RetryHTTPMethods returns a RetryFn that retries if the request's // HTTP method is one of the provided methods. It is meant to be used -// in conjunction with another RetryFn such as RetryTemporaryErr combined +// in conjunction with another RetryFn such as RetryTimeoutErr combined // using RetryAll, otherwise this function will retry any successful // request made with one of the provided methods. func RetryHTTPMethods(methods ...string) RetryFn { diff --git a/rehttp_mock_test.go b/rehttp_mock_test.go index 31e0ca2..8ddcd8b 100644 --- a/rehttp_mock_test.go +++ b/rehttp_mock_test.go @@ -3,6 +3,7 @@ package rehttp import ( "bytes" "io" + "net" "net/http" "net/url" "strings" @@ -135,3 +136,27 @@ func TestMockClientRetryWithBody(t *testing.T) { assert.Equal(t, 2, mock.Calls()) assert.Equal(t, []string{"hello", "hello"}, mock.Bodies()) } + +func TestMockClientRetryTimeout(t *testing.T) { + retFn := func(att int, req *http.Request) (*http.Response, error) { + return nil, &net.OpError{ + Err: timeoutErr{}, + } + } + mock := &mockRoundTripper{t: t, retFn: retFn} + + tr := NewTransport(mock, RetryAll(RetryMaxRetries(1), RetryTimeoutErr()), ConstDelay(0)) + + client := &http.Client{ + Transport: tr, + } + _, err := client.Get("http://example.com") + if assert.NotNil(t, err) { + uerr, ok := err.(*url.Error) + require.True(t, ok) + assert.Equal(t, &net.OpError{ + Err: timeoutErr{}, + }, uerr.Err) + } + assert.Equal(t, 2, mock.Calls()) +} diff --git a/rehttp_retryfn_test.go b/rehttp_retryfn_test.go index 0c77a55..6063009 100644 --- a/rehttp_retryfn_test.go +++ b/rehttp_retryfn_test.go @@ -2,6 +2,7 @@ package rehttp import ( "io" + "net" "net/http" "testing" "time" @@ -98,6 +99,15 @@ type tempErr struct{} func (t tempErr) Error() string { return "temp error" } func (t tempErr) Temporary() bool { return true } +type timeoutErr struct{} + +func (t timeoutErr) Error() string { return "temp error" } +func (t timeoutErr) Timeout() bool { return true } + +var timeoutNetErr = net.OpError{ + Err: timeoutErr{}, +} + func TestRetryTemporaryErr(t *testing.T) { cases := []struct { retries int @@ -119,6 +129,27 @@ func TestRetryTemporaryErr(t *testing.T) { } } +func TestRetryTimeoutErr(t *testing.T) { + cases := []struct { + retries int + err error + att int + want bool + }{ + {retries: 1, err: nil, att: 0, want: false}, + {retries: 1, err: nil, att: 1, want: false}, + {retries: 1, err: io.EOF, att: 0, want: false}, + {retries: 1, err: &timeoutNetErr, att: 0, want: true}, + {retries: 1, err: &timeoutNetErr, att: 1, want: false}, + } + + for i, tc := range cases { + fn := RetryAll(RetryMaxRetries(tc.retries), RetryTimeoutErr()) + got := fn(Attempt{Index: tc.att, Error: tc.err}) + assert.Equal(t, tc.want, got, "%d", i) + } +} + func TestRetryAll(t *testing.T) { max := RetryMaxRetries(2) status := RetryStatusInterval(500, 600) diff --git a/timeouterr_post113.go b/timeouterr_post113.go new file mode 100644 index 0000000..4143d7c --- /dev/null +++ b/timeouterr_post113.go @@ -0,0 +1,18 @@ +//go:build go1.13 +// +build go1.13 + +package rehttp + +import ( + "errors" + "net" +) + +func isTimeoutErr(err error) bool { + var neterr net.Error + if errors.As(err, &neterr) && neterr.Timeout() { + return true + } + + return false +} diff --git a/timeouterr_pre113.go b/timeouterr_pre113.go new file mode 100644 index 0000000..4517e44 --- /dev/null +++ b/timeouterr_pre113.go @@ -0,0 +1,15 @@ +//go:build !go1.13 +// +build !go1.13 + +package rehttp + +type timeouter interface { + Timeout() bool +} + +func isTimeoutErr(err error) bool { + if terr, ok := err.(timeouter); ok { + return terr.Timeout() + } + return false +} diff --git a/timeouterr_test.go b/timeouterr_test.go new file mode 100644 index 0000000..c6ca410 --- /dev/null +++ b/timeouterr_test.go @@ -0,0 +1,42 @@ +package rehttp + +import ( + "errors" + "net" + "testing" + + "golang.org/x/net/context" +) + +func Test_isTimeoutErr(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + { + name: "context timeouts should be retryable", + err: context.DeadlineExceeded, + want: true, + }, + { + name: "net op errors are true", + err: &net.OpError{ + Err: timeoutErr{}, + }, + want: true, + }, + { + name: "non-network related errors should not be retryable", + err: errors.New("fake error"), + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isTimeoutErr(tt.err); got != tt.want { + t.Errorf("isTimeoutErr() = %v, want %v", got, tt.want) + } + }) + } +}