Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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
  • Loading branch information
tritone committed Oct 5, 2021
1 parent 07c804b commit 0e102a3
Show file tree
Hide file tree
Showing 7 changed files with 36 additions and 30 deletions.
7 changes: 5 additions & 2 deletions storage/bucket.go
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions storage/doc.go
Expand Up @@ -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 { ... }
}
*/
Expand Down
28 changes: 14 additions & 14 deletions storage/integration_test.go
Expand Up @@ -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")
}
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion storage/invoke.go
Expand Up @@ -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
})
Expand Down
4 changes: 3 additions & 1 deletion storage/retry_test.go
Expand Up @@ -25,6 +25,7 @@ import (
"time"

"golang.org/x/oauth2"
"golang.org/x/xerrors"

"cloud.google.com/go/storage"
"google.golang.org/api/googleapi"
Expand Down Expand Up @@ -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)
}
Expand Down
17 changes: 8 additions & 9 deletions storage/storage.go
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
Expand Down
3 changes: 2 additions & 1 deletion storage/writer.go
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
}
Expand Down

0 comments on commit 0e102a3

Please sign in to comment.