From 0e102a385dc67a06f6b444b3a93e6998428529be Mon Sep 17 00:00:00 2001 From: Chris Cotter Date: Tue, 5 Oct 2021 16:36:12 -0600 Subject: [PATCH] feat(storage): returned wrapped error for timeouts (#4802) This piggybacks on #4797 to allow storage to expose wrapped service errors when a call retries without success until timeout or cancellation. I also updated all checks for context sentinel errors in storage to use xerrors.Is instead of strict equality. Users of this package should do the same. I'll update the documentation on errors from this package in a subsequent PR. We will have to bump the dependency on the root module before merging this change I believe. Fixes #4197 --- storage/bucket.go | 7 +++++-- storage/doc.go | 5 +++-- storage/integration_test.go | 28 ++++++++++++++-------------- storage/invoke.go | 2 +- storage/retry_test.go | 4 +++- storage/storage.go | 17 ++++++++--------- storage/writer.go | 3 ++- 7 files changed, 36 insertions(+), 30 deletions(-) diff --git a/storage/bucket.go b/storage/bucket.go index f2a59f3b3d1..89a2347b511 100644 --- a/storage/bucket.go +++ b/storage/bucket.go @@ -23,6 +23,7 @@ import ( "cloud.google.com/go/internal/optional" "cloud.google.com/go/internal/trace" + "golang.org/x/xerrors" "google.golang.org/api/googleapi" "google.golang.org/api/iterator" raw "google.golang.org/api/storage/v1" @@ -166,7 +167,8 @@ func (b *BucketHandle) Attrs(ctx context.Context) (attrs *BucketAttrs, err error resp, err = req.Context(ctx).Do() return err }) - if e, ok := err.(*googleapi.Error); ok && e.Code == http.StatusNotFound { + var e *googleapi.Error + if ok := xerrors.As(err, &e); ok && e.Code == http.StatusNotFound { return nil, ErrBucketNotExist } if err != nil { @@ -1313,7 +1315,8 @@ func (it *ObjectIterator) fetch(pageSize int, pageToken string) (string, error) return err }) if err != nil { - if e, ok := err.(*googleapi.Error); ok && e.Code == http.StatusNotFound { + var e *googleapi.Error + if ok := xerrors.As(err, &e); ok && e.Code == http.StatusNotFound { err = ErrBucketNotExist } return "", err diff --git a/storage/doc.go b/storage/doc.go index 418e16068a9..53259936d64 100644 --- a/storage/doc.go +++ b/storage/doc.go @@ -247,9 +247,10 @@ as the documentation of GenerateSignedPostPolicyV4. Errors Errors returned by this client are often of the type [`googleapi.Error`](https://godoc.org/google.golang.org/api/googleapi#Error). -These errors can be introspected for more information by type asserting to the richer `googleapi.Error` type. For example: +These errors can be introspected for more information by using `errors.As` with the richer `googleapi.Error` type. For example: - if e, ok := err.(*googleapi.Error); ok { + var e *googleapi.Error + if ok := errors.As(err, &e); ok { if e.Code == 409 { ... } } */ diff --git a/storage/integration_test.go b/storage/integration_test.go index b0af5250efb..45f56ee4039 100644 --- a/storage/integration_test.go +++ b/storage/integration_test.go @@ -1508,16 +1508,17 @@ func TestIntegration_Objects(t *testing.T) { // Since a 429 or 5xx is hard to cause, we trigger a 416. realLen := len(contents[objName]) _, err := bkt.Object(objName).NewRangeReader(ctx, int64(realLen*2), 10) - if err, ok := err.(*googleapi.Error); !ok { + var e *googleapi.Error + if ok := xerrors.As(err, &e); !ok { t.Error("NewRangeReader did not return a googleapi.Error") } else { - if err.Code != 416 { - t.Errorf("Code = %d; want %d", err.Code, 416) + if e.Code != 416 { + t.Errorf("Code = %d; want %d", e.Code, 416) } - if len(err.Header) == 0 { + if len(e.Header) == 0 { t.Error("Missing googleapi.Error.Header") } - if len(err.Body) == 0 { + if len(e.Body) == 0 { t.Error("Missing googleapi.Error.Body") } } @@ -2782,8 +2783,9 @@ func TestIntegration_RequesterPays(t *testing.T) { if err == nil { return 0 } - if err, ok := err.(*googleapi.Error); ok { - return err.Code + var e *googleapi.Error + if ok := xerrors.As(err, &e); ok { + return e.Code } return -1 } @@ -3032,8 +3034,8 @@ func TestIntegration_PublicBucket(t *testing.T) { } errCode := func(err error) int { - err2, ok := err.(*googleapi.Error) - if !ok { + var err2 *googleapi.Error + if ok := xerrors.As(err, &err2); !ok { return -1 } return err2.Code @@ -3215,14 +3217,12 @@ func TestIntegration_CancelWrite(t *testing.T) { cancel() // The next Write should return context.Canceled. _, err = w.Write(buf) - // TODO: Once we drop support for Go versions < 1.13, use errors.Is() to - // check for context cancellation instead. - if err != context.Canceled && !strings.Contains(err.Error(), "context canceled") { + if !xerrors.Is(err, context.Canceled) { t.Fatalf("got %v, wanted context.Canceled", err) } // The Close should too. err = w.Close() - if err != context.Canceled && !strings.Contains(err.Error(), "context canceled") { + if !xerrors.Is(err, context.Canceled) { t.Fatalf("got %v, wanted context.Canceled", err) } } @@ -3874,7 +3874,7 @@ func TestIntegration_ReaderCancel(t *testing.T) { buf := make([]byte, 1000) _, readErr = r.Read(buf) if readErr != nil { - if readErr == context.Canceled { + if xerrors.Is(readErr, context.Canceled) { return } break diff --git a/storage/invoke.go b/storage/invoke.go index 2ecf48fffd4..0c44c2db14c 100644 --- a/storage/invoke.go +++ b/storage/invoke.go @@ -36,7 +36,7 @@ func runWithRetry(ctx context.Context, call func() error) error { return true, nil } if shouldRetry(err) { - return false, nil + return false, err } return true, err }) diff --git a/storage/retry_test.go b/storage/retry_test.go index cfa31fb2909..fe66e443f7c 100644 --- a/storage/retry_test.go +++ b/storage/retry_test.go @@ -25,6 +25,7 @@ import ( "time" "golang.org/x/oauth2" + "golang.org/x/xerrors" "cloud.google.com/go/storage" "google.golang.org/api/googleapi" @@ -160,7 +161,8 @@ func TestIndefiniteRetries(t *testing.T) { case <-time.After(maxWait): t.Fatalf("Test took longer than %s to return", maxWait) case err := <-closeDone: - ge, ok := err.(*googleapi.Error) + var ge *googleapi.Error + ok := xerrors.As(err, &ge) if !ok { t.Fatalf("Got error (%v) of type %T, expected *googleapi.Error", err, err) } diff --git a/storage/storage.go b/storage/storage.go index a5627be34d4..ab5b8133452 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -41,6 +41,7 @@ import ( "cloud.google.com/go/internal/trace" "cloud.google.com/go/internal/version" gapic "cloud.google.com/go/storage/internal/apiv2" + "golang.org/x/xerrors" "google.golang.org/api/googleapi" "google.golang.org/api/option" "google.golang.org/api/option/internaloption" @@ -867,7 +868,8 @@ func (o *ObjectHandle) Attrs(ctx context.Context) (attrs *ObjectAttrs, err error var obj *raw.Object setClientHeader(call.Header()) err = runWithRetry(ctx, func() error { obj, err = call.Do(); return err }) - if e, ok := err.(*googleapi.Error); ok && e.Code == http.StatusNotFound { + var e *googleapi.Error + if ok := xerrors.As(err, &e); ok && e.Code == http.StatusNotFound { return nil, ErrObjectNotExist } if err != nil { @@ -967,7 +969,8 @@ func (o *ObjectHandle) Update(ctx context.Context, uattrs ObjectAttrsToUpdate) ( var obj *raw.Object setClientHeader(call.Header()) err = runWithRetry(ctx, func() error { obj, err = call.Do(); return err }) - if e, ok := err.(*googleapi.Error); ok && e.Code == http.StatusNotFound { + var e *googleapi.Error + if ok := xerrors.As(err, &e); ok && e.Code == http.StatusNotFound { return nil, ErrObjectNotExist } if err != nil { @@ -1030,13 +1033,9 @@ func (o *ObjectHandle) Delete(ctx context.Context) error { // Encryption doesn't apply to Delete. setClientHeader(call.Header()) err := runWithRetry(ctx, func() error { return call.Do() }) - switch e := err.(type) { - case nil: - return nil - case *googleapi.Error: - if e.Code == http.StatusNotFound { - return ErrObjectNotExist - } + var e *googleapi.Error + if ok := xerrors.As(err, &e); ok && e.Code == http.StatusNotFound { + return ErrObjectNotExist } return err } diff --git a/storage/writer.go b/storage/writer.go index 0b9c5414484..c25a20e4b17 100644 --- a/storage/writer.go +++ b/storage/writer.go @@ -24,6 +24,7 @@ import ( "unicode/utf8" "github.com/golang/protobuf/proto" + "golang.org/x/xerrors" "google.golang.org/api/googleapi" raw "google.golang.org/api/storage/v1" storagepb "google.golang.org/genproto/googleapis/storage/v2" @@ -219,7 +220,7 @@ func (w *Writer) Write(p []byte) (n int, err error) { // Preserve existing functionality that when context is canceled, Write will return // context.Canceled instead of "io: read/write on closed pipe". This hides the // pipe implementation detail from users and makes Write seem as though it's an RPC. - if werr == context.Canceled || werr == context.DeadlineExceeded { + if xerrors.Is(werr, context.Canceled) || xerrors.Is(werr, context.DeadlineExceeded) { return n, werr } }