diff --git a/storage/client_test.go b/storage/client_test.go index af8fc4c32e9f..d54686c2b3dc 100644 --- a/storage/client_test.go +++ b/storage/client_test.go @@ -28,6 +28,7 @@ import ( "cloud.google.com/go/iam/apiv1/iampb" "github.com/google/go-cmp/cmp" + "github.com/googleapis/gax-go/v2" "github.com/googleapis/gax-go/v2/apierror" "github.com/googleapis/gax-go/v2/callctx" "google.golang.org/api/iterator" @@ -1351,41 +1352,105 @@ func TestObjectConditionsEmulated(t *testing.T) { func TestRetryNeverEmulated(t *testing.T) { transportClientTest(t, func(t *testing.T, project, bucket string, client storageClient) { ctx := context.Background() + instructions := map[string][]string{"storage.buckets.get": {"return-503"}} + testID := createRetryTest(t, project, bucket, client, instructions) + ctx = callctx.SetHeaders(ctx, "x-retry-test-id", testID) + _, err := client.GetBucket(ctx, bucket, nil, withRetryConfig(&retryConfig{policy: RetryNever})) - attrs, err := client.CreateBucket(ctx, project, bucket, &BucketAttrs{}, nil) - if err != nil { - t.Fatalf("creating bucket: %v", err) + var ae *apierror.APIError + if errors.As(err, &ae) { + // We expect a 503/UNAVAILABLE error. For anything else including a nil + // error, the test should fail. + if ae.GRPCStatus().Code() != codes.Unavailable && ae.HTTPCode() != 503 { + t.Errorf("GetBucket: got unexpected error %v; want 503", err) + } } + }) +} - // Need the HTTP hostname to set up a retry test, as well as knowledge of - // underlying transport to specify instructions. - host := os.Getenv("STORAGE_EMULATOR_HOST") - endpoint, err := url.Parse(host) - if err != nil { - t.Fatalf("parsing endpoint: %v", err) +// Test that errors are wrapped correctly if retry happens until a timeout. +func TestRetryTimeoutEmulated(t *testing.T) { + transportClientTest(t, func(t *testing.T, project, bucket string, client storageClient) { + ctx := context.Background() + instructions := map[string][]string{"storage.buckets.get": {"return-503", "return-503", "return-503", "return-503", "return-503"}} + testID := createRetryTest(t, project, bucket, client, instructions) + ctx = callctx.SetHeaders(ctx, "x-retry-test-id", testID) + ctx, cancel := context.WithTimeout(ctx, 100*time.Millisecond) + defer cancel() + _, err := client.GetBucket(ctx, bucket, nil, idempotent(true)) + + var ae *apierror.APIError + if errors.As(err, &ae) { + // We expect a 503/UNAVAILABLE error. For anything else including a nil + // error, the test should fail. + if ae.GRPCStatus().Code() != codes.Unavailable && ae.HTTPCode() != 503 { + t.Errorf("GetBucket: got unexpected error: %v; want 503", err) + } } - var transport string - if _, ok := client.(*httpStorageClient); ok { - transport = "http" - } else { - transport = "grpc" + // Error should be wrapped so it's also equivalent to a context timeout. + if !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("GetBucket: got unexpected error %v, want to match DeadlineExceeded.", err) } + }) +} - et := emulatorTest{T: t, name: "testRetryNever", resources: resources{}, - host: endpoint} - et.create(map[string][]string{"storage.buckets.get": {"return-503"}}, transport) - ctx = callctx.SetHeaders(ctx, "x-retry-test-id", et.id) - _, err = client.GetBucket(ctx, attrs.Name, nil, withRetryConfig(&retryConfig{policy: RetryNever})) +// Test that errors are wrapped correctly if retry happens until max attempts. +func TestRetryMaxAttemptsEmulated(t *testing.T) { + transportClientTest(t, func(t *testing.T, project, bucket string, client storageClient) { + ctx := context.Background() + instructions := map[string][]string{"storage.buckets.get": {"return-503", "return-503", "return-503", "return-503", "return-503"}} + testID := createRetryTest(t, project, bucket, client, instructions) + ctx = callctx.SetHeaders(ctx, "x-retry-test-id", testID) + config := &retryConfig{maxAttempts: expectedAttempts(3), backoff: &gax.Backoff{Initial: 10 * time.Millisecond}} + _, err := client.GetBucket(ctx, bucket, nil, idempotent(true), withRetryConfig(config)) var ae *apierror.APIError if errors.As(err, &ae) { - // We espect a 503/UNAVAILABLE error. For anything else including a nil + // We expect a 503/UNAVAILABLE error. For anything else including a nil // error, the test should fail. if ae.GRPCStatus().Code() != codes.Unavailable && ae.HTTPCode() != 503 { t.Errorf("GetBucket: got unexpected error %v; want 503", err) } } + // Error should be wrapped so it indicates that MaxAttempts has been reached. + if got, want := err.Error(), "retry failed after 3 attempts"; !strings.Contains(got, want) { + t.Errorf("got error: %q, want to contain: %q", got, want) + } + }) +} + +// createRetryTest creates a bucket in the emulator and sets up a test using the +// Retry Test API for the given instructions. This is intended for emulator tests +// of retry behavior that are not covered by conformance tests. +func createRetryTest(t *testing.T, project, bucket string, client storageClient, instructions map[string][]string) string { + t.Helper() + ctx := context.Background() + + _, err := client.CreateBucket(ctx, project, bucket, &BucketAttrs{}, nil) + if err != nil { + t.Fatalf("creating bucket: %v", err) + } + + // Need the HTTP hostname to set up a retry test, as well as knowledge of + // underlying transport to specify instructions. + host := os.Getenv("STORAGE_EMULATOR_HOST") + endpoint, err := url.Parse(host) + if err != nil { + t.Fatalf("parsing endpoint: %v", err) + } + var transport string + if _, ok := client.(*httpStorageClient); ok { + transport = "http" + } else { + transport = "grpc" + } + + et := emulatorTest{T: t, name: t.Name(), resources: resources{}, host: endpoint} + et.create(instructions, transport) + t.Cleanup(func() { + et.delete() }) + return et.id } // createObject creates an object in the emulator and returns its name, generation, and diff --git a/storage/invoke.go b/storage/invoke.go index 1b52eb5d2c65..376be1adf1a4 100644 --- a/storage/invoke.go +++ b/storage/invoke.go @@ -70,8 +70,8 @@ func run(ctx context.Context, call func(ctx context.Context) error, retry *retry return internal.Retry(ctx, bo, func() (stop bool, err error) { ctxWithHeaders := setInvocationHeaders(ctx, invocationID, attempts) err = call(ctxWithHeaders) - if retry.maxAttempts != nil && attempts >= *retry.maxAttempts { - return true, err + if err != nil && retry.maxAttempts != nil && attempts >= *retry.maxAttempts { + return true, fmt.Errorf("storage: retry failed after %v attempts; last error: %w", *retry.maxAttempts, err) } attempts++ return !errorFunc(err), err diff --git a/storage/invoke_test.go b/storage/invoke_test.go index a89695a86734..d3549b235bb8 100644 --- a/storage/invoke_test.go +++ b/storage/invoke_test.go @@ -251,9 +251,9 @@ func TestInvoke(t *testing.T) { return test.finalErr } got := run(ctx, call, test.retry, test.isIdempotentValue) - if test.expectFinalErr && got != test.finalErr { + if test.expectFinalErr && !errors.Is(got, test.finalErr) { s.Errorf("got %v, want %v", got, test.finalErr) - } else if !test.expectFinalErr && got != test.initialErr { + } else if !test.expectFinalErr && !errors.Is(got, test.initialErr) { s.Errorf("got %v, want %v", got, test.initialErr) } wantAttempts := 1 + test.count