grpc: document ctx closure best practices #600
Conversation
protoc-gen-go/grpc/grpc.go
Outdated
| @@ -303,6 +303,19 @@ func (g *grpc) generateClientMethod(servName, fullServName, serviceDescVar strin | |||
| if method.GetOptions().GetDeprecated() { | |||
| g.P(deprecationComment) | |||
| } | |||
| streamType := unexport(servName) + methName + "Client" | |||
| g.P(fmt.Sprintf(`// The ctx passed in will be used for the lifetime of this object. | |||
There was a problem hiding this comment.
This is not a godoc-compliant comment.
// <method> initiates a new streaming RPC to the server, yadda yadda.However, the type that owns this method is not exported (unexport(servName) below). This should be documented in the interface instead, or else it will never be seen. Note that those include the comments from the proto file, so we'll have to be smart about how to keep pulling those in but also document the Go implementation as well.
jeanbza
force-pushed
the
document_ctx_closure
branch
from
32e0e19
to
61399b7
Compare
|
@dfawley PTAL - moved to interface, use pointer instead of dupe doc |
protoc-gen-go/grpc/grpc.go
Outdated
| g.P() | ||
| g.P(fmt.Sprintf(`// Client API for %s service. | ||
| // | ||
| // For semantics around ctx use and closing/ending RPCs, please refer to https://godoc.org/google.golang.org/grpc#ClientConn.NewStream.`, servName)) |
There was a problem hiding this comment.
Nit: "closing/ending streaming RPCs".
|
@dsnet Could you describe what the problem with travis is? It seems to be stuck on some commit? |
|
@jadekler I'm pretty sure you need to regenerate these pb.go files and commit them along with this PR, as you've changed how they are generated. |
|
Aha - thanks, done. |
protoc-gen-go/grpc/grpc.go
Outdated
| @@ -160,8 +160,9 @@ func (g *grpc) generateService(file *generator.FileDescriptor, service *pb.Servi | |||
| deprecated := service.GetOptions().GetDeprecated() | |||
|
|
|||
| g.P() | |||
| g.P("// Client API for ", servName, " service") | |||
| g.P() | |||
| g.P(fmt.Sprintf(`// Client API for %s service. | |||
There was a problem hiding this comment.
According to https://golang.org/doc/effective_go.html#commentary, this should probably be written as:
// TestClient is the client API for the Test service.
//
// For semantics around resource cleanup, please refer to
// https://godoc.org/google.golang.org/grpc#ClientConn.NewStream.There was a problem hiding this comment.
Also, shouldn't a similar comment be on the server equivalent of this?
There was a problem hiding this comment.
Done
And, although I think you're right, the server's case + recommendations are different and should probably live in a different PR.
protoc-gen-go/grpc/grpc.go
Outdated
| g.P() | ||
| g.P(fmt.Sprintf(`// Client API for %s service. | ||
| // | ||
| // For semantics around ctx use and closing/ending streaming RPCs, please refer to https://godoc.org/google.golang.org/grpc#ClientConn.NewStream.`, servName)) | ||
|
|
||
| // Client interface. | ||
| if deprecated { |
There was a problem hiding this comment.
We should put a newline before the deprecation comment:
g.P("\n"+deprecationComment)
|
Changes LGTM, but you'll need to regenerate the protos again. |
|
Shoot, good point. Done. |
Corresponding grpc PR: grpc/grpc-go#2060