fs: add more errors to be considered temporary errors

This makes a framework for adding temporary errors identified by
syscall number or by error string.

Fixes #1660

fs/closed_conn.go

@@ -1,9 +1,46 @@
-// +build !windows
+// +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/closed_conn_unsupported.go

@@ -0,0 +1,9 @@
+// +build plan9
+
+package fs
+
+// isClosedConnErrorPlatform reports whether err is an error from use
+// of a closed network connection using platform specific error codes.
+func isClosedConnErrorPlatform(err error) bool {
+	return false
+}

fs/closed_conn_win.go

@@ -3,19 +3,9 @@
 package fs
 
 import (
-	"net"
-	"os"
 	"syscall"
 )
 
-// isClosedConnErrorPlatform reports whether err is an error from use
-// of a closed network connection using platform specific error codes.
-//
-// Code adapted from net/http
-func isClosedConnErrorPlatform(err error) bool {
-	if oe, ok := err.(*net.OpError); ok {
-		if se, ok := oe.Err.(*os.SyscallError); ok {
-			if errno, ok := se.Err.(syscall.Errno); ok {
 const (
 	WSAECONNABORTED   syscall.Errno = 10053
 	WSAHOST_NOT_FOUND syscall.Errno = 11001
@@ -23,12 +13,19 @@ func isClosedConnErrorPlatform(err error) bool {
 	WSAENETRESET      syscall.Errno = 10052
 	WSAETIMEDOUT      syscall.Errno = 10060
 )
-				switch errno {
-				case syscall.WSAECONNRESET, WSAECONNABORTED, WSAHOST_NOT_FOUND, WSATRY_AGAIN, WSAENETRESET, WSAETIMEDOUT:
-					return true
-				}
-			}
-		}
-	}
-	return false
+
+func init() {
+	// append some lower level errors since the standardized ones
+	// don't seem to happen
+	closedConnErrors = append(closedConnErrors,
+		syscall.WSAECONNRESET,
+		WSAECONNABORTED,
+		WSAHOST_NOT_FOUND,
+		WSATRY_AGAIN,
+		WSAENETRESET,
+		WSAETIMEDOUT,
+		syscall.ERROR_HANDLE_EOF,
+		syscall.ERROR_NETNAME_DELETED,
+		syscall.ERROR_BROKEN_PIPE,
+	)
 }

fs/error.go

@@ -167,8 +167,18 @@ func IsNoRetryError(err error) bool {
 	return false
 }
 
+// closedConnErrorStrings 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{
+	"use of closed network connection", // not exported :-(
+}
+
 // isClosedConnError reports whether err is an error from use of a closed
-// network connection.
+// network connection or prematurely closed connection
 //
 // Code adapted from net/http
 func isClosedConnError(err error) bool {
@@ -176,12 +186,13 @@ func isClosedConnError(err error) bool {
 		return false
 	}
 
-	// Note that this error isn't exported so we have to do a
-	// string comparison :-(
-	str := err.Error()
-	if strings.Contains(str, "use of closed network connection") {
+	errString := err.Error()
+
+	for _, phrase := range closedConnErrorStrings {
+		if strings.Contains(errString, phrase) {
 			return true
 		}
+	}
 
 	return isClosedConnErrorPlatform(err)
 }

fs/errors_test.go

@@ -0,0 +1,80 @@
+package fs
+
+import (
+	"fmt"
+	"io"
+	"net"
+	"net/url"
+	"os"
+	"syscall"
+	"testing"
+
+	"github.com/pkg/errors"
+	"github.com/stretchr/testify/assert"
+)
+
+var errUseOfClosedNetworkConnection = errors.New("use of closed network connection")
+
+// make a plausible network error with the underlying errno
+func makeNetErr(errno syscall.Errno) error {
+	return &net.OpError{
+		Op:     "write",
+		Net:    "tcp",
+		Source: &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 123},
+		Addr:   &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 8080},
+		Err: &os.SyscallError{
+			Syscall: "write",
+			Err:     errno,
+		},
+	}
+}
+
+func TestIsClosedConnError(t *testing.T) {
+	for i, test := range []struct {
+		err  error
+		want bool
+	}{
+		{nil, false},
+		{errors.New("potato"), false},
+		{errUseOfClosedNetworkConnection, true},
+		{makeNetErr(syscall.EAGAIN), true},
+		{makeNetErr(syscall.Errno(123123123)), false},
+	} {
+		got := isClosedConnError(test.err)
+		assert.Equal(t, test.want, got, fmt.Sprintf("test #%d: %v", i, test.err))
+	}
+}
+
+func TestShouldRetry(t *testing.T) {
+	for i, test := range []struct {
+		err  error
+		want bool
+	}{
+		{nil, false},
+		{errors.New("potato"), false},
+		{errors.Wrap(errUseOfClosedNetworkConnection, "connection"), true},
+		{io.EOF, true},
+		{io.ErrUnexpectedEOF, true},
+		{&url.Error{Op: "post", URL: "/", Err: io.EOF}, true},
+		{&url.Error{Op: "post", URL: "/", Err: errUseOfClosedNetworkConnection}, true},
+		{
+			errors.Wrap(&url.Error{
+				Op:  "post",
+				URL: "http://localhost/",
+				Err: makeNetErr(syscall.EPIPE),
+			}, "potato error"),
+			true,
+		},
+		{
+			errors.Wrap(&url.Error{
+				Op:  "post",
+				URL: "http://localhost/",
+				Err: makeNetErr(syscall.Errno(123123123)),
+			}, "listing error"),
+			false,
+		},
+	} {
+		got := ShouldRetry(test.err)
+		assert.Equal(t, test.want, got, fmt.Sprintf("test #%d: %v", i, test.err))
+	}
+}