Application cannot return errors with gprc.Code(err) != codes.Unknown, errors are noisy, codes have no Stringer

[tv42]

$ cd $GOPATH/src/github.com/grpc/grpc-common/go
$ git diff
diff --git i/go/greeter_server/main.go w/go/greeter_server/main.go
index c7fa06a..c7682cb 100644
--- i/go/greeter_server/main.go
+++ w/go/greeter_server/main.go
@@ -40,6 +40,7 @@ import (
        pb "github.com/grpc/grpc-common/go/helloworld"
        "golang.org/x/net/context"
        "google.golang.org/grpc"
+       "google.golang.org/grpc/codes"
 )

 const (
@@ -51,7 +52,7 @@ type server struct{}

 // SayHello implements helloworld.GreeterServer
 func (s *server) SayHello(ctx context.Context, in *pb.HelloRequest) (*pb.HelloReply, error) {
-       return &pb.HelloReply{Message: "Hello " + in.Name}, nil
+       return nil, grpc.Errorf(codes.DataLoss, "too grumpy to greet")
 }

 func main() {
$ go run greeter_server/main.go &
[1] 12775
$ go run greeter_client/main.go 
2015/03/03 13:53:30 could not greet: rpc error: code = 2 desc = "rpc error: code = 15 desc = \"too grumpy to greet\""
exit status 1

I expected to see code=15, now I see code=2 wrapping a code=15.

My reading of the source (https://github.com/grpc/grpc-go/blob/master/server.go#L246 , https://github.com/grpc/grpc-go/blob/master/rpc_util.go#L227 ) says convertCode should have if err, ok := err.(rpcError); ok { return err.code }.

That still leaves the textual double-wrapping; it seems that should not be done on the server-side for rpcError, just send rpcError.desc if the error is a grpcError.

Also, could there be a little less textual wrapping? And could those codes get a go stringer on them? Here's what might be ideal:

could not greet: rpc error: DataLoss: too grumpy to greet

and only start doing quoting if the message is "hostile", as in contains \n etc. That would require defining a safe set of runes.

[iamqizhao]

This is good catch. I planned to fix this before releasing but forgot to do
that. will send out a PR to fix this.

On Tue, Mar 3, 2015 at 2:08 PM, Tv notifications@github.com wrote:

$ cd $GOPATH/src/github.com/grpc/grpc-common/go
$ git diffdiff --git i/go/greeter_server/main.go w/go/greeter_server/main.goindex c7fa06a..c7682cb 100644--- i/go/greeter_server/main.go+++ w/go/greeter_server/main.go@@ -40,6 +40,7 @@ import ( pb "github.com/grpc/grpc-common/go/helloworld" "golang.org/x/net/context" "google.golang.org/grpc"+ "google.golang.org/grpc/codes" )
const (@@ -51,7 +52,7 @@ type server struct{}
// SayHello implements helloworld.GreeterServer func (s _server) SayHello(ctx context.Context, in *pb.HelloRequest) (_pb.HelloReply, error) {- return &pb.HelloReply{Message: "Hello " + in.Name}, nil+ return nil, grpc.Errorf(codes.DataLoss, "too grumpy to greet") }
func main() {
$ go run greeter_server/main.go &[1] 12775
$ go run greeter_client/main.go 2015/03/03 13:53:30 could not greet: rpc error: code = 2 desc = "rpc error: code = 15 desc = "too grumpy to greet""exit status 1

I expected to see code=15, now I see code=2 wrapping a code=15.

My reading of the source (
https://github.com/grpc/grpc-go/blob/master/server.go#L246 ,
https://github.com/grpc/grpc-go/blob/master/rpc_util.go#L227 ) says
convertCode should have if err, ok := err.(rpcError); ok { return
err.code }.

That still leaves the textual double-wrapping; it seems that should not be
done on the server-side for rpcError, just send rpcError.desc if the
error is a grpcError.

Also, could there be a little less textual wrapping? And could those codes
get a go stringer on them? Here's what might be ideal:

could not greet: rpc error: DataLoss: too grumpy to greet

and only start doing quoting if the message is "hostile", as in contains
\n etc. That would require defining a safe set of runes.

—
Reply to this email directly or view it on GitHub
#92.

[iamqizhao]

okay, this is actually a misuse. We do not expect users to set error code in their service handler implementation. They should simply return nil, fmt.Errorf("too grumpy to greet"). We should have had an example for this.

But I probably should handle this case as well to obey whatever error code the server application sets if it does. Let me think ...

[tv42]

If what you're saying is going to hold, then it sounds like grpc.Errorf should not be exported.

However, consider the implications: if Codes are only supposed to be created by convertCode/toRPCErr, that means the only way to return a codes.AlreadyExists from something that is not an OS-level file system operation or such is to use platform-dependent erorrs like syscall.EEXIST (attempt to make os.IsExist(err)==true). At that point, it seems like the Code mechanism is unusable, and people would be better off with enum error types in their response messages. And in that case, why are there so many Codes?

And yes, more clarity into intents of the mechanism is most welcome.

[iamqizhao]

On Wed, Mar 4, 2015 at 6:41 PM, Tv notifications@github.com wrote:

If what you're saying is going to hold, then it sounds like grpc.Errorf
should not be exported.

However, consider the implications: if Codes are only supposed to be
created by convertCode/toRPCErr, that means the only way to return a
codes.AlreadyExists from something that is not an OS-level file system
operation or such is to use platform-dependent erorrs like syscall.EEXIST
(attempt to make os.IsExist(err)==true). At that point, it seems like the
Code mechanism is unusable, and people would be better off with enum error
types in their response messages. And in that case, why are there so many
Codes?

And yes, more clarity into intents of the mechanism is most welcome.

Yup, I will have a pull request to fix this shortly.

—
Reply to this email directly or view it on GitHub
#92 (comment).