protoc-gen-go-grpc: final service registration API decision for v1.0

[dfawley]

We have two current proposals for supporting service registration. I'm opening this issue to solicit feedback on which would be preferred and why, or how to improve upon them. I plan to make a decision on this within a week, as this discussion has been ongoing for over three months, and nothing new has been proposed in some time.

1. The code at HEAD (example). This is: a FooService struct with fields for the methods. Service implementers assign the fields of FooService for their method handlers. Legacy FooServer interfaces generated via an option to maintain compatibility for legacy code.

2. The code at ad51f57 (example). This is: FooServer interface with a requirement to embed UnimplementedFooServer struct in implementations. Service implementers implement method handlers as methods on a struct and embed the Unimplemented server struct. Generated code can be made to not require this via an option to maintain compatibility for legacy code.

At this point, I believe (2) would be preferable as it requires less boilerplate for users, and the odds of forgetting to add an implemented method to the struct in (1) are similar to the odds of misspelling a method in (2). It is also more similar to the legacy codegen tool, and in fact will work transparently for users already embedding the Unimplemented struct as recommended by that codegen.

---

Please note: it is a requirement that adding methods to a service cannot break existing implementations of the service. This requirement is not up for debate. To avoid adding noise to this issue, please do not discuss it here, and please make sure any counter-proposals honor it. We recognize this is a change from the legacy codegen, which regretfully violated this requirement. #3669 contains more details.

[ernestoalejo]

Between those two options I think the first one has an advantage. In both a new method might go unnoticed but in the first one you have to forgot to add it or update a library without knowing there are new methods, whereas in the second option any minor rename while developing the service can cause trouble debugging the typo.

In the example while developing I may need to rename the message EchoRequest to EchoOtherRequest. The old method func(context.Context, *EchoRequest) (*EchoResponse, error) would now be wrong. The first method would return a compilation error because you can't assign that type of function and the second would simply go along changing the behaviour without any alert. This also applies to any method rename (only while developing as it is a backwards incompatible change) or any deprecated method deletion, at least it would alert of the unknown field assigned to the struct and the developer can debug the problem easily.

Finally a note: with the first option grpc could generate an interface (e.g. EchoServiceFullyImplemented) that would allow us to do this in our code to opt-in to the old compile-time guarantees:

var _ EchoServiceFullyImplemented = new(MyServer)

For services implemented once internally inside a company this might be a good-enough solution. This wouldn't change anything about the proposed design, it's only added for this check and does not need to be used anywhere in the generated code.

[daved]

I think this be done conditionally and with reflection (should be low cost - i.e. at registration only). https://play.golang.org/p/WWf8qneiMl8

[veith]

Personally I prefer variant 2

The second variant gives us more flexibility and possibilities like

• separating generated code from our own.
• Implementing interfaces which are used by interceptors in your package

Example: If I would like to use the auth middleware from grpc-ecosystem/go-grpc-middleware i have to put the override function to implement the interface (which is checked in line 39) to the generated code package. Otherwise i can add the override in my own package.

[belak]

Variant 1

... once we remove func NewFooService(interface{}) and add func UnstableRegisterFooService(UnstableFooService).

If you do decide to go with variant 1, is this still planned? (ref #3669 (comment))

Variant 2

Alternatively, if you do go with variant 2, instead of requiring a compile-time flag to be set, could you allow for embedding an UnstableFooService (similar to UnimplementedFooService but only providing the private method) to support legacy code, or is that off the table?

This would make it clear that it's not the recommended way of doing things (and satisfy the requirement that those server implementations be considered "unstable"), it would keep the interface similar between "stable" and "unstable" implementations.

I believe the main reason for looking at other options was because there's no way to ensure methods are implemented correctly at compile time if you have to embed the "unimplemented" version (as mentioned here #3669 (comment)). However, if variant 2 is being considered, is that requirement being dropped?

Preference

I personally prefer variant 2 with an additional embeddable UnstableFooService as described above. It would be the least amount of change (making it easy for people to migrate and not making many tutorials drastically out of date), it would meet the requirements of forward compatibility for service definitions (as the service implementer would have to choose which variant they want with a strong emphasis towards the forward-compatible version), it allows users to choose which one they want to use, it still uses interfaces (which are arguably more go-like than the other options).

[dfawley]

@daved - The reflection code looks essentially the same as when we had NewFooService (reflection-less), but combines RegisterFooService with NewFooService into one function call.

@belak - Yes, I think an UnstableServer interface sounds reasonable if we go with option 2. Thanks for the suggestion. I do think we still want to keep the option for generated code so embedding is not required, to avoid the need to make any code changes when migrating.

[daved]

@dfawley IIUC, either having two registration functions or a registration param to achieve "unimplemented fill-ins" sounds far better than requiring forwarding functions manually or embedding a special type. Following that line of thinking, still granting outside access to the unimplemented type would allow for manual setup/forwarding.

[dfawley]

@daved, the issue with this pattern is it throws away all type safety whatsoever. This is worse than embedding Unimplemented, which at least catches a bad function signature, and makes sure you pass the intended parameter. Imagine:

	RegisterFooService(s, &myBarService{})

Now all methods are unimplemented.

[daved]

Yes, I'm voicing explicitly that it seems better to provide a mechanism that is less awkward/cumbersome while permitting increased control over the behavior of registration with the small chance that someone accidentally provides an entirely incorrect implementation to the registration function. Which does seem like a feature in the case that I wanted to slam together an unimplemented "make it just turn on" compilable example.

Registration in this manner would allow users to determine their own level of comfort. For example, an error return that reports implemented vs total methods permits logging, alerting, or halting at the users' discretion.

[mpx]

I just noticed the change in server registration at HEAD with the FooService struct. I'd much prefer using an embedded type (option 2) to maintain the interface. It would be less awkward to use (compared to lots of boilerplate for option 1). It would also simplify migration to the new code generator.

Expanding on @belak's UnstableFooService idea, private methods could be used to require one of the interface options to be embedded:

• UnstableFooService: Author accepts that adding methods to the server will break existing code - they want to be warned.
• StableFooService: Author prefers for unimplemented methods to return an error.

(Perhaps this is already the intent, but I don't see the private marker method in the option 2 example.)

Eg:

type FooServer interface {
   F1(context.Context, *F1Request) (*F1Reply, error)
   F2(context.Context, *F2Request) (*F2Reply, error)
   _FooServerImpl()
}

type UnstableFooServer struct{}
func (s UnstableFooServer) _FooServerImpl()

type StableFooServer struct{}
func F1(context.Context, *F1Request) (*F1Reply, error) { return status.Errorf(codes.Unimplemented, "method F1 not  implemented") }
func F2(context.Context, *F2Request) (*F2Reply, error) { return status.Errorf(codes.Unimplemented, "method F2 not  implemented") }
func (s FallbackFooServer) _FooServerImpl()

This would be a simpler one-time migration to explicitly opt into forward compatibility (or not).

Alternatively, embeding the fallback type could be optional, but I suspect most server implementations won't make any changes, hence forward compatibility will not be handled across the ecosystem.

I'd prefer to use something other than "Unimplemented" that better reflects the intent. Every server type will have one of these marker structs, using "Unimplemented" is awkward in documentation since almost everything is typically implementated and it doesn't represent the authors intent. Perhaps "Stable" and "Unstable" better represent the intent from the author? Or maybe some other pair?

[daved]

Here is another example which, in my opinion/experience, would be far more expected than the class-based "accents" currently offered: https://play.golang.org/p/S-ODT7NblXn

edit to add - Alternately, all of the forwarding can be moved to the construction function: https://play.golang.org/p/E687BM_fkdy
Though, I prefer the first example and consider it more flexible (i.e. users can provide their own UnimplementedForwarder type) despite the convenience of this second one focusing all unimplemented behavior changes into one area. Again, I'm just trying to emphasize that options 1 and 2 stick out as not Go-like.

[dfawley]

I've sent #3911 to revert the repo back to the interface-based service registration method. This includes the interface suggested by @belak to allow users to opt out of forward compatibility. If there are no problems discovered with it, the intent is to release this as v1.0 by Sept 30.

While it seems all the options have their pros and cons, the decision was made to revert to this because: 1. it works with existing code (as long as it embedded Unimplemented already, which is recommended), 2. its cons are not meaningfully worse than the cons of the struct-based alternative (error by typo vs. error by omission), and 3. it has the least boilerplate requirements.

I'd prefer to use something other than "Unimplemented" that better reflects the intent.

I don't disagree, but this is the historical name used, so I would prefer to keep it to allow all existing code to be compatible with it. I thought about having type aliases, but that would just add confusion and API clutter.

Here is another example which, in my opinion/experience, would be far more expected than the class-based "accents" currently offered

Both these snippets include an interface that is unstable (i.e. would grow as methods are added to the service) without any naming that hints at it. These are really UnstableFooServers. Propagating that to Reg, the primary service registration API now deals with unstable interfaces, which is not allowed by the requirements of this design.

[belak]

Thanks for listening to community feedback and trying to come up with the best possible solution!

If the goal is to move away from UnimplementedFooServer as a name, I think you could provide that as a type alias, though I can't test this at the moment.

type StableFooServer struct {}

func (s StableFooServer) mustEmbedFooServerStability() {}

type UnimplementedFooServer = StableFooServer