diff --git a/fs/closed_conn.go b/fs/closed_conn.go deleted file mode 100644 index 6f00b3894..000000000 --- a/fs/closed_conn.go +++ /dev/null @@ -1,46 +0,0 @@ -// +build !plan9 - -package fs - -import ( - "net" - "os" - "syscall" -) - -// closedConnErrors indicate a connection is closed or broken and -// should be retried -// -// These are added to in closed_conn_win.go -var closedConnErrors = []syscall.Errno{ - syscall.EPIPE, - syscall.ETIMEDOUT, - syscall.ECONNREFUSED, - syscall.EHOSTDOWN, - syscall.EHOSTUNREACH, - syscall.ECONNABORTED, - syscall.EAGAIN, - syscall.EWOULDBLOCK, - syscall.ECONNRESET, -} - -// isClosedConnErrorPlatform reports whether err is an error from use -// of a closed network connection using platform specific error codes. -func isClosedConnErrorPlatform(err error) bool { - // now check whether err is an error from use of a closed - // network connection using platform specific error codes. - // - // Code adapted from net/http - if oe, ok := err.(*net.OpError); ok { - if se, ok := oe.Err.(*os.SyscallError); ok { - if errno, ok := se.Err.(syscall.Errno); ok { - for _, retriableErrno := range closedConnErrors { - if errno == retriableErrno { - return true - } - } - } - } - } - return false -} diff --git a/fs/error.go b/fs/error.go index 16fe4085d..aa928cb9c 100644 --- a/fs/error.go +++ b/fs/error.go @@ -6,7 +6,7 @@ import ( "fmt" "io" "net/http" - "net/url" + "reflect" "strings" "github.com/pkg/errors" @@ -167,34 +167,72 @@ func IsNoRetryError(err error) bool { return false } -// closedConnErrorStrings is a list of phrases which when we find it +// Cause is a souped up errors.Cause which can unwrap some standard +// library errors too. It returns true if any of the intermediate +// errors had a Timeout() or Temporary() method which returned true. +func Cause(cause error) (retriable bool, err error) { + err = cause + for prev := err; err != nil; prev = err { + // Check for net error Timeout() + if x, ok := err.(interface { + Timeout() bool + }); ok && x.Timeout() { + retriable = true + } + + // Check for net error Temporary() + if x, ok := err.(interface { + Temporary() bool + }); ok && x.Temporary() { + retriable = true + } + + // Unwrap 1 level if possible + err = errors.Cause(err) + if err == prev { + // Unpack any struct or *struct with a field + // of name Err which satisfies the error + // interface. This includes *url.Error, + // *net.OpError, *os.SyscallError and many + // others in the stdlib + errType := reflect.TypeOf(err) + errValue := reflect.ValueOf(err) + if errType.Kind() == reflect.Ptr { + errType = errType.Elem() + errValue = errValue.Elem() + } + if errType.Kind() == reflect.Struct { + if errField := errValue.FieldByName("Err"); errField.IsValid() { + errFieldValue := errField.Interface() + if newErr, ok := errFieldValue.(error); ok { + err = newErr + } + } + } + } + if err == prev { + break + } + } + return retriable, err +} + +// retriableErrorStrings is a list of phrases which when we find it // in an an error, we know it is a networking error which should be // retried. // // This is incredibly ugly - if only errors.Cause worked for all // errors and all errors were exported from the stdlib. -var closedConnErrorStrings = []string{ +var retriableErrorStrings = []string{ "use of closed network connection", // not exported :-( } -// isClosedConnError reports whether err is an error from use of a closed -// network connection or prematurely closed connection +// Errors which indicate networking errors which should be retried // -// Code adapted from net/http -func isClosedConnError(err error) bool { - if err == nil { - return false - } - - errString := err.Error() - - for _, phrase := range closedConnErrorStrings { - if strings.Contains(errString, phrase) { - return true - } - } - - return isClosedConnErrorPlatform(err) +// These are added to in retriable_errors*.go +var retriableErrors = []error{ + io.EOF, + io.ErrUnexpectedEOF, } // ShouldRetry looks at an error and tries to work out if retrying the @@ -207,30 +245,24 @@ func ShouldRetry(err error) bool { } // Find root cause if available - err = errors.Cause(err) - - // Unwrap url.Error - if urlErr, ok := err.(*url.Error); ok { - err = urlErr.Err - } - - // Look for premature closing of connection - if err == io.EOF || err == io.ErrUnexpectedEOF || isClosedConnError(err) { + retriable, err := Cause(err) + if retriable { return true } - // Check for net error Timeout() - if x, ok := err.(interface { - Timeout() bool - }); ok && x.Timeout() { - return true + // Check if it is a retriable error + for _, retriableErr := range retriableErrors { + if err == retriableErr { + return true + } } - // Check for net error Temporary() - if x, ok := err.(interface { - Temporary() bool - }); ok && x.Temporary() { - return true + // Check error strings (yuch!) too + errString := err.Error() + for _, phrase := range retriableErrorStrings { + if strings.Contains(errString, phrase) { + return true + } } return false diff --git a/fs/errors_test.go b/fs/error_test.go similarity index 55% rename from fs/errors_test.go rename to fs/error_test.go index 2d8a64751..5900c6d45 100644 --- a/fs/errors_test.go +++ b/fs/error_test.go @@ -29,19 +29,56 @@ func makeNetErr(errno syscall.Errno) error { } } -func TestIsClosedConnError(t *testing.T) { +type myError1 struct { + Err error +} + +func (e myError1) Error() string { return e.Err.Error() } + +type myError2 struct { + Err error +} + +func (e *myError2) Error() string { return e.Err.Error() } + +type myError3 struct { + Err int +} + +func (e *myError3) Error() string { return "hello" } + +type myError4 struct { + e error +} + +func (e *myError4) Error() string { return e.e.Error() } + +func TestCause(t *testing.T) { + e3 := &myError3{3} + e4 := &myError4{io.EOF} + + errPotato := errors.New("potato") for i, test := range []struct { - err error - want bool + err error + wantRetriable bool + wantErr error }{ - {nil, false}, - {errors.New("potato"), false}, - {errUseOfClosedNetworkConnection, true}, - {makeNetErr(syscall.EAGAIN), true}, - {makeNetErr(syscall.Errno(123123123)), false}, + {nil, false, nil}, + {errPotato, false, errPotato}, + {errors.Wrap(errPotato, "potato"), false, errPotato}, + {errors.Wrap(errors.Wrap(errPotato, "potato2"), "potato"), false, errPotato}, + {errUseOfClosedNetworkConnection, false, errUseOfClosedNetworkConnection}, + {makeNetErr(syscall.EAGAIN), true, syscall.EAGAIN}, + {makeNetErr(syscall.Errno(123123123)), false, syscall.Errno(123123123)}, + {myError1{io.EOF}, false, io.EOF}, + {&myError2{io.EOF}, false, io.EOF}, + {e3, false, e3}, + {e4, false, e4}, } { - got := isClosedConnError(test.err) - assert.Equal(t, test.want, got, fmt.Sprintf("test #%d: %v", i, test.err)) + gotRetriable, gotErr := Cause(test.err) + what := fmt.Sprintf("test #%d: %v", i, test.err) + assert.Equal(t, test.wantErr, gotErr, what) + assert.Equal(t, test.wantRetriable, gotRetriable, what) } } @@ -55,6 +92,8 @@ func TestShouldRetry(t *testing.T) { {errors.Wrap(errUseOfClosedNetworkConnection, "connection"), true}, {io.EOF, true}, {io.ErrUnexpectedEOF, true}, + {makeNetErr(syscall.EAGAIN), true}, + {makeNetErr(syscall.Errno(123123123)), false}, {&url.Error{Op: "post", URL: "/", Err: io.EOF}, true}, {&url.Error{Op: "post", URL: "/", Err: errUseOfClosedNetworkConnection}, true}, { diff --git a/fs/retriable_errors.go b/fs/retriable_errors.go new file mode 100644 index 000000000..d8a3bd4de --- /dev/null +++ b/fs/retriable_errors.go @@ -0,0 +1,21 @@ +// +build !plan9 + +package fs + +import ( + "syscall" +) + +func init() { + retriableErrors = append(retriableErrors, + syscall.EPIPE, + syscall.ETIMEDOUT, + syscall.ECONNREFUSED, + syscall.EHOSTDOWN, + syscall.EHOSTUNREACH, + syscall.ECONNABORTED, + syscall.EAGAIN, + syscall.EWOULDBLOCK, + syscall.ECONNRESET, + ) +} diff --git a/fs/closed_conn_win.go b/fs/retriable_errors_windows.go similarity index 92% rename from fs/closed_conn_win.go rename to fs/retriable_errors_windows.go index 17e0cbfa9..a4b86a6d0 100644 --- a/fs/closed_conn_win.go +++ b/fs/retriable_errors_windows.go @@ -17,7 +17,7 @@ const ( func init() { // append some lower level errors since the standardized ones // don't seem to happen - closedConnErrors = append(closedConnErrors, + retriableErrors = append(retriableErrors, syscall.WSAECONNRESET, WSAECONNABORTED, WSAHOST_NOT_FOUND,