Proposal to remove default methods on service traits

[alce]

Crates

build

Motivation

When a service trait is generated, tonic-build generates default methods that return Status::unimplemented. Although this is nice because the server code compiles with an empty trait implementation, we loose the usefulness of the compiler informing us what to do next.

Additionally, the generated methods are not really useful. I don't think many of us would intentionally keep them but can -and most likely will- forget to implement them.

Similarly, when adding new methods and recompiling protos, we loose the nice workflow of relying on the compiler to discover next steps.

As a bonus, some editors have the functionality to generate trait implementations, which we also loose if we have default methods.

Proposal

Update tonic-build to generate service trait definitions only.

[LucioFranco]

I think this is a good idea 👍

[nevi-me]

:( I was busy opening an issue requesting the inverse of this, when I saw this under similar issues. Is it possible to make this optional?

Here's the issue that I was opening, I wasn't aware that earlier versions used to generate the default methods.

---

Feature Request

When creating an RPC service for a proto file with a lot of endpoints, it becomes tedious to implement all the trait members (esp if one wants to implement just a few). Would it be useful to instead create default implementations of all generated methods like is done in other languages (like Java)? Something like the below:

// currently generated code

#[doc = "Generated trait containing gRPC methods that should be implemented for use with GreeterServer."]
#[async_trait]
pub trait Transit: Send + Sync + 'static {
    async fn say_hello(
        &self,
        request: tonic::Request<super::HelloRequest>,
    ) -> Result<tonic::Response<super::HelloReply>, tonic::Status>;
}

// expected generated code

#[doc = "Generated trait containing gRPC methods that should be implemented for use with GreeterServer."]
#[async_trait]
pub trait Transit: Send + Sync + 'static {
    async fn say_hello(
        &self,
        request: tonic::Request<super::HelloRequest>,
    ) -> Result<tonic::Response<super::HelloReply>, tonic::Status> {
        Err(tonic::Status::unimplemented("method not implemented"))
    }
}

Motivation

When implementing a subset of gRPC endpoints in Rust, it could be helpful for the user not to be forced to implement all trait methods (like when implementing an Iterator where things like fn map are optional).
In my case, I have a service that's split across a few languages, with endpoints routed with a load balancer.

Proposal

The generated trait methods should include a default implementation

Alternatives

• User implementing the various endpoints and returning unimplemented() errors
• Commenting out the unused endpoints on the proto service, which is not a good solution if protos are shared on some git submodule

[alce]

Hi @nevi-me

The main motivation behind removing default methods was to force implementations to be explicit, even if that means having a bunch of unimplemented() methods.

Unlike the Iterator trait, Tonic service traits do not have useful natural default implementations. If you implement iterator's next method, you can still have a useful and correct Iterator implementation. This is not the case for services.

I can see how this is inconvenient for your use-case and maybe even for services with a smaller number of methods. However, I would argue that by explicitly implementing a bunch of methods that return Status::unimplemented, the intention is clearer, right there in your source, as opposed to relying on a default implementation that could potentially even change in future releases. You could even implement a 'custom' error to signal your router is failing or what not.

Having said that, I am personally not strongly opposed to bringing default implementations back, but if we do, I believe they should panic, instead of returning Status::unimplemented.

[LucioFranco]

So the big reason we decided to revert the default change was 1) like what @alce said it doesn't fit the same model as Iterator 2) You would still need to define all the type aliases for streaming based ones so you really only get part of the way since we do not yet have default associated types 3) the errors are much better when things are not default since the compiler will tell you what endpoints you are missing. This last one I feel strong about since implementing a new service with defaults you may forget about certain rpc's. I think the initial friction is better. Overall, I tried to make implementing unimplemented endpoints easier but there is only so much we can do with the current trait system.

[nevi-me]

I'm fine with the current behaviour, as I now have more context. We could defer the inconvenience of implementing the methods to IDEs, as when IDE support gets better, this could be a right-click problem.

The thing that made me think of this was actually when I was adding a method to an unrelated Java service.

[alce]

Intellij can generate trait methods for you now if you place the generated code inside /src.

[wchargin]

+1 to permitting this again. This is also the behavior of the Go gRPC
bindings and (I believe) the Python bindings.

IMHO, unimplemented-by-default is consistent with the protocol buffers
philosophy. Just as adding a field to a message is a backward-compatible
change, so too is adding a method to a service. The “zero value” for a
method is that it returns Unimplemented.

Without this default, a backward-compatible proto change leads to an
backward-incompatible Rust API change. This makes it much harder to
evolve our protos, which undermines one of the system’s primary goals.

Regarding IDE support: I use rust-analyzer in my IDE, and rust-analyzer
can generate trait method implementations but it doesn’t work for Tonic
traits even if I set loadOutDirsFromCheck=true*. Placing the generated
code in /src is not an easy workaround, since it requires me to change
my project structure (I compile multiple packages of protos from
different Git submodules) and is inconsistent with the standard
build.rs pattern. Also, this doesn’t suffice for streaming responses,
which force you to declare a type FooStream; auto-generated
implementations can’t provide this.

(* edit: I was wrong about this point and have gotten rust-analyzer
to produce empty impls, but still not stream types)

[LucioFranco]

I agree that unimplemented by default would be nice to have and is consistent with protobufs. That said, I do not see an advantage with the downsides stated above. If you have any streaming requests this unimplemented by default because very confusing with error messages. I would rather people add the impls and then just use Status::unimplemented since this will force them to enumerate all the endpoints and thus discover the types.

RA should be able to auto generate all the trait fn, if it can't do it now it will in the future as it gets better. This is a supported use case.

Without this default, a backward-compatible proto change leads to an
backward-incompatible Rust API change.

A default impl changing is still a breaking change? I don't see how unimplemented by default helps here? That said, users implementing these protobufs are the ones consuming the breaking change so the effect is the least strongest.

This is also the behavior of the Go gRPC
bindings and (I believe) the Python bindings

I also personally think this is kinda a null point since we are not go/python. I am not sure if java does this or not? I'd rather keep this crate more aligned with rust than anything else and default fn here imo are not idiomatic. They would be idiomatic if they were helper fn but they are apart of the core functionality.

I spent a lot of time thinking about how to make the default work but to me it just doesn't fit well and the fact that we don't have default associated types makes this worse. I think we can reconsider once those have been implemented.

[alce]

The Go implementation has this behavior to guard against a nil pointer dereference. If you add a method to your service definition but do not provide a value for the corresponding field in the service struct, the compiler won't catch this and the server will crash when trying to call the function.

To prevent this, the generated code checks that the handler value is not nil and that it satisfies the service's interface. If the value is nil, it returns Unimplemented, because there is not much it can do at that point.

We don't need this behavior in Tonic because can have these two checks at compile time.

In terms of service and schema evolution, if a client does not know about a method is ok. If a client does not know about a couple of fields is also ok. But a server that just returns Unimplemented is not. It is 'almost' the same behavior as removing a method from the server's implementation, which is a breaking change.

[LucioFranco]

The client, is not affected because there is no trait to have a impl default on anyways. This only talks about the server side iirc.

[alce]

@LucioFranco Agree. But what I'm trying to say is that if a client knows about a method it will call it and having a server that by default returns Unimplemented is just a failure waiting to happen. If we can prevent this at compile time, why not?

[wchargin]

Thanks for your consideration!

I think our philosophical disagreement is that, from my perspective,
“having a server that by default returns Unimplemented” is not
necessarily just a failure waiting to happen. Returning Unimplemented
is a valid implementation of the protocol, clearly describes the actual
situation, and makes it easy to upgrade gRPC service definitions.

But I’ll defer to your judgment as maintainers here, so let me just say
thanks for your work on Tonic: I’ve been pretty happy with it so far.
:-)

[LucioFranco]

I hope my early morning response wasn't too harsh 👍

Yeah, my big issue is that with the fact that streams still require you to implement something really removes the benefit by a large enough amount that it makes me not want to do it. Maybe, in the future we can support it once rust gets the features we need.

[xanather]

:( I was busy opening an issue requesting the inverse of this, when I saw this under similar issues. Is it possible to make this optional?

Here's the issue that I was opening, I wasn't aware that earlier versions used to generate the default methods.

Feature Request

When creating an RPC service for a proto file with a lot of endpoints, it becomes tedious to implement all the trait members (esp if one wants to implement just a few). Would it be useful to instead create default implementations of all generated methods like is done in other languages (like Java)? Something like the below:

// currently generated code

#[doc = "Generated trait containing gRPC methods that should be implemented for use with GreeterServer."]
#[async_trait]
pub trait Transit: Send + Sync + 'static {
    async fn say_hello(
        &self,
        request: tonic::Request<super::HelloRequest>,
    ) -> Result<tonic::Response<super::HelloReply>, tonic::Status>;
}

// expected generated code

#[doc = "Generated trait containing gRPC methods that should be implemented for use with GreeterServer."]
#[async_trait]
pub trait Transit: Send + Sync + 'static {
    async fn say_hello(
        &self,
        request: tonic::Request<super::HelloRequest>,
    ) -> Result<tonic::Response<super::HelloReply>, tonic::Status> {
        Err(tonic::Status::unimplemented("method not implemented"))
    }
}

Motivation

When implementing a subset of gRPC endpoints in Rust, it could be helpful for the user not to be forced to implement all trait methods (like when implementing an Iterator where things like fn map are optional). In my case, I have a service that's split across a few languages, with endpoints routed with a load balancer.

Proposal

The generated trait methods should include a default implementation

Alternatives

* User implementing the various endpoints and returning `unimplemented()` errors

* Commenting out the unused endpoints on the proto service, which is not a good solution if protos are shared on some git submodule

See #1347 for the inverse of this.