From dff8bf937153823e3414d3d66c4241b53703cfa5 Mon Sep 17 00:00:00 2001 From: Antoine Tollenaere Date: Fri, 16 Jun 2023 11:50:33 +0200 Subject: [PATCH 1/2] fix: Never return nil from GRPCStatus() in internal/retry An error's GRPCStatus() method should never returns nil, which maps to a status with Code.OK (https://github.com/grpc/grpc-go/blob/642dd63a85275a96d172f446911fd04111e2c74c/internal/status/status.go#L71-L76). Instead it should return an actual error code, such as Status.Unknown. This addresses https://github.com/googleapis/google-cloud-go/issues/8127. --- internal/retry.go | 3 ++- internal/retry_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/internal/retry.go b/internal/retry.go index 2943a6d0b457..fce0c48ed21a 100644 --- a/internal/retry.go +++ b/internal/retry.go @@ -17,6 +17,7 @@ package internal import ( "context" "fmt" + "google.golang.org/grpc/codes" "time" gax "github.com/googleapis/gax-go/v2" @@ -81,5 +82,5 @@ func (e wrappedCallErr) GRPCStatus() *status.Status { if s, ok := status.FromError(e.wrappedErr); ok { return s } - return nil + return status.New(codes.Unknown, e.Error()) } diff --git a/internal/retry_test.go b/internal/retry_test.go index 771cb26ffca4..1c26dcbcc394 100644 --- a/internal/retry_test.go +++ b/internal/retry_test.go @@ -93,3 +93,29 @@ func TestRetryPreserveError(t *testing.T) { t.Errorf("got message %q, want %q", g, w) } } + +func TestRetryWrapsErrorWithStatusUnknown(t *testing.T) { + // When retrying on an error that is not a grpc error, make sure to return + // a valid gRPC status. + err := retry(context.Background(), gax.Backoff{}, + func() (bool, error) { + return false, errors.New("test error") + }, + func(context.Context, time.Duration) error { + return context.DeadlineExceeded + }) + if err == nil { + t.Fatalf("unexpectedly got nil error") + } + wantError := "retry failed with context deadline exceeded; last error: test error" + if g, w := err.Error(), wantError; g != w { + t.Errorf("got error %q, want %q", g, w) + } + got, ok := status.FromError(err) + if !ok { + t.Fatal("expected error to implement a gRPC status") + } + if g, w := got.Code(), codes.Unknown; g != w { + t.Errorf("got code %v, want %v", g, w) + } +} From 0cdc4bed3a3a3756074f185e11ab8a79edbf850c Mon Sep 17 00:00:00 2001 From: Noah Dietz Date: Fri, 16 Jun 2023 20:33:59 +0000 Subject: [PATCH 2/2] fix import formatting --- internal/retry.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/retry.go b/internal/retry.go index fce0c48ed21a..290e744b7d36 100644 --- a/internal/retry.go +++ b/internal/retry.go @@ -17,10 +17,10 @@ package internal import ( "context" "fmt" - "google.golang.org/grpc/codes" "time" gax "github.com/googleapis/gax-go/v2" + "google.golang.org/grpc/codes" "google.golang.org/grpc/status" )