protoc-gen-go/grpc: Add null guard for gRPC connection

[yagotome]

Generated code for gRPC Service Client does an RPC call by deferencing a connection pointer (grpc.ClientConn) without checking whether it is nil.

In this PR, an error is returned if that connection pointer is nil, so that no nil pointer dereference is thrown (panic) due to nil connection.

[dsnet]

\cc @dfawley

[puellanivis]

It would possibly make more sense to test this during the NewXYZClient(…) call, rather than at every single function call. (The underlying pointer only need be checked once, after all.)

😕 It’s hard to say what the correct behavior here really should be because there is no way to actually recover or deal with the error. Either there was a fundamental mistake in the calling code (failing to test an error from grpc.Dial, deliberately passing a nil value, or instantiating a new client with zero values), or something has fundamentally violated a contractual obligation (grpc.Dial returned a nil connection, and a nil error).

[yagotome]

It would possibly make more sense to test this during the NewXYZClient(…) call, rather than at every single function call. (The underlying pointer only need be checked once, after all.)

It’s hard to say what the correct behavior here really should be because there is no way to actually recover or deal with the error. Either there was a fundamental mistake in the calling code (failing to test an error from grpc.Dial, deliberately passing a nil value, or instantiating a new client with zero values), or something has fundamentally violated a contractual obligation (grpc.Dial returned a nil connection, and a nil error).

@puellanivis I agree that usually it makes more sense to test it before calling the constructor. On the other hand, since nil is accepted (no panic in constructor call), it turns out that the struct gets in a inconsistent state, allowing method calls that crashes (panic) the running goroutine, as you mentioned latter.

If we think about how expensive is adding a null check for each RPC call, I would guess that it increases insignificant CPU time usage at all. However, we may think why would it be necessary? I would say that consistency is enough, altough a panic in constructor would solve that problem. So, one reason to put it in each gRPC stub method is that if I would like to start my gRPC channel (HTTP/2 connections) to some Server only when I need to use it, either because I cannot spend resource with connections that is not going to be used yet or because I knew the server was not available yet at that moment (but now it is possibly available). In that case, I would need to check for connection nullability in each place (in code) I call a gRPC stub method. Well, if the gRPC stub method returned an error indicating nil connection (a precondition failed), I would be able to rely on that for error handling, avoiding double error handling for each place.

[dfawley]

Ideally this would fail at client creation, but given the API, there is no way to fail there except with a panic. That said, as @puellanivis said, this situation can only happen as a result of programmer error. So it seems there are three options:

• Panic at use (behavior today)
• Error at use (this PR)
• Panic at creation

Of the three, possibly my least favorite is today's behavior. Panicking at creation is strictly better, as it moves the failure closer to the mistake. I'm fine with panics as a result of programmer error, but the main problem with changing to panic at creation is that we would be introducing a new panic. If the client is unused, we'd be making existing code that was safe (albeit strange) panic, e.g.:

cc, err := grpc.Dial(...)
foo := NewFooClient(cc)
if err != nil {
  return err
}

So, I don't want to change the code to panic at creation. That leaves erroring at use. This option, I feel like, is just papering over programmer error.

@yagotome, this use case is exactly how gRPC clients are supposed to work (but not how grpc-go works, unfortunately). The initial state of the client is IDLE until the first RPC is attempted, at which point the client starts connecting. Updating Go to this behavior is tracked here: grpc/grpc-go#1786.

However, I don't see how checking for nil ahead of time vs. for a special error after helps save any code or complexity. The fact that this expands the API of the stub is concerning. Also, the error code used, FailedPrecondition is explicitly called out in our spec as never produced by grpc: https://github.com/grpc/grpc/blob/master/doc/statuscodes.md.

IMO we should leave things as they are, and work toward supporting idleness in grpc-go instead.

[puellanivis]

My concern about checking it at every function call is less concerned with performance. A smart compiler should be able to elide the code where the condition is never possible. And actually, I’m pretty sure, the compiler conditionally generates code to test for nil in order to implement the panic, anyways.

My concern is centrally that it should be a programming fault for such an error path to ever even be possible. Safety guards can be good, but runtime panics can sometimes be the correct choice. 🤷‍♀️

P.S.: Yeah, the documentation says FAILED_PRECONDITION cannot be generated within the library, so we really should try to find something more appropriate.

[yagotome]

@dfawley alright! I agree that introducing a new panic is harmful, that's why I am not proposing that.

@yagotome, this use case is exactly how gRPC clients are supposed to work (but not how grpc-go works, unfortunately). The initial state of the client is IDLE until the first RPC is attempted, at which point the client starts connecting. Updating Go to this behavior is tracked here: grpc/grpc-go#1786.

I wasn't aware of that open issue. That's exactly what I would like to have 🙂
However, would it be possible that feature is implemented in v1? Otherwise, i.e. if this is coming in v2, is v1 going to keep this current panic behavior? 😕

However, I don't see how checking for nil ahead of time vs. for a special error after helps save any code or complexity.

The scenario is that if I tried to connect before each RPC, I would need to check for err != nil twice (one for connection error and other for RPC's).

Also, the error code used, FailedPrecondition is explicitly called out in our spec as never produced by grpc:

My fault! If we go ahead with this PR, I will replace with a proper error.

IMO we should leave things as they are, and work toward supporting idleness in grpc-go instead.

Totally agree! Let's focus on supporting idleness in grpc-go!

That said, what about v1/v2's question? Should that panic be kept while idleness is not released?

[dsnet]

@dfawley may correct me here, but I'm not aware of a v2 effort for gRPC.

The discussion about v2 on this repository is specifically about the Go protobuf implementation, which does not cover the grpc generator. The fact that the grpc generator is part of this repository is more for historical reasons and we intend for it to eventually be part of the grpc-go repository in the future.

[dfawley]

@dsnet, correct, we have no plans for a gRPC-Go v2.

@yagotome while we can't change the default behavior of grpc.Dial, we intend to add grpc.NewClient that will have this idleness behavior (in 1.x). For now I do think we should leave the current panic in place, as explained in the comment above.

[yagotome]

@dfawley @dsnet I agree with you! It's better keeping current panic since grpc.NewClient will have that idleness behavior soon.

Closing this PR therefore.