Skip to content

Commit

Permalink
cmd/protoc-gen-go-grpc: test the embedded struct at registration time…
Browse files Browse the repository at this point in the history
… for proper usage (grpc#7438)
  • Loading branch information
dfawley authored and printchard committed Jul 30, 2024
1 parent e6acb33 commit 037f47d
Show file tree
Hide file tree
Showing 22 changed files with 412 additions and 77 deletions.
17 changes: 14 additions & 3 deletions balancer/grpclb/grpc_lb_v1/load_balancer_grpc.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 14 additions & 3 deletions channelz/grpc_channelz_v1/channelz_grpc.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions cmd/protoc-gen-go-grpc/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,10 @@ E.g.:

Note that this is not recommended, and the option is only provided to restore
backward compatibility with previously-generated code.

When embedding the `Unimplemented<ServiceName>Server` in a struct that
implements the service, it should be embedded by _value_ instead of as a
_pointer_. If it is embedded as a pointer, it must be assigned to a valid,
non-nil pointer or else unimplemented methods would panic when called. This is
tested at service registration time, and will lead to a panic in
`Register<ServiceName>Server` if it is not embedded properly.
14 changes: 13 additions & 1 deletion cmd/protoc-gen-go-grpc/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,16 @@ module google.golang.org/grpc/cmd/protoc-gen-go-grpc

go 1.21

require google.golang.org/protobuf v1.34.1
require (
google.golang.org/grpc v1.65.0
google.golang.org/protobuf v1.34.1
)

require (
golang.org/x/net v0.26.0 // indirect
golang.org/x/sys v0.21.0 // indirect
golang.org/x/text v0.16.0 // indirect
google.golang.org/genproto/googleapis/rpc v0.0.0-20240604185151-ef581f913117 // indirect
)

replace google.golang.org/grpc => ../..
14 changes: 10 additions & 4 deletions cmd/protoc-gen-go-grpc/go.sum
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
github.com/google/go-cmp v0.5.5 h1:Khx7svrCpmxxtHBq5j2mp/xVjsi8hQMfNLvJFAlrGgU=
github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4=
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
golang.org/x/net v0.26.0 h1:soB7SVo0PWrY4vPW/+ay0jKDNScG2X9wFeYlXIvJsOQ=
golang.org/x/net v0.26.0/go.mod h1:5YKkiSynbBIh3p6iOc/vibscux0x38BZDkn8sCUPxHE=
golang.org/x/sys v0.21.0 h1:rF+pYz3DAGSQAxAu1CbC7catZg4ebC4UIeIhKxBZvws=
golang.org/x/sys v0.21.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
golang.org/x/text v0.16.0 h1:a94ExnEXNtEwYLGJSIUxnWoxoRz/ZcCsV63ROupILh4=
golang.org/x/text v0.16.0/go.mod h1:GhwF1Be+LQoKShO3cGOHzqOgRrGaYc9AvblQOmPVHnI=
google.golang.org/genproto/googleapis/rpc v0.0.0-20240604185151-ef581f913117 h1:1GBuWVLM/KMVUv1t1En5Gs+gFZCNd360GGb4sSxtrhU=
google.golang.org/genproto/googleapis/rpc v0.0.0-20240604185151-ef581f913117/go.mod h1:EfXuqaE1J41VCDicxHzUDm+8rk+7ZdXzHV0IhO/I6s0=
google.golang.org/protobuf v1.34.1 h1:9ddQBjfCyZPOHPUiPxpYESBLc+T8P3E+Vo4IbKZgFWg=
google.golang.org/protobuf v1.34.1/go.mod h1:c6P6GXX6sHbq/GpV6MGZEdwhWPcYBgnhAHhKbcUYpos=
17 changes: 14 additions & 3 deletions cmd/protoc-gen-go-grpc/grpc.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,12 @@ func (serviceGenerateHelper) generateUnimplementedServerType(gen *protogen.Plugi
mustOrShould = "should"
}
// Server Unimplemented struct for forward compatibility.
g.P("// Unimplemented", serverType, " ", mustOrShould, " be embedded to have forward compatible implementations.")
g.P("type Unimplemented", serverType, " struct {")
g.P("}")
g.P("// Unimplemented", serverType, " ", mustOrShould, " be embedded to have")
g.P("// forward compatible implementations.")
g.P("//")
g.P("// NOTE: this should be embedded by value instead of pointer to avoid a nil")
g.P("// pointer dereference when methods are called.")
g.P("type Unimplemented", serverType, " struct {}")
g.P()
for _, method := range service.Methods {
nilArg := ""
Expand All @@ -100,6 +103,7 @@ func (serviceGenerateHelper) generateUnimplementedServerType(gen *protogen.Plugi
if *requireUnimplemented {
g.P("func (Unimplemented", serverType, ") mustEmbedUnimplemented", serverType, "() {}")
}
g.P("func (Unimplemented", serverType, ") testEmbeddedByValue() {}")
g.P()
}

Expand Down Expand Up @@ -306,6 +310,13 @@ func genService(gen *protogen.Plugin, file *protogen.File, g *protogen.Generated
}
serviceDescVar := service.GoName + "_ServiceDesc"
g.P("func Register", service.GoName, "Server(s ", grpcPackage.Ident("ServiceRegistrar"), ", srv ", serverType, ") {")
g.P("// If the following call pancis, it indicates Unimplemented", serverType, " was")
g.P("// embedded by pointer and is nil. This will cause panics if an")
g.P("// unimplemented method is ever invoked, so we test this at initialization")
g.P("// time to prevent it from happening at runtime later due to I/O.")
g.P("if t, ok := srv.(interface { testEmbeddedByValue() }); ok {")
g.P("t.testEmbeddedByValue()")
g.P("}")
g.P("s.RegisterService(&", serviceDescVar, `, srv)`)
g.P("}")
g.P()
Expand Down
46 changes: 46 additions & 0 deletions cmd/protoc-gen-go-grpc/unimpl_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
*
* Copyright 2024 gRPC authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

package main_test

import (
"testing"

"google.golang.org/grpc"
testgrpc "google.golang.org/grpc/interop/grpc_testing"
)

type unimplEmbeddedByPointer struct {
*testgrpc.UnimplementedTestServiceServer
}

type unimplEmbeddedByValue struct {
testgrpc.UnimplementedTestServiceServer
}

func TestUnimplementedEmbedding(t *testing.T) {
// Embedded by value, this should succeed.
testgrpc.RegisterTestServiceServer(grpc.NewServer(), &unimplEmbeddedByValue{})
defer func() {
if recover() == nil {
t.Fatalf("Expected panic; received none")
}
}()
// Embedded by pointer, this should panic.
testgrpc.RegisterTestServiceServer(grpc.NewServer(), &unimplEmbeddedByPointer{})
}
17 changes: 14 additions & 3 deletions credentials/alts/internal/proto/grpc_gcp/handshaker_grpc.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 14 additions & 3 deletions examples/features/proto/echo/echo_grpc.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 14 additions & 3 deletions examples/helloworld/helloworld/helloworld_grpc.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 14 additions & 3 deletions examples/route_guide/routeguide/route_guide_grpc.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 14 additions & 3 deletions health/grpc_health_v1/health_grpc.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 037f47d

Please sign in to comment.