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

mustEmbedUnimplemented*** method appear in grpc-server #3794

Closed
seifchen opened this issue Aug 5, 2020 · 25 comments
Closed

mustEmbedUnimplemented*** method appear in grpc-server #3794

seifchen opened this issue Aug 5, 2020 · 25 comments

Comments

@seifchen
Copy link

seifchen commented Aug 5, 2020

version

protoc:libprotoc 3.12.4
protoc-gen-go v1.25.0-devel

go version?

macos go version go1.14.4 darwin/amd64

What operating system (Linux, Windows, …) and version?

macos catalina 10.15.5

What did you do?

git clone https://github.com/grpc/grpc-go.git
cd grpc-go/examples/helloworld/helloworld
protoc -I. --go-grpc_out=. --go_out=. ./helloworld.proto

result

cat google.golang.org/grpc/examples/helloworld/helloworld/helloworld_grpc.pb.go

found this code

// GreeterServer is the server API for Greeter service.
// All implementations must embed UnimplementedGreeterServer
// for forward compatibility
type GreeterServer interface {
	// Sends a greeting
	SayHello(context.Context, *HelloRequest) (*HelloReply, error)
	mustEmbedUnimplementedGreeterServer()
}

what is the mustEmbedUnimplementedGreeterServer method?

@dfawley
Copy link
Member

dfawley commented Aug 5, 2020

As the name says, your service implementation must embed the helloworld pb.UnimplementedGreeterServer in order to be passed to the RegisterGreeterService function. This is being discussed to potentially change in #3669. Note that you are using an unreleased version of the code generator and it may change in non-backward compatible ways.

@dfawley dfawley closed this as completed Aug 5, 2020
@seifchen
Copy link
Author

seifchen commented Aug 6, 2020

@dfawley Thanks, which version could be work now?

@garydevenay
Copy link

I have run in to this too.
Having a method inside the obvious (and documented) interface to implement as an instruction seems like an awful code smell.

@dfawley
Copy link
Member

dfawley commented Oct 15, 2020

@garydevenay see the (very, very long) discussion in #3669 if you want to appreciate why we're doing this. And as documented in the README, you can have the legacy behavior with a command-line option if you really want it.

@seifchen Sorry I missed your comment earlier. Back then it was advisable to keep using the legacy tool at https://github.com/golang/protobuf, but we released v1.0 of the codegen in this repo recently, so it should be used instead: https://github.com/grpc/grpc-go/releases/tag/cmd%2Fprotoc-gen-go-grpc%2Fv1.0.0

@josegonzalez
Copy link

For those like me who have no idea what "embedding" means, can you provide a hint as to what we should do? I get this error for the test app here though that test app otherwise seems to deploy fine. Its not clear where I'm going wrong, so any hints would be greatly appreciated.

@manikanta-talanki
Copy link

@josegonzalez
I believe

// server is used to implement helloworld.GreeterServer.
type server struct{
// Embed the unimplemented server
helloworld.UnimplementedGreeterServer
}

is what you are looking for. correct me if I am wrong @dfawley

@dfawley
Copy link
Member

dfawley commented Nov 2, 2020

@manikanta-talanki - correct, thank you for assisting.

https://golang.org/doc/effective_go.html#embedding

@yujintang
Copy link

protoc --go-grpc_out=require_unimplemented_servers=false[,other options...]:. can solve this problem.

@josegonzalez
Copy link

That worked great, thank y ou @manikanta-talanki !

@dfawley
Copy link
Member

dfawley commented Nov 13, 2020

@yujintang - This works, but your binary will fail to compile if you add methods to your service(s) and regenerate/recompile. That is why we have the embedding requirement by default. We recommend against using this option.

@Avi-M
Copy link

Avi-M commented Dec 8, 2020

@manikanta-talanki thank you so much.

@teuber789
Copy link

So honestly, there are a lot of us who think that the newly added "feature" is totally useless. We want it to fail to compile after we've added methods to our services that haven't been implemented yet. We understand that client and server versions both have to be updated when you add a service method, but that's not a problem.

@arashrahimi46
Copy link

@manikanta-talanki thank you this solved my problem

@dfawley
Copy link
Member

dfawley commented Dec 11, 2020

@teuber789 if you prefer the old behavior of breakages when new methods are added, embed the UnsafeFooService interface instead.

@arrijabba
Copy link

@manikanta-talanki thanks a bunch

@BusinessOcean
Copy link

  protoc --go_out=. **--go-grpc_opt=require_unimplemented_servers=false** --go-grpc_out=. proto/*.proto 

add this it will fix the issue

@dfawley
Copy link
Member

dfawley commented Feb 16, 2021

add this it will fix the issue

This was already suggested above, and as was also mentioned above:

"This works, but your binary will fail to compile if you add methods to your service(s) and regenerate/recompile. That is why we have the embedding requirement by default. We recommend against using this option."

@lmingzhi618
Copy link

lmingzhi618 commented Feb 19, 2021

add this it will fix the issue

it works for me, thank you!
but this works too:
adding pb.UnimplementedSimpleServer to my implementation.

type SimpleService struct {
	pb.UnimplementedSimpleServer
}

@marcusljx
Copy link

marcusljx commented Feb 22, 2021

Update: This seems to be an issue with the Go linters instead of with protobuf. The program still compiles as expected. My apologies for the noise

As the name says, your service implementation must embed the helloworld pb.UnimplementedGreeterServer in order to be passed to the RegisterGreeterService function. This is being discussed to potentially change in #3669. Note that you are using an unreleased version of the code generator and it may change in non-backward compatible ways.

@dfawley even after embedding the UnimplementedGreeterServer type, I get this error when attempting to pass the implementation into RegisterGreeterServer:

Type cannot implement 'GreeterServer' as it has a non-exported method and is defined in a different package

Is there some compatibility setup I'm missing please? I'm on

go version go1.15.8 linux/amd64
libprotoc 3.14.0
google.golang.org/protobuf v1.25.0
google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.1.0

@dfawley
Copy link
Member

dfawley commented Feb 22, 2021

Update: This seems to be an issue with the Go linters instead of with protobuf. The program still compiles as expected. My apologies for the noise

Is this a custom linter you have? We run golint and go vet -all on .pb.go files and don't see any warnings like this. I understand the warning being output, but don't know where it might come from.

@marcusljx
Copy link

Update: This seems to be an issue with the Go linters instead of with protobuf. The program still compiles as expected. My apologies for the noise

Is this a custom linter you have? We run golint and go vet -all on .pb.go files and don't see any warnings like this. I understand the warning being output, but don't know where it might come from.

I'm using JetBrain's GoLand IDE (version 2020.3), so I'm not sure if the IDE uses a custom linter or not.

@shairozan
Copy link

Seconded @marcusljx Goland seems to be complaining, but I can still build / run correctly. Might be worth opening a bug with Jetbrains

@mrothroc
Copy link

mrothroc commented May 7, 2021

I am trying to use composition for my server. I have a struct that implements one of the methods. I add that to my main struct. When my main struct also includes Unimplemented... the compiler throws an error that says my methods are ambiguous. This is because the same method exists at the same level in both my code and in the Unimplemented... code.

How do I include both my struct and the other one without the collision?

@dfawley
Copy link
Member

dfawley commented May 8, 2021

Unfortunately, I think the only way around this would be to declare the method(s) on the main struct, and dispatch to the proper implementation beneath it:

type PartialFooService struct {}

func (p *PartialFooService) SomeMethod(...) { ... }

type MainFooService struct {
	PartialFooService
	pb.UnimplementedFooServer
}

func (m *MainFooService) SomeMethod(...) { return m.PartialFooService.SomeMethod(...) }

OR embed the pb.UnsafeFooService struct instead, which carries the risk of newly added methods breaking your code when you regenerate the _grpc.pb.go file.

@mrothroc
Copy link

mrothroc commented May 10, 2021

The first option is not DRY, breaks the isolation with separate services, and generally defeats the purpose of composition. The second option clearly says it is entirely discouraged.
Since composition is a first class feature of the language, I'm looking for a way to do this that is actively encouraged.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests