fs: improve retriable error detection

fs/closed_conn.go

@@ -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
-}

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,31 +245,25 @@ 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() {
+	// 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() {
+	// Check error strings (yuch!) too
+	errString := err.Error()
+	for _, phrase := range retriableErrorStrings {
+		if strings.Contains(errString, phrase) {
 			return true
 		}
+	}
 
 	return false
 }

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
+		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},
 		{

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,
+	)
+}

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,