Support error unwrapping (errors.As)

[dfawley]

Several options:

• Implement As on *Status so that users can do:

var s *status.Status
ok := errors.As(err, s)
// use s if ok

Note that *Status does not implement error, so this may be considered an anti-pattern. Another concern is that it would not be discoverable - how would users know to do this? Possibly through status package documentation.

• Export statusError, though there is no other reason to do so. (I don't like this.)
• Add a status.Unwrap(err) *Status function to do unwrapping. This seems pretty reasonable, though it should theoretically be unnecessary.
• Implement unwrapping support in status.FromError(). My only minor concern with this is that it would unwrap automatically, which may not be expected by users.

cc @jsm

[dfawley]

For the time being, we have decided to not pursue any solution here until:

1. error wrapping matures and is used more extensively, so we can determine if anything here truly necessary, and
2. a standard way of doing this has emerged (since we have at least three options), if so.

[FUSAKLA]

Hm, Ok I understand the concerns. Thanks for clearing this up.

What would you recommend as a workaround for the time being? Add the unwrapping to the go-grpc-prometheus package untill the time comes?

[mbyio]

If you're interested in another viewpoint:

Implement As on *Status

That seems like a random but helpful feature. I agree with waiting on this one until it's clearly worth the complexity.

Export statusError, though there is no other reason to do so. (I don't like this.)

I don't see why you would need to do that, and I think it is an anti-pattern.

Add a status.Unwrap(err) *Status function to do unwrapping.

I'm not really sure what that would accomplish? It seems like that would just reimplement errors.As.

Implement unwrapping support in status.FromError(). My only minor concern with this is that it would unwrap automatically, which may not be expected by users.

This would be my preference. Currently I'm planning to use interceptors to bring the GRPCStatus() up to the top level so the status package can read it.

I think this is the only option that is actually making anything new possible or impossible, the others are just helpers and don't change the actual functionality.

[dfawley]

@FUSAKLA,

What would you recommend as a workaround for the time being? Add the unwrapping to the go-grpc-prometheus package untill the time comes?

Can you describe your use case here? What code is wrapping grpc status errors and why, and what code needs to access wrapped grpc status errors and why?

@mbyio,

Implement As on *Status

That seems like a random but helpful feature. I agree with waiting on this one until it's clearly worth the complexity.

Apparently this option would cause a panic in errors, because *Status doesn't implement error:
https://golang.org/src/errors/wrap.go?s=1905:2057#L64

Note that this restriction is entirely unnecessary, but it is there intentionally. It might be possible to lobby the Go team to remove it.

Export statusError, though there is no other reason to do so. (I don't like this.)

I don't see why you would need to do that, and I think it is an anti-pattern.

If we did this, we could implement As on *StatusError, legally, and it would work with errors.As, but as I said, I don't like this option, either, as it adds a new type to the API because of one, uncommon, use case.

Add a status.Unwrap(err) *Status function to do unwrapping.

I'm not really sure what that would accomplish? It seems like that would just reimplement errors.As.

Adding status.Unwrap is probably the best option today, actually, because....

Implement unwrapping support in status.FromError().

This would be my preference.

Unfortunately, this would be a breaking behavior change, so this option is probably completely off the table.

status.Unwrap would be exactly like FromError, except that it would optionally unwrap if necessary.

[FUSAKLA]

@dfawley
I already managed to integrate the workaround to the go-grpc-prometheus interceptor I was having issues with. grpc-ecosystem/go-grpc-prometheus#84

The issue was that errors were wrapped using the github.com/pkg/errors package and the interceptor reported them as Unknown since they did not implement the gRPC status interface.
The workaround handles wrapping of that library and the new Go native wrapping added in 1.13.

[mbyio]

@dfawley

Yeah without making a breaking change, I'm not sure you need to make any code changes. It is trivial to fetch the status object (and associated info) manually:

resp, err := runRPC(ctx, req)
var grpcstatus interface{ GRPCStatus() *status.Status }
var code codes.Code 
if errors.As(err, &grpcStatus) {
    code = grpcstatus.GRPCStatus().Code()
}

If you add new functions or make any other changes, I think it will be confusing, because people might think GRPC is using those functions to retrieve the status. In other words, IMO, the status package should not encourage unwrapping if it isn't going to use that for the actual GRPC error response.

An alternative could be to define a new interface for errors to implement. Something like interface{ GRPCStatus2() *status.Status}. In the status package helper functions, if you can't cast to something with GRPCStatus() *status.Status, try calling errors.As for GRPCStatus2(). However, that is starting to get complicated, and you'd have to add a new error type people have to opt into to avoid breaking backwards compatibility.

I think the best way to address this without a breaking change would be to add interceptors to grpc_middleware that simplify this error conversion process (which for most casual users will be very confusing), and then document how to set them up in this package's docs. Similar to what @FUSAKLA did for that one interceptor. Maybe one interceptor for pkg/errors since that is extremely popular, and one for the stdlib package, and users can use both if they want to cover all their bases. The interceptors would search for a wrapped GRPCStatus() and pull it up to the top level.

As a user I'd strongly prefer a breaking change here, especially because the breakage is easy to undo with an interceptor if people want to opt-out. When monitoring middleware get involved, keeping everything in agreement about how to fetch GRPC errors gets complicated. But I recognize breaking is not always possible.

[jace-ys]

+1 on this feature request. Not sure how common my use case is, but would be nice to have this.

I use the grpc-gateway as a gRPC client to expose my gRPC service as a REST API. The gateway allows you customize the HTTPError to return a custom HTTP response from the gateway based on the error.

HTTPError has the following signature:

func HTTPError(ctx context.Context, mux *runtime.ServeMux, marshaler runtime.Marshaler, w http.ResponseWriter, r *http.Request, err error)

where the err error is produced by status.Errorf. It will nice to have the ability to unwrap this error / use errors.As and errors.Is to inspect the underlying error that's being wrapped by status.Errorf to perform custom handling.

In my case, I would like to be able to do this to handle json unmarshaling errors that occurs in the gateway:

func HTTPError(ctx context.Context, mux *runtime.ServeMux, marshaler runtime.Marshaler, w http.ResponseWriter, r *http.Request, err error) {
	var jsonUnmarshalTypeErr *json.UnmarshalTypeError
	if errors.As(err, &jsonUnmarshalTypeErr) {
		w.WriteHeader(http.StatusBadRequest)
		w.Write([]byte("Payload invalid: type mismatch"))
	}
        ...
}

Just here to highlight my use case, thanks! 🙂

[dfawley]

@jace-ys,

The gateway allows you customize the HTTPError to return a custom HTTP response from the gateway based on the error.

Where does that error come from? How does it get passed to HTTPError?

[jace-ys]

@jace-ys,

The gateway allows you customize the HTTPError to return a custom HTTP response from the gateway based on the error.

Where does that error come from? How does it get passed to HTTPError?

@dfawley,

Here's an example:

The error is produced by a generated RPC function in the grpc-gateway, using status.Errorf (source).

func request_ABitOfEverythingService_CreateBody_0(ctx context.Context, marshaler runtime.Marshaler, client ABitOfEverythingServiceClient, req *http.Request, pathParams map[string]string) (proto.Message, runtime.ServerMetadata, error) {
	var protoReq ABitOfEverything
	var metadata runtime.ServerMetadata

	newReader, berr := utilities.IOReaderFactory(req.Body)
	if berr != nil {
		return nil, metadata, status.Errorf(codes.InvalidArgument, "%v", berr)
	}
        // this returns a json error that is wrapped using status.Errorf
	if err := marshaler.NewDecoder(newReader()).Decode(&protoReq); err != nil && err != io.EOF {
		return nil, metadata, status.Errorf(codes.InvalidArgument, "%v", err)
	}

	msg, err := client.CreateBody(ctx, &protoReq, grpc.Header(&metadata.HeaderMD), grpc.Trailer(&metadata.TrailerMD))
	return msg, metadata, err

}

This RPC call then gets invoked by the grpc-gateway server, returning the error from the above function (source).

mux.Handle("POST", pattern_ABitOfEverythingService_CreateBody_0, func(w http.ResponseWriter, req *http.Request, pathParams map[string]string) {
	ctx, cancel := context.WithCancel(req.Context())
	defer cancel()
	inboundMarshaler, outboundMarshaler := runtime.MarshalerForRequest(mux, req)
	rctx, err := runtime.AnnotateContext(ctx, mux, req)
	if err != nil {
		runtime.HTTPError(ctx, mux, outboundMarshaler, w, req, err)
		return
	}
        // we are not able to unwrap this error and get the underlying cause
	resp, md, err := request_ABitOfEverythingService_CreateBody_0(rctx, inboundMarshaler, client, req, pathParams)
	ctx = runtime.NewServerMetadataContext(ctx, md)
	if err != nil {
		runtime.HTTPError(ctx, mux, outboundMarshaler, w, req, err)
		return
	}

	forward_ABitOfEverythingService_CreateBody_0(ctx, mux, outboundMarshaler, w, req, resp, mux.GetForwardResponseOptions()...)

})

So currently, when the error gets wrapped by status.Errorf, runtime.HTTPError is not able to unwrap this error and get the underlying cause for further processing (as seen in my previous comment). I'm just a user of grpc-gateway and not a contributor, so please correct me if I'm missing something! 🙂

On another note, I'm happy to help contribute to this feature once there's a clear direction on how this should be implemented (unless it's already done?).

[dfawley]

@jace-ys - this issue is about unwrapping errors to find grpc status errors within them. It sounds like what you're asking for is the ability to do the opposite: wrap arbitrary errors inside status errors. This is actually ~possible by using the error details, although admittedly not in a way that works transparently with Go's error wrapping. Unfortunately, we are not considering this kind of capability, because it would result in a status error that cannot be transmitted over the wire from a server to a client identically. (Errors are arbitrary types which may not be serializable.)

[jzbyers]

I definitely think this feature would be helpful for gRPC clients.

Say you have a gRPC client that is capable of making requests transactionally. If any of the requests return errors, it'd be nice to let the client wrap those errors so that it can provide more context about which request was executing when the server returned an error.

Ex.

if err := grpcClient.WithTransaction(ctx, func(ctx context.Context) error {
    // Do something
    _, err := grpcClient.DoSomething(ctx, &doSomethingRequest{})
    if err != nil {
    	return fmt.Errorf("error doing something: %w", err)
    }
    
    // Do something else
    _, err := grpcClient.DoSomethingElse(ctx, &doSomethingElseRequest{})
    if err != nil {
    	return fmt.Errorf("error doing something else: %w", err)
    }
    
    return nil
); err != nil {
    // Handle the error somehow
}

I'll probably use something similar to @FUSAKLA's approach here until this package supports some form of sustainable error wrapping, which I imagine should follow the Go community settling on an idiomatic approach to the problem. Probably a good idea to let the dust settle after the Go 1.13 change.

@dfawley for reference.

[dfawley]

Based on what @mbyio mentioned above, another solution here could be:

package status

type StatError interface { // needs a better name; Error already defined
  error
  GRPCStatus() *Status
}

Usage:

_, err := runRPC(req)  // err has "Unwrap() error" returning a grpc status error
var unwrap status.StatError
if !errors.As(wrapped, &unwrap) {
   t.Fatalf("unable tounwrap wrapped error %v", wrapped)
}
st, ok := status.FromError(unwrap)
// use st safely if ok

This works more "naturally" with errors.As, but it seems much harder to use than:

_, err := runRPC(req)
st, ok := status.Unwrap(err)
// use st safely if ok

[kazegusuri]

The status package provides high level functions for gRPC status and its error, so status.Unwrap seems very good to handle wrapped errors than using errors.As directly for me. It is so helpful if it's available.

[bpeake-illuscio]

+1 on this. errors.Is() and errors.As() are becoming more and more common as the default way to handle error inspection, would be very useful to be able to pass a wrapped status error to status.FromError() or errors.As().

[steeling]

I use an interceptor to check for aborted/conflict grpc errors and implement an automatic retry. However I have a lot of code in the server itself that is doing wrapping at each level to implement a form of stack traces.

Being able to unwrap an error would help me with this.

status.Code() method checks is already doing the interface check, why not just do an unwrap on that?

[ansel1]

There was a PR for this which was declined and closed: #4091

I don't really understand the commentors comment about why it was declined. The argument was something like, if the handler is returning an error without attempting to transform it to a specific Status, then the author of the controller will be expecting this error to result in an Unknown status code. If later, a library decides to implement GRPCStatus() in order to return a different status code, then the controller author could be surprised that their codes are changing.

But

1. that could happen today anyway, if the handler author doesn't wrap those errors floating up from the library
2. if the handler wants more explicit control over the Status, they always have that ability. they can just replace the library error with their own

But today, when they do that, they lose all the rich context information attached to the original error. If they are using an intercepter for transaction logging, that middleware will never get to see the original error object, and all that interesting context will be lost. Even if I use a custom error implementation which holds the original error, and implement GRPCStatus(), I can't then wrap that error any further, because grpc won't use errors.As() to unwrap down to by Status holder.

After so many years of errors.Is() and errors.As() in the ecosystem, it's very surprising behavior that grpc doesn't use errors.As() to find GRPCStatus() implementors.

[dfawley]

I believe #6031 implemented this.