Add status package for reporting gRPC status and errors #1156
Conversation
transport/transport_test.go
Outdated
| if err != io.EOF { | ||
| t.Fatalf("Got err %v, want <EOF>", err) | ||
| } | ||
| if st, ok := s.status.(status.Status); !ok || st.Code() != codes.Internal { |
There was a problem hiding this comment.
Do we need this assertion?
There was a problem hiding this comment.
No, this is vestigial from a previous iteration. Removed.
| @@ -652,7 +653,7 @@ func (s *Server) processUnaryRPC(t transport.ServerTransport, stream *transport. | |||
| stream.SetSendCompress(s.opts.cp.Type()) | |||
| } | |||
| p := &parser{r: stream} | |||
| for { | |||
| for { // TODO: delete | |||
There was a problem hiding this comment.
It has no effect. It never reaches the end, continues, or breaks.
test/end2end_test.go
Outdated
| failAppUA = "fail-this-RPC" | ||
| detailedError = status.ErrorProto(&spb.Status{ | ||
| Code: int32(codes.DataLoss), | ||
| Message: "missing expected user-agent", |
There was a problem hiding this comment.
Change this error message?
If an error implemented by the status package is returned from a service handler, the server will transmit a rich status message in the "grpc-status-details-bin" trailing metadata field if any detailed data is attached to the error. Client-side, we will decode them if present in the server's response and return them to the user code performing the RPC. This is backward compatible with the existing errors supported by the grpc package. However, the grpc.Errorf, grpc.Code and grpc.ErrorDesc functions for managing errors are now deprecated; status.Errorf and status.Status type asserions should be used instead.
|
|
||
| // Status provides access to grpc status details and is implemented by all | ||
| // errors returned from this package except nil errors, which are not typed. | ||
| // Note: gRPC users should not implement their own Statuses. Custom data may |
There was a problem hiding this comment.
If users should not implement this interface, why is it an interface at all?
It seems like you could just as easily export some concrete type, and then you wouldn't have to contend with the (inevitable) inappropriate mocks of this interface.
Something like:
type Status struct {
…
}
type statusError struct { *Status }
func (s *statusError) Error() string { … }
func (s *Status) Err() error { return statusError{s} }
There was a problem hiding this comment.
The rationale for the interface is:
- so the package can return multiple different things implementing this type (an okStatus as well as a statusError).
- so statusError can be unexported (essential to avoid typed-nil-error problems), implement both error and Status accessors (convenient but not essential), have documentation for its methods, and not be visible externally.
I agree there are other options here, and PR #1171 is essentially what you're suggesting. I'm not convinced this version is "wrong", per se, but I do generally agree with the desire to avoid defining interfaces when they aren't strictly necessary.
menghanl
added
the
Type: API Change
As of grpc/grpc-go#1156, the gRPC errors package is a separate self-contained pair of packages: - google.golang.org/grpc/status for manipulating Status / errors - google.golang.org/grpc/codes for error code values. This means that pulling in gRPC errors no longer pulls in the whole of gRPC, which removes one of the main reasons why we had our own Trillian errors package. So shift to use gRPC errors throughout the codebase.
As of grpc/grpc-go#1156, the gRPC errors package is a separate self-contained pair of packages: - google.golang.org/grpc/status for manipulating Status / errors - google.golang.org/grpc/codes for error code values. This means that pulling in gRPC errors no longer pulls in the whole of gRPC, which removes one of the main reasons why we had our own Trillian errors package. So shift to use gRPC errors throughout the codebase.
Fixes #1122
If an error implemented by the status package is returned from a service
handler, the server will transmit a rich status message in the
"grpc-status-details-bin" trailing metadata field if any detailed data is
attached to the error. Client-side, we will decode them if present in the
server's response and return them to the user code performing the RPC.
This is backward compatible with the existing errors supported by the grpc
package. However, the grpc.Errorf, grpc.Code and grpc.ErrorDesc functions for
managing errors are now deprecated; status.Errorf and status.FromError should
be used instead on the server and client respectively.