Skip to content

Commit

Permalink
(internal/retry) Simplify gRPC status code mapping of retry error
Browse files Browse the repository at this point in the history
gRPC v1.55.0 with a change in semantic for status.FromError such that the gRPC status of wrapped
errors is returned when defined. The implementation of GRPCStatus in internal/retry code was doing
just that. Removing GRPCStatus entirely makes the code much easier to understand: at the moment,
status.FromError calls GRPCStatus which in turns calls status.FromError and it's not immediately
obvious why this cannot result in infinite recursion.

A previous change I made to this code made sure this method never returns
status Code.OK (googleapis#8128), but I failed to realize that
since this project depends on gRPC 1.55 that already handles wrapped errors in status.FromError, we
can simply remove the implementation of `GRPCStatus` and let gRPC status.FromError handle
unwrapping.

Note that I barely had to change the tests, but there *IS* a slight change in behavior: the message
of the wrapping error is included in the gRPC error. I think it's fine, bet let me know if you think
otherwise (for the same reasons discussed in grpc/grpc-go#6150).
  • Loading branch information
atollena committed Jul 3, 2023
1 parent 2b99e4f commit 017c367
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 15 deletions.
10 changes: 0 additions & 10 deletions internal/retry.go
Expand Up @@ -20,8 +20,6 @@ import (
"time"

gax "github.com/googleapis/gax-go/v2"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
)

// Retry calls the supplied function f repeatedly according to the provided
Expand Down Expand Up @@ -76,11 +74,3 @@ func (e wrappedCallErr) Unwrap() error {
func (e wrappedCallErr) Is(err error) bool {
return e.ctxErr == err || e.wrappedErr == err
}

// GRPCStatus allows the wrapped error to be used with status.FromError.
func (e wrappedCallErr) GRPCStatus() *status.Status {
if s, ok := status.FromError(e.wrappedErr); ok {
return s
}
return status.New(codes.Unknown, e.Error())
}
7 changes: 2 additions & 5 deletions internal/retry_test.go
Expand Up @@ -88,7 +88,7 @@ func TestRetryPreserveError(t *testing.T) {
if g, w := got.Code(), codes.NotFound; g != w {
t.Errorf("got code %v, want %v", g, w)
}
wantMessage := "not found"
wantMessage := "retry failed with context deadline exceeded; last error: rpc error: code = NotFound desc = not found"
if g, w := got.Message(), wantMessage; g != w {
t.Errorf("got message %q, want %q", g, w)
}
Expand All @@ -111,10 +111,7 @@ func TestRetryWrapsErrorWithStatusUnknown(t *testing.T) {
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")
}
got, _ := status.FromError(err)
if g, w := got.Code(), codes.Unknown; g != w {
t.Errorf("got code %v, want %v", g, w)
}
Expand Down

0 comments on commit 017c367

Please sign in to comment.