From 798502b2040a41ea7dc08b1a1df096e80e50db95 Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Thu, 14 Sep 2017 16:09:48 +0100 Subject: [PATCH] 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 | 39 ++++++++++++++++- fs/closed_conn_unsupported.go | 9 ++++ fs/closed_conn_win.go | 47 ++++++++++---------- fs/error.go | 23 +++++++--- fs/errors_test.go | 80 +++++++++++++++++++++++++++++++++++ 5 files changed, 166 insertions(+), 32 deletions(-) create mode 100644 fs/closed_conn_unsupported.go create mode 100644 fs/errors_test.go diff --git a/fs/closed_conn.go b/fs/closed_conn.go index dcf4974f9..6f00b3894 100644 --- a/fs/closed_conn.go +++ b/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 } diff --git a/fs/closed_conn_unsupported.go b/fs/closed_conn_unsupported.go new file mode 100644 index 000000000..cacd20adf --- /dev/null +++ b/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 +} diff --git a/fs/closed_conn_win.go b/fs/closed_conn_win.go index 7bed21455..17e0cbfa9 100644 --- a/fs/closed_conn_win.go +++ b/fs/closed_conn_win.go @@ -3,32 +3,29 @@ 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 - WSATRY_AGAIN syscall.Errno = 11002 - 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 +const ( + WSAECONNABORTED syscall.Errno = 10053 + WSAHOST_NOT_FOUND syscall.Errno = 11001 + WSATRY_AGAIN syscall.Errno = 11002 + WSAENETRESET syscall.Errno = 10052 + WSAETIMEDOUT syscall.Errno = 10060 +) + +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, + ) } diff --git a/fs/error.go b/fs/error.go index dc826501a..16fe4085d 100644 --- a/fs/error.go +++ b/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,11 +186,12 @@ 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") { - return true + errString := err.Error() + + for _, phrase := range closedConnErrorStrings { + if strings.Contains(errString, phrase) { + return true + } } return isClosedConnErrorPlatform(err) diff --git a/fs/errors_test.go b/fs/errors_test.go new file mode 100644 index 000000000..2d8a64751 --- /dev/null +++ b/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)) + } +}