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
codegen: Use Go generics for stream types #7030
Comments
Something like this could be really interesting. Unfortunately this is a very niche kind of thing and we likely won't have the bandwidth to work on it in the foreseeable future. If you are interested in fleshing this out and sending PRs, we can review them. As for an implementation plan, I'd propose something like:
Not being backward compatible would be the biggest problem with changing how the codegen works. We'd either need to generate both together with different names (the non-generic implementation could even call the generic one), or we'd need a flag to decide which one to generate. If we generated both, we'd need to choose good names to be ergonomic and to avoid conflicts. I'm not concerned about supporting versions of Go older than 1.18, as this kind of thing is necessary from time to time anyway (e.g. we will soon be making changes to the codegen that will require the most recent release of grpc-go, which doesn't support 1.18 already). |
Thanks for taking the time to read! I'd be happy to contribute implementation, but of course also have a lot of other things on my plate so no guarantees. Would you also be willing to accept an implementation plan like this:
To my mind, the big change here is in the publicly-exposed interface, and that's the thing worth protecting behind a flag. But I definitely don't have insight into all of the things the gRPC team might be concerned about (do Go generics have meaningfully different runtime performance? I don't think so, but maybe) so I'm not sure if swapping out the underlying implementation is too risky to not put in Thanks for the note about go 1.18; I'll keep an eye on other compatibility stuff and just make sure any reliance on generics lands after the other changes you mentioned. |
I definitely would like to put the interfaces in Does the type aliases prevent any backward compatibility breakage problems? If so, that might be the ideal final state? I really don't want to be in a situation where users have to choose between two different forms of codegen that are incompatible. Otherwise no old generated code library can ever migrate to the new style (without a v2 since it's a breaking change), and users of that generated code would be stuck implementing it that way forever. |
I've added the generic implementations to the experimental package, and added the new codegen behind a flag, in this PR: #7057 Let me know what you think! |
Let's leave this issue open to track:
|
Do we have a rough timeline for when a new version of protoc-gen-go-grpc (presumably v1.4.0?) will be released? |
Update the version of protoc-gen-go-grpc that we use to generate Go gRPC code from our proto files, and update the versions of other gRPC tools and libraries that we use to match. Turn on the new `use_generic_streams` code generation flag to change how protoc-gen-go-grpc generates implementations of our streaming methods, from creating a wholly independent implementation for every stream to using shared generic implementations. Take advantage of this code-sharing to remove our SA "wrapper" methods, now that they have truly the same signature as the SARO methods which they wrap. Also remove all references to the old-style stream names (e.g. foopb.FooService_BarMethodClient) and replace them with the new underlying generic names, for the sake of consistency. Finally, also remove a few custom stream test mocks, replacing them with the generic mocks.ServerStreamClient. Note that this PR does not change the names in //mocks/sa.go, to avoid conflicts with work happening in the pursuit of #7476. Note also that this PR updates the version of protoc-gen-go-grpc that we use to a specific commit. This is because, although a new release of grpc-go itself has been cut, the codegen binary is a separate Go module with its own releases, and it hasn't had a new release cut yet. Tracking for that is in grpc/grpc-go#7030.
Also, I'd just like to thank the gRPC maintainers (particularly @dfawley and @arvindbr8) for being willing to review and accept this contribution. Let's Encrypt is already using it, and the cleanup it enabled -- now that one implementation can satisfy two service interfaces that share some of the same methods -- is truly delightful. Thank you! |
I was hoping we could get it done this week, but it looks like that isn't going to happen. So, hopefully early next week. There is nothing blocking us at this point. And thank you for the suggestion, contribution, and for sticking with it through all the changes during review! |
Use case(s)
Suppose you have the following service definitions:
The idea here being that someone with access to a Greeter client could call StoreHello multiple times to store many names that should be greeted, and then either a Greeter client or a GreeterReadOnly client can call SayHello to get a stream of all the stored replies. Obviously this is a toy example, but you can see a real example of this same pattern here. The salient feature here is this: we have two different services which define the exact same streaming RPC.
Now because both Greeter and GreeterReadOnly share functionality, we want to use a single implementation type to implement both of them. So we write something like this:
But of course when we go to Register this implementation, it doesn't actually work. It satisfies the
Greeter
interface, but not theGreeterReadOnly
interface.This is because grpc-go has generated the following interfaces for us:
The
SayHello
methods in these two interfaces do not have the same signature, despite having the exact same definition in the original .proto file. This means that they can never both be implemented by the same Go struct.Note: this is different from the case for Unary RPCs: if two different gRPC services define the same unary rpc, then the generated Go methods have the exact same signature and can both be implemented by the same type.
Proposed Solution
The obvious solution here is that the stream object could be named by the message type(s) it is capable of streaming, rather than by the service and rpc which define it. For example, instead of
Greeter_SayHelloServer
, the type could be namedHelloReplyServerStreamServer
, indicating that it a) is the Server half of the stream object, b) that it is for a Server-Streaming (i.e. one request, many replies) method, and c) that it streamsHelloReply
messages. Then this same exact type could be used in both versions of theSayHello
method, giving them the same signature, and allowing them to be implemented by a single type.But I propose going even further: rather than generating a unique type for every stream, I think that protoc-gen-go-grpc should instead take advantage of Go's generics support and define only six streaming types, parameterized by the message type they stream:
Then the generated code for the service interfaces and their
SayHello
method would simply be reduced to:Note first that this eliminates the need for there ever to be service- or method-specific types in the protoc-gen-go-grpc generated code. And in fact these
[Server|Client|Bidi]Stream[Server|Client]
generic types could be defined once in the gRPC package itself and then simply referenced in the generated code.But note more importantly that this means that both
GreeterServer.SayHello
andGreeterReadOnlyServer.SayHello
have the exact same function signature, meaning that they can both be implemented by a single type:Additional Context
This proposal is obviously not backwards-compatible, and if it were adopted it would mean that the new version of protoc-gen-go-grpc cannot be used by versions of Go prior to when it got generics (1.18). But I think it is sufficiently elegant that it is worth considering anyway.
The text was updated successfully, but these errors were encountered: