Server nil returns cause problems

[ashishgandhi]

In Go it is pretty idiomatic to return nil, err from a func when we encounter an error when the return values are a pointer and an error. The servers fail to marshal this nil while sending back a response to the client; and fatal logs. I think it might be better to do new(T) for the user before marshaling.

[iamqizhao]

I have no idea what func you are talking about. Can you elaborate?

[ashishgandhi]

Apologies, that was very vague of me. Here's more details.

ProtoBuf

syntax = "proto3";

package test;

service Nop {
  rpc Do(Empty) returns (Empty) {}
}

message Empty {}

Server

package main

import (
    "flag"
    "log"
    "net"
    "test"

    "golang.org/x/net/context"
    "google.golang.org/grpc"
)

var addr = flag.String("addr", ":1337", "listen addr")

func main() {
    flag.Parse()
    s := grpc.NewServer()
    test.RegisterNopServer(s, &server{})

    l, err := net.Listen("tcp", *addr)
    if err != nil {
        log.Fatalf("Cannot listen on %s: %v", *addr, err)
    }
    log.Fatalf("Server stopped: %v", s.Serve(l))
}

type server struct{}

// XXX: If you do this the server crashes. Instead, if you do a
//
//    return new(test.Empty), nil
//
// then life is okay. The error being nil or not does not make
// a difference.
func (*server) Do(context.Context, *test.Empty) (*test.Empty, error) { return nil, nil }

Any client is fine.

[xiang90]

@ashishgandhi It works fine I believe. You need to return the error correctly, not nil response + nil error.

[iamqizhao]

what prevents you from doing:

func (_server) Do(context.Context, *test.Empty) (_test.Empty, error) {
return new(test.Empty), nil
}
?

[ashishgandhi]

Well, nothing prevents me from doing that. But it'd be nice to not crash the server because of this? Same reason why the standard library http package does not crash even if you panic.

[iamqizhao]

I would think crash is right thing to do because this is a fatal error due to the local operations.

[theraphim]

I've just wasted a few hours on this, because in my case crash was happening without any diagnostic info or anything (just exit 1, I suppose).

[songshine]

I got this error:

grpc: Server failed to encode response proto: Marshal called with nil

Refer to https://github.com/grpc/grpc-go/blob/master/rpc_util.go#L265, the nil message case has been considered. So the msg is not a nil interface{} here but the real object of msg is nil. Shouldn't this be fixed?

[menghanl]

So the msg is not a nil interface{} here but the real object of msg is nil.

@songshine Can you give an example for this? Or a reproduction of the error?

[songshine]

@menghanl please think about the example from @ashishgandhi

func (*server) Do(context.Context, *test.Empty) (*test.Empty, error) { return nil, nil }

so when the code run into the line https://github.com/grpc/grpc-go/blob/master/rpc_util.go#L265,

if msg != nil {
...
}

the msg is (*test.Empty)(nil), but msg != nil will return true. This is an expected behavior?

[menghanl]

I think the fix should be

• remove the Fatalf in server.go and return the error

So that the behavior will be, if the error returned by service handler is nil, the response will be passed to Codec to do Marshal, even if the response is nil.

In the case of using protobuf and return nil, nil, the marshal will still fail with Marshal called with nil, and the user will be notified of the error, but not a Fatalf.