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.

Sign up for GitHub

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

Jump to bottom

Add an optional implementation of streams using generics #7057

Merged
merged 16 commits into from May 3, 2024
Merged

Add an optional implementation of streams using generics #7057

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
d551dad
Add generic interfaces and implementations for streams
aarongable Mar 20, 2024
faab808
Add codegen to use the new generic implementations
aarongable Mar 20, 2024
f75c2d6
Update route_guide example to show the result
aarongable Mar 20, 2024
06c9a00
Fix go-vet complaint
aarongable Mar 20, 2024
9ddc54d
Regenerate all protos
aarongable Apr 3, 2024
8a9ade3
Undo comment change in vet.sh
aarongable Apr 6, 2024
617090d
Merge branch master into generic-streams
aarongable Apr 15, 2024
cc25c38
Move from experimental package to grpc package
aarongable Apr 15, 2024
ba0d4e3
Regenerate all protos with new version
aarongable Apr 15, 2024
586bde9
Factor out interface type construction into helper func
aarongable Apr 15, 2024
9f4fa15
Change type in generated interfaces and signatures
aarongable Apr 15, 2024
2230a4e
Merge branch master into generic-streams
aarongable Apr 22, 2024
08f3f64
Address review comments
aarongable Apr 22, 2024
f159599
Fix protoc-gen-go-grpc_test.sh to match regenerate.sh
aarongable Apr 22, 2024
8c66d69
Fix Req->Res in ServerStreaming interfaces
aarongable Apr 23, 2024
8f93711
Mark flag as experimental
aarongable May 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions balancer/grpclb/grpc_lb_v1/load_balancer_grpc.pb.go
View file
Open in desktop

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

63 changes: 54 additions & 9 deletions cmd/protoc-gen-go-grpc/grpc.go
View file
Open in desktop
Expand Up @@ -29,10 +29,11 @@ import (
)

const (
contextPackage = protogen.GoImportPath("context")
grpcPackage = protogen.GoImportPath("google.golang.org/grpc")
codesPackage = protogen.GoImportPath("google.golang.org/grpc/codes")
statusPackage = protogen.GoImportPath("google.golang.org/grpc/status")
contextPackage = protogen.GoImportPath("context")
experimentalPackage = protogen.GoImportPath("google.golang.org/grpc/experimental")
grpcPackage = protogen.GoImportPath("google.golang.org/grpc")
codesPackage = protogen.GoImportPath("google.golang.org/grpc/codes")
statusPackage = protogen.GoImportPath("google.golang.org/grpc/status")
)

type serviceGenerateHelperInterface interface {
Expand Down Expand Up @@ -318,11 +319,25 @@ func genClientMethod(gen *protogen.Plugin, file *protogen.File, g *protogen.Gene
g.P()
return
}

streamType := unexport(service.GoName) + method.GoName + "Client"
var streamInterface string
if *useGenericStreams {
typeParam := g.QualifiedGoIdent(method.Input.GoIdent) + ", " + g.QualifiedGoIdent(method.Output.GoIdent)
streamType = g.QualifiedGoIdent(experimentalPackage.Ident("StreamClientImpl")) + "[" + typeParam + "]"
if method.Desc.IsStreamingClient() && method.Desc.IsStreamingServer() {
streamInterface = g.QualifiedGoIdent(experimentalPackage.Ident("BidiStreamClient")) + "[" + typeParam + "]"
} else if method.Desc.IsStreamingClient() {
streamInterface = g.QualifiedGoIdent(experimentalPackage.Ident("ClientStreamClient")) + "[" + typeParam + "]"
} else { // i.e. if method.Desc.IsStreamingServer()
streamInterface = g.QualifiedGoIdent(experimentalPackage.Ident("ServerStreamClient")) + "[" + g.QualifiedGoIdent(method.Output.GoIdent) + "]"
}
}

serviceDescVar := service.GoName + "_ServiceDesc"
g.P("stream, err := c.cc.NewStream(ctx, &", serviceDescVar, ".Streams[", index, `], `, fmSymbol, `, opts...)`)
g.P("if err != nil { return nil, err }")
g.P("x := &", streamType, "{stream}")
g.P("x := &", streamType, "{ClientStream: stream}")
arvindbr8 marked this conversation as resolved.
Show resolved Hide resolved
if !method.Desc.IsStreamingClient() {
g.P("if err := x.ClientStream.SendMsg(in); err != nil { return nil, err }")
g.P("if err := x.ClientStream.CloseSend(); err != nil { return nil, err }")
Expand All @@ -331,11 +346,19 @@ func genClientMethod(gen *protogen.Plugin, file *protogen.File, g *protogen.Gene
g.P("}")
g.P()

// Stream auxiliary types and methods.
if *useGenericStreams {
// Use a type alias so that the type name in the generated function
// signature can remain identical even while we swap out the implementation.
g.P("type ", service.GoName, "_", method.GoName, "Client = ", streamInterface)
g.P()
return
}

genSend := method.Desc.IsStreamingClient()
genRecv := method.Desc.IsStreamingServer()
genCloseAndRecv := !method.Desc.IsStreamingServer()

// Stream auxiliary types and methods.
g.P("type ", service.GoName, "_", method.GoName, "Client interface {")
if genSend {
g.P("Send(*", method.Input.GoIdent, ") error")
Expand Down Expand Up @@ -459,23 +482,45 @@ func genServerMethod(gen *protogen.Plugin, file *protogen.File, g *protogen.Gene
g.P()
return hname
}

streamType := unexport(service.GoName) + method.GoName + "Server"
var streamInterface string
if *useGenericStreams {
typeParam := g.QualifiedGoIdent(method.Input.GoIdent) + ", " + g.QualifiedGoIdent(method.Output.GoIdent)
streamType = g.QualifiedGoIdent(experimentalPackage.Ident("StreamServerImpl")) + "[" + typeParam + "]"
if method.Desc.IsStreamingClient() && method.Desc.IsStreamingServer() {
streamInterface = g.QualifiedGoIdent(experimentalPackage.Ident("BidiStreamServer")) + "[" + typeParam + "]"
} else if method.Desc.IsStreamingClient() {
streamInterface = g.QualifiedGoIdent(experimentalPackage.Ident("ClientStreamServer")) + "[" + typeParam + "]"
} else { // i.e. if method.Desc.IsStreamingServer()
streamInterface = g.QualifiedGoIdent(experimentalPackage.Ident("ServerStreamServer")) + "[" + g.QualifiedGoIdent(method.Output.GoIdent) + "]"
}
}

g.P("func ", hnameFuncNameFormatter(hname), "(srv interface{}, stream ", grpcPackage.Ident("ServerStream"), ") error {")
if !method.Desc.IsStreamingClient() {
g.P("m := new(", method.Input.GoIdent, ")")
g.P("if err := stream.RecvMsg(m); err != nil { return err }")
g.P("return srv.(", service.GoName, "Server).", method.GoName, "(m, &", streamType, "{stream})")
g.P("return srv.(", service.GoName, "Server).", method.GoName, "(m, &", streamType, "{ServerStream: stream})")
} else {
g.P("return srv.(", service.GoName, "Server).", method.GoName, "(&", streamType, "{stream})")
g.P("return srv.(", service.GoName, "Server).", method.GoName, "(&", streamType, "{ServerStream: stream})")
}
g.P("}")
g.P()

// Stream auxiliary types and methods.
if *useGenericStreams {
// Use a type alias so that the type name in the generated function
// signature can remain identical even while we swap out the implementation.
g.P("type ", service.GoName, "_", method.GoName, "Server = ", streamInterface)
g.P()
return hname
}

genSend := method.Desc.IsStreamingServer()
genSendAndClose := !method.Desc.IsStreamingServer()
genRecv := method.Desc.IsStreamingClient()

// Stream auxiliary types and methods.
g.P("type ", service.GoName, "_", method.GoName, "Server interface {")
if genSend {
g.P("Send(*", method.Output.GoIdent, ") error")
Expand Down
2 changes: 2 additions & 0 deletions cmd/protoc-gen-go-grpc/main.go
View file
Open in desktop
Expand Up @@ -44,6 +44,7 @@ import (
const version = "1.3.0"

var requireUnimplemented *bool
var useGenericStreams *bool

func main() {
showVersion := flag.Bool("version", false, "print the version and exit")
Expand All @@ -55,6 +56,7 @@ func main() {

var flags flag.FlagSet
requireUnimplemented = flags.Bool("require_unimplemented_servers", true, "set to false to match legacy behavior")
useGenericStreams = flags.Bool("use_generic_streams", false, "set to true to use generic types for streaming client and server objects")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the new flag be marked experimental in the next release of protoc-gen-go-grpc? By experimental, I mean flag string be use_generic_streams_experimental and the usage says something like This flag is EXPERIMENTAL and may be changed or removed in a later release.

Later when we stabilize the API we can switch the flag's default to true and remove the experimental tags.

@dfawley, wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense - the idea would be to delete the flag entirely one release after it is on by default. So:

  1. Add flag, disabled, release.
  2. Switch default of flag, release.
  3. Remove flag, release.

(All separated by some reasonable amount of time to find bugs/etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me! I've updated the name and description of the flag, updated the two places it is referenced in scripts in this repo, and updated the release notes in the PR description.


protogen.Options{
ParamFunc: flags.Set,
Expand Down
4 changes: 2 additions & 2 deletions credentials/alts/internal/proto/grpc_gcp/handshaker_grpc.pb.go
View file
Open in desktop

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

12 changes: 6 additions & 6 deletions examples/features/proto/echo/echo_grpc.pb.go
View file
Open in desktop

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