Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

protoc-gen-go-grpc: Use grpc.ServiceRegistrar instead of *grpc.Server in _grpc.pb.go #3966

Closed
vadimsht opened this issue Oct 20, 2020 · 1 comment · Fixed by #3968
Closed
Assignees
Labels
Type: Feature New features or improvements in behavior

Comments

@vadimsht
Copy link

Use case(s) - what problem will this feature solve?

I'm from a team behind Chromium CI system aka LUCI. We use gRPC domain model (services defined in *.proto, generated stubs etc) to publish our internal APIs on Appengine Standard Environment. We don't use gRPC protocol proper, since Appengine doesn't really support inbound HTTP2. This thing we built is called prpc and it is kind of a hack. The idea is to eventually switch to real gRPC, when we can, by swapping prpc server and client instantiations with gRPC ones.

As such we have our own Server and Client implementations that conform to gRPC semantics (sans streaming), but use simpler HTTP1 protocol.

Client implements grpc.ClientConnInterface and stubs generated by protoc-gen-go-grpc work splendidly with it!

Server implements grpc.ServiceRegistrar, but stubs generated by protoc-gen-go-grpc are not using it and instead hardcode dependency on *grpc.Server. So we have an extra code-generatation stage to "fix" .pb.go to use an interface instead of a concrete type. We really-really want to get rid of it.

Curiously, at some point grpc stubs were using ServiceRegistrar, but it was reverted in #3885 (and now ServiceRegistrar seems to be completely unreferenced).

Proposed Solution

I propose to change protoc-gen-go-grpc to emit e.g.

func RegisterFooServer(s grpc.ServiceRegistrar, srv FooServer) {
	s.RegisterService(&_Foo_serviceDesc, srv)
}

instead of the current

func RegisterFooServer(s *grpc.Server, srv FooServer) {
	s.RegisterService(&_Foo_serviceDesc, srv)
}

It seems like a small change, but it will make a group of ~15 developers a bit happier and will justify continuing existence of grpc.ServiceRegistrar.

@vadimsht vadimsht added the Type: Feature New features or improvements in behavior label Oct 20, 2020
@dfawley
Copy link
Member

dfawley commented Oct 20, 2020

Oops. Yes, that was the whole point. It seems we missed this when switching back to the older codegen style shortly before 1.0.

@dfawley dfawley self-assigned this Oct 20, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants