protoc-gen-go-grpc: API for service registration

[belak]

There were some changes in #3657 that make it harder to develop gRPC services and harder to find new unimplemented methods - I wanted to start a discussion around the new default and figure out why the change was made. I do understand this is in an unreleased version, so I figured a discussion would be better than a bug report or feature request.

From my perspective, this is a number of steps backwards for reasons I will outline below.

When implementing a gRPC service in Go, I often start with a blank slate - the service has been defined in proto files, the go and gRPC protobuf definitions have been generated, all that's left to do is write the code. I often use something like the following so the compiler will help me along, telling me about missing methods, incorrect signatures, things like that.

package chat
func init() {
	// Ensure that Server implements the ChatIngestServer interface
	var server *Server = nil
	var _ pb.ChatIngestServer = server
}

This can alternatively be done with var _ pb.ChatIngestServer = &Server{} but that theoretically leaves a little bit more memory around at runtime.

After this, I add all the missing methods myself (returning the unimplemented status) and start adding implementations to them until I have a concrete implementation for all the methods.

Problems with the new approach

• As soon as you embed the Unimplemented implementation, the Go compiler gets a lot less useful - it will no longer tell you about missing methods and because Go interfaces are implicit, if you make a mistake in implementing a method (like misspelling a method name), you will not find out until runtime. Additionally, because these are public methods, if they're attached to a public struct (like the commonly used Server), they may not be detected as an unused method.
• If protos and generated files are updated, you will not know about any missing methods until runtime. Personally, I would prefer to know if I have not fully implemented a service at compile time rather than waiting until clients are running against it.
• IDE hinting is worse with the new changes - IDEs will generally not recommend a method stub for an unimplemented method if it's provided by an embedded struct because even though it's technically implemented, it does not have a working implementation.

I generally prefer compile time guarantees that all methods are implemented over runtime checks.

Benefits of the new approach

• Protos and generated files can be updated without requiring updates to server implementations.

Proposal

By default, the option requireUnimplementedServers should default to false. This option is more valuable when dealing with external protobufs which are not versioned (maybe there should be a recommendation to embed the unimplemented struct in this instance) and makes it harder to catch mistakes if you are developing a canonical implementation of a service which should implement all the available methods.

At least for me, the problems with the new approach vastly outweigh the benefits I've seen so far.

[dfawley]

By default, the option requireUnimplementedServers should default to false.

If this defaults to false, nobody will use it. Note that we wouldn't even have created a flag if we were starting from scratch. The only reason for the flag is to enable backward compatibility without requiring code changes.

It was always supposed to be a requirement (undocumented, most likely, so I have no pointers for you) that the addition of new methods to a proto would be a backward-compatible change. The initial implementation of the grpc-go proto codegen that didn't allow for this broke that requirement.

This issue is covered in #2318 and golang/protobuf#784.

If you have suggestions for other ways to implement this requirement, then we're willing to discuss -- we still haven't released 1.0 yet, so things can be changed. This is the best approach we found.

Note that C++ and Java both have similar implementations. In C++, there is a base class that you use and then override methods for the handlers, and in Java, the service is generated as an abstract class that your implementation must extend.

[belak]

By default, the option requireUnimplementedServers should default to false.

If this defaults to false, nobody will use it. Note that we wouldn't even have created a flag if we were starting from scratch. The only reason for the flag is to enable backward compatibility without requiring code changes.

It seems useful, especially in a language where you don't have to explicitly implement an interface or override methods, to make this optional because, as mentioned before this removes some of the compile-time checks that you're doing things correctly. Even before this point, I'm fairly sure you could still embed the unimplemented version if you wanted to.

It was always supposed to be a requirement (undocumented, most likely, so I have no pointers for you) that the addition of new methods to a proto would be a backward-compatible change. The initial implementation of the grpc-go proto codegen that didn't allow for this broke that requirement.

It makes perfect sense for clients to not break when things are added, but if you're dealing with the canonical implementation of a service, isn't it more logical to realize you're not implementing an interface at compile time rather than runtime? You're not meeting the contract specified by the service you're implementing. I do understand that for services which are implemented in multiple places this doesn't make as much sense and the changes are an improvement.

If you have suggestions for other ways to implement this requirement, then we're willing to discuss -- we still haven't released 1.0 yet, so things can be changed. This is the best approach we found.

Would you consider making the generated method provided by the UnimplementedSomethingServer public? Something like MustProvideForwardCompatibility This way you could still opt out if you absolutely wanted to and the generated code wouldn't need to be different between versions.

Alternatively, a second generated struct could be provided allowing you to opt out of the forward compatibility in services when it's embedded - both UnimplementedSomethingServer and ManualSomethingServer could be provided as embed options, but strongly recommending using unimplemented over the manual implementation unless it is the implementation of this RPC service.

Note that C++ and Java both have similar implementations. In C++, there is a base class that you use and then override methods for the handlers, and in Java, the service is generated as an abstract class that your implementation must extend.

The nice thing about Java though is that you at least get a compile time guarantee that you didn't misspell a method with @Overrides. It's been a while since I've written C++, but I assume there would be compile time checks against the header file. With Go and implicit interfaces it's not as safe to do that.

Thanks for explaining your reasoning - I wasn't aware other languages already did this.

[dfawley]

The nice thing about Java though is that you at least get a compile time guarantee that you didn't misspell a method with @OVERRIDES. It's been a while since I've written C++, but I assume there would be compile time checks against the header file. With Go and implicit interfaces it's not as safe to do that.

That's a good point. OOP languages have a way of distinguishing adding methods vs. overriding methods, which prevents typos like this. Go isn't OO (it lacks inheritance), and so the type system isn't capable of this kind of feat.

Brainstorming some other options:

1. Make RegisterFooServer accept interface{}. Applications could then use var _ FooServer = (*myFooServer)(nil) for the check you wanted, above.

   • Con: no type checking at all on the input, but, with an embedded UnimplementedFooServer, half of the value of the type check is already lost so maybe this is not that bad.

2. Alter UnimplementedFooServer so it is essentially a dispatching implementation instead:

   type UnimplementedFooServer struct { ... }
   func (s *UnimplementedFooServer) MyMethod(...) ... { return s.myMethod(...) }
   func (s *UnimplementedFooServer) SetMyMethod(m func(...)...) { s.myMethod = m }
   func UnsafeWrapFooServer(s UnsafeFooServer) FooServer {
   	// Return simple wrapper that adds a magic method to s.  Breaks if a new method is added (due to UnsafeFooServer parameter type).
   }
   
   // Usage:
   func main() {
   	s := &myFooServer{}
   	u := &pb.UnimplementedFooServer{}
   	u.SetMyMethod(s.MyMethod) // etc...
   	pb.RegisterFooServer(grpcServer, u)
   	// -- or --
   	pb.RegisterFooServer(grpcServer, pb.UnsafeWrapFooServer(&myFooServer{}))
   }

[bufdev]

I always appreciate the work of the grpc-go maintainers, but I do think this issue deserves some speaking up on the issue, so my two cents:

It was always supposed to be a requirement (undocumented, most likely, so I have no pointers for you) that the addition of new methods to a proto would be a backward-compatible change. The initial implementation of the grpc-go proto codegen that didn't allow for this broke that requirement.

It wasn't a requirement though - there's a lot of things I do in my code that I wish I hadn't done, but unless I'm willing to v2 the code, that's just how it is. Changing that on a minor will cause more harm than the problem it solves.

If you have suggestions for other ways to implement this requirement, then we're willing to discuss -- we still haven't released 1.0 yet, so things can be changed. This is the best approach we found.

The issue is that grpc-go as a whole IS 1.0, and has been for a while. More to the point, in practice, this generator is a port from another 1.0'ed repository, where people are being asked to move to this.

With setting the generator as unreleased and adding this requirement based on granular and/or documented compatibility guarantees, perhaps this doesn't violate the letter of SemVer, but it certainly violates the spirit. It also doesn't follow principle of least surprise - I happen to follow these issues closely, but the vast majority of gRPC users do not, and adding a (especially runtime) requirement to embed this could quite literally break the internet. I sympathize with the intent here, but in practice, this generator should be considered 1.0, and not have breaking changes. Opt-in to this feature, while not optimal, is better than lots of users being being broken, which will result in users leaving the Protobuf/gRPC ecosystem out of a lack of trust.

[dfawley]

It wasn't a requirement though - there's a lot of things I do in my code that I wish I hadn't done, but unless I'm willing to v2 the code, that's just how it is. Changing that on a minor will cause more harm than the problem it solves.

Adding methods to services was always intended to be backward compatible. It is a backward compatible change in every other language as far as I'm aware. It is also backward compatible at a wire protocol level. Asking users to do a major version bump of their protocol for every new method added, just because of Go, is unacceptable. In a monorepo, this problem can be mitigated by updating every service implementation atomically with the addition of the method, but that doesn't work in OSS.

The issue is that grpc-go as a whole IS 1.0, and has been for a while. More to the point, in practice, this generator is a port from another 1.0'ed repository, where people are being asked to move to this.

Flags will be provided in protoc-gen-go-grpc to enable backward-compatible behavior. If you can't tolerate the new version of the codegen, set the flag and you get the old codgen's code. By default we want to encourage the new behavior, which will allow the addition of methods to be backward-compatible. If it helps avoid confusion, we could name the first release of protoc-gen-go-grpc v2.0 to follow the "spirit" of semver more closely, though I fail to see how this would help, practically speaking.

adding a (especially runtime) requirement to embed this could quite literally break the internet.

The current implementation of this functionality makes it a compile-time requirement, not anything checked at runtime. We are aware of pros and cons of various alternatives and will do everything we can to minimize any disruption.

[bufdev]

It is also backward compatible at a wire protocol level. Asking users to do a major version bump of their protocol for every new method added, just because of Go, is unacceptable.

This isn't what's happening though - users' Go code is being broken with this change. It's up to users to decide what compatibility requirements they want for their generated code - for example, we wouldn't expose generated gRPC code as part of SemVer.

In a monorepo, this problem can be mitigated by updating every service implementation atomically with the addition of the method, but that doesn't work in OSS.

This is the key thing though: 99% of gRPC users are not in a monorepo, and SemVer matters as a part of the Golang toolchain at this point.

Flags will be provided in protoc-gen-go-grpc to enable backward-compatible behavior. If you can't tolerate the new version of the codegen, set the flag and you get the old codgen's code.

This in itself is not backwards-compatible. Making breaking changes that are opt-out is not a backwards-compatible change, it has to be opt-in.

This change will further erode trust in the Protobuf/gRPC community and have the opposite effect of it's intent. I strongly recommend reconsidering this, and perhaps just making this flag opt-out for Google internal use? More to the point, set this flag to opt-in for everyone else, and then opt into it Google-wide?

[neild]

Making breaking changes that are opt-out is not a backwards-compatible change, it has to be opt-in.

Note that there is no change to existing tools. The gRPC service generator in github.com/golang/protobuf/protoc-gen-go behaves as it has in the past.

Someone changing from github.com/golang/protobuf/protoc-gen-go to google.golang.org/grpc/cmd/protoc-gen-go-grpc will need to choose between updating their code or setting --go-grpc_opt=requireUnimplementedServers=false, but they're already making an explicit choice to change generators.

[belak]

What was the problem with the original approach? Before you could choose to embed the UnimplementedXXXServer and you would get forward compatibility, but it wasn't required. Requiring that it be embedded makes it so when projects update protobufs, they may miss implementing new methods, which seems worse.

What if there were two structs provided that could be embedded, one which provides forward compatibility and one which doesn't? It wouldn't require a wrapper, the default method name could point to the Unimplemented version rather than the one which doesn't provide forward compatibility, and you would have to explicitly choose to not have forward compatibility for service definitions.