Support error matching (errors.Is) and error wrapping (status.Errorf("%w"))

[sfllaw]

This feature request is related to #2934, but does not suggest that status.Status implement the error interface.

Use case(s) - what problem will this feature solve?

internal/status.Errorf, status.Errorf, functions wrap an error with its text message using fmt.Sprintf:

grpc-go/internal/status/status.go

Lines 66 to 69 in 42e450f

	// Errorf returns Error(c, fmt.Sprintf(format, a...)).
	func Errorf(c codes.Code, format string, a ...interface{}) error {
	return Err(c, fmt.Sprintf(format, a...))
	}

grpc-go/status/status.go

Lines 61 to 64 in 754ee59

	// Errorf returns Error(c, fmt.Sprintf(format, a...)).
	func Errorf(c codes.Code, format string, a ...interface{}) error {
	return Error(c, fmt.Sprintf(format, a...))
	}

Unfortunately, by discarding the error, it makes errors.Is tests impossible to use:

ctx, cancel := context.WithCancel(context.Background())
cancel() // Cancel the context so GetBook will immediately return context.Canceled.
book, err := client.GetBook(ctx, &pb.GetBookRequest{Title: "The Go Programming Language"})
fmt.Print(errors.Is(err, ctx.Err()) // Expect true but actually false

Proposed Solution

internal/status.Error should implement the errors.Unwrap interface:

// Error wraps a pointer of a status proto. It implements error and Status,
// and a nil *Error should never be returned by this package.
type Error struct {
	e   *spb.Status
	err error
}

// Unwrap implements the errors.Unwrap interface.
func (e *Error) Unwrap() error {
	if e == nil {
		return nil
	}
	return e.err
}

Implementing errors.Is might be a bit subtle, because the existing behavior only cares that the *spb.Status field contain the same protobuf representations:

// Is implements the errors.Is interface.
func (e *Error) Is(target error) bool {
	if t, ok := target.(Error); ok {
		// Errors are equal when their *spb.Status messages are equal.
		return proto.Equal(e.e, t.e)
	}
	return false
}

Obviously, status.Errorf should support the %w verb to match fmt.Errorf, such that:

switch err := db.QueryRow("SELECT id FROM books WHERE title = $1", req.GetTitle()).Scan(&id); {
case err == nil:
	return &GetBookResponse{Id: id, Title: req.GetTitle())
case errors.Is(err, sql.ErrNoRows):
	return nil, status.Errorf(codes.NotFound, "title %q: %w", title, err)
default:
	return nil, status.Errorf(codes.Internal, "title %q: %w", title, err)
}

This means that internal/status.Errorf needs to properly wrap the error:

// Errorf returns an Error formatted like fmt.Errorf.
func Errorf(c codes.Code, format string, a ...interface{}) error {
	e := fmt.Errorf(format, a...)
	err := Err(c, e.Error())
	err.err = e.Unwrap()
	return err
}

and that status.Errorf should delegate to this function:

// Errorf returns an Error formatted like fmt.Errorf.
func Errorf(c codes.Code, format string, a ...interface{}) error {
	return status.Errorf(c, format, a...)
}

Embedding errors in status.Status

Note that uses of status.FromError and status.FromContextError probably need to be slightly rewritten. For example:

grpc-go/clientconn.go

Lines 546 to 563 in 4eb418e

	// waitForResolvedAddrs blocks until the resolver has provided addresses or the
	// context expires. Returns nil unless the context expires first; otherwise
	// returns a status error based on the context.
	func (cc *ClientConn) waitForResolvedAddrs(ctx context.Context) error {
	// This is on the RPC path, so we use a fast path to avoid the
	// more-expensive "select" below after the resolver has returned once.
	if cc.firstResolveEvent.HasFired() {
	return nil
	}
	select {
	case <-cc.firstResolveEvent.Done():
	return nil
	case <-ctx.Done():
	return status.FromContextError(ctx.Err()).Err()
	case <-cc.ctx.Done():
	return ErrClientConnClosing
	}
	}

needs to be rewritten so that:

	case <-ctx.Done():
		err := ctx.Err()
		code := status.FromContextError(err).Code()
		return status.Errorf(code, "%w", err)
}

However, this seems a bit clunky because internal/status.Status.Err creates a new error each time.

We might consider embedding an error in internal/status.Status if it was created from an error:

// Status represents an RPC status code, message, and details.  It is immutable
// and should be created with New, Newf, or FromProto.
type Status struct {
	s   *spb.Status
	err error
}

// Err returns an immutable error representing s; returns nil if s.Code() is OK.
func (s *Status) Err() error {
	if s.Code() == codes.OK {
		return nil
	}
	return &Error{e: s.Proto(), err: s.err}
}

// GRPCStatus returns the Status represented by e.
func (e *Error) GRPCStatus() *Status {
	s := FromProto(e.e)
	s.err = e.err
	return s
}

[easwars]

This seems to be dup of #3115.

[sfllaw]

@easwars This request is actually to make #2868 compatible with status.Errorf without needing to introduce different semantics or types, as proposed by @dfawley in #2934. Can we please discuss this, as #3115 is locked?

[easwars]

@dfawley : Thoughts?

[dfawley]

internal/status.Error should implement the errors.Unwrap interface:

We cannot wrap errors inside a status, since they may not be serializable, as mentioned in #3115.

[sfllaw]

@dfawley internal/status.Error and Errorf don’t return an internal/status.Status, they return an error:

grpc-go/internal/status/status.go

Lines 61 to 69 in 6c9e30c

	// Err returns an error representing c and msg. If c is OK, returns nil.
	func Err(c codes.Code, msg string) error {
	return New(c, msg).Err()
	}
	// Errorf returns Error(c, fmt.Sprintf(format, a...)).
	func Errorf(c codes.Code, format string, a ...interface{}) error {
	return Err(c, fmt.Sprintf(format, a...))
	}

I suggested that we might want to keep track of the error in internal/status.Status but there is a tradeoff there. If that happens, then you are correct: it won’t round-trip to its protobuf representation and back.

Nevertheless, my proposal was to make status.Errorf("%w", err) construct a internal/status.Error that implements errors.Unwrap.

[dfawley]

Nevertheless, my proposal was to make status.Errorf("%w", err) construct a internal/status.Error that implements errors.Unwrap.

Right, and because it cannot round-trip, this wrapping will not be allowed. You can use the Details instead if you want to embed something inside a gRPC status error, which can be transmitted between server and client.

If you really wanted, you could make your own type, compatible with our status package, that does support Unwrap. E.g.

type myGRPCError struct { ... }

func (e *myGRPCError) Error() string { ... }  // implements error
func (e *myGRPCError) GRPCStatus() *status.Status { ... } // implements grpc status conversion
func (e *myGRPCError) Unwrap() error { ... } // implements error unwrapping

This would be compatible with the gRPC library -- i.e. your service handlers could return an error of this type and the *status.Status it converts to would be transmitted properly over the wire.

[sfllaw]

Right, and because it cannot round-trip, this wrapping will not be allowed.

I understand why we want internal/status.Status to round-trip, but I am puzzled as to why internal/status.Error needs to round-trip? It does not implement the proto.Message interface, so this should not be a problem.

You can use the Details instead if you want to embed something inside a gRPC status error, which can be transmitted between server and client.

Maybe you are thinking that my feature request is to magically tunnel a wrapped server error to the client? This is not what I’m asking for. Obviously, wrapped errors cannot survive the trip between the server and the client: no Go program can send errors across process boundaries. To clarify, if this proposal were implemented, we should add doc comments to explain that if a client calls Unwrap on a server error, it will always return nil.

This feature request is for the gRPC library to support wrapped errors within the same process. This is useful for:

• handling context.Context.Err errors with errors.Is, like context.Canceled and context.DeadlineExceeded;
• implementing interceptors that handle certain classes of errors, using errors.Is and errors.As.

If you really wanted, you could make your own type, compatible with our status package, that does support Unwrap.

Unfortunately, this does not solve the problem listed in the description. Even if I made a custom error type, gprc.ClientConn.waitForResolvedAddrs would still eat its ctx.Err.

In our case, the client wants to distinguish between a codes.CANCELED error coming from the server and the expected error when the client’s own context is cancelled by testing errors.Is(err, ctx.Err()).

[puellanivis]

In our case, the client wants to distinguish between a codes.CANCELED error coming from the server and the expected error when the client’s own context is cancelled by testing errors.Is(err, ctx.Err()).

In this case you should be able to simply test ctx.Err() != nil.

[dfawley]

internal/status.Error is a go-error representation of a status message.

This feature request is for the gRPC library to support wrapped errors within the same process.

These are not intended use cases for status errors. There are other ways to do what you want. E.g. you can return any custom error type from your method handlers if you have an interceptor that needs to interpret them.

In our case, the client wants to distinguish between a codes.CANCELED error coming from the server and the expected error when the client’s own context is cancelled by testing errors.Is(err, ctx.Err()).

This sounds more like #3328. Since it is not possible to distinguish these cases, you could make your server return a different status code -- one the gRPC library cannot produce:

https://github.com/grpc/grpc/blob/master/doc/statuscodes.md

Or, as @puellanivis suggested, you could check the context directly in this case.