Generated client structs should implement a Client trait to allow abstraction

[GregOwen]

Feature Request

Crates

tonic-build, possibly tonic

Motivation

I have a service that runs as a cluster of nodes and exposes 2 APIs: an external API that clients use to talk to the cluster and an internal API that the cluster nodes use to talk to each other. In order to keep the internal API separate from the external, I've made each one a separate gRPC service. Using Tonic, this gives me an ExternalClient type and an InternalClient type.

I'd like to be able to write helper code that's generic over the two different flavors of client. The motivating example here is to allow nodes (and external clients) to keep a lazily-initiated cache of connections to all the nodes in the cluster. The external clients will keep a map of ExternalClients and the cluster nodes will keep a map of InternalClients. The cache needs to be able to create new clients, so it needs to be able to call the connect method.

Right now, it looks like the generated code initializes each flavor of client as a separate struct and defines connect on the struct:

pub mod internal_client {
    #![allow(unused_variables, dead_code, missing_docs)]
    use tonic::codegen::*;
    pub struct InternalClient<T> {
        inner: tonic::client::Grpc<T>,
    }
    impl InternalClient<tonic::transport::Channel> {
        /// Attempt to create a new client by connecting to a given endpoint.
        pub async fn connect<D>(dst: D) -> Result<Self, tonic::transport::Error>
        where
            D: std::convert::TryInto<tonic::transport::Endpoint>,
            D::Error: Into<StdError>,
        {
            let conn = tonic::transport::Endpoint::new(dst)?.connect().await?;
            Ok(Self::new(conn))
        }
    }
    impl<T> InternalClient<T>
...

I can get the behavior I want by writing my own CanConnect trait and implementing it manually for InternalClient and ExternalClient, then making my cache take a T: CanConnect type arg, but it would be nice to have a generic Client trait implemented automatically by Tonic (that way it can also include any other methods that end up being common among all clients).

Proposal

1. Add a new public Client trait (possibly as part of tonic::transport::channel?) that defines a connect method (and any other methods that are common across all derived gRPC clients)
2. During codegen, generate an implementation of this trait for each derived gRPC client struct (this could either replace the impl FooClient<tonic::transport::Channel> block or delegate to it)

Alternatives

[olix0r]

Why should the client trait be different from the server-side trait? IMO, it would be extremely useful to have a single trait for a service that is implemented by both client and server implementations so that they may be used interchangeably.

[GregOwen]

You mean why does my application have a separate interface for external clients vs cluster nodes? There are certain cluster-internal messages (e.g. those related to leader election and consensus determination) that external clients should never be able to send, so I've separated them to a different interface.

[olix0r]

Sorry, I should have been clearer: I'm arguing that both generated clients and servers should effectively be able to use one common trait. From the point of view of code that interacts with an API, there's no functional difference between interacting with a remote service and a local/mocked/etc implementation of that service.

For instance, when I generate a tonic server, I get something like the following:

pub mod foo_server {
    #![allow(unused_variables, dead_code, missing_docs)]
    use tonic::codegen::*;
    #[async_trait]
    pub trait Foo: Send + Sync + 'static {
        type FooStream: Stream<Item = Result<super::Bar, tonic::Status>>
            + Send
            + Sync
            + 'static;
        async fn foo(
            &self,
            request: tonic::Request<super::FooRequest>,
        ) -> Result<tonic::Response<Self::FooStream>, tonic::Status>;
    }

Really, this same trait could be implemented by the generated client code as well. I think it would be great if we could extract this trait interface out of the server module and have generated client code implement this same interface.

[GregOwen]

I think you're suggesting something related but distinct. My goal is "I should be able to treat the client of service A the same as the client of service B", whereas yours sounds more like "I should be able to treat the client of service A the same as the server of service A".

The difference is that for my goal the abstracted type can only contain functions that are common to all clients (to my knowledge, this is only connect()), whereas for your goal it sounds like you would want the abstracted type to contain functions that are specific to one service (that is, you would want to be able to call endpoint_a_1 and endpoint_a_2 on either a client or a server of service A). Is that an accurate summary?

[olix0r]

@GregOwen Yes, correct. Thanks for the clarification!

[allada]

I would also like to point that writing unit tests where the output is the GRPC client is quite difficult. Currently there's no easy way to mock out a client without wrapping the grpc client in another layer.

I'm less concerned that they should be the same trait, but more concerned of how difficult it is to write a unit test to ensure my module ends up calling the API of the GRPC connection on certain calls.

Example:
I have a class Worker, which is constructed with the WorkerClient GRPC connection. When the function worker.do_work() is called, I expect worker.worker_client.set_status(Executing) to be called.

Now, I need to write a unit test to ensure my contract is solid. Right now it is quite difficult, I could use an interceptor or something similar and decode the bytes being sent, however, all I want to test is to ensure set_status() was called with Executing.

If there was a trait that Client implemented, it would solve my problem.

[LucioFranco]

For mocking I would checkout the mock example in the examples folder. That should cover most unit testing needs. I don't think introducing more traits solves this problem and introduces more complexity that I on purpose wanted to avoid. Client abstraction is done via tower::Service.

[allada]

That example only shows how to mock out the server side, not the client side.

There's a real life example of the pain here:
https://github.com/allada/turbo-cache/blob/f207dfaf41226ec568720534c1d28ca2d57ef634/cas/worker/local_worker.rs#L263

There's a lot of layers going on here, but what is happening is we create a new LocalWorker and the caller passes the factory of a GRPC client. The LocalWorker will communicate with the server (in this case it's the Scheduler) and they will begin negotiating on work that is being executed or needing to execute. The server sends commands to the client through a stream that is persistently being listened too and respond back to the server with updates through grpc function calls Client->Server.

Now the problem here is I want to unit test my code. I am only wanting to ensure that if I send LocalWorker some work, the worker makes certain calls to the Server (like a WorkCompleted kind of response).

Since right now it's not a trait, this leaves two options, wrap all GRPC client calls in my own wrapper or to actually create a GRPC connection over TCP in my unit test. TCP connections generally really hard to deal with due to port collisions and thus should be generally avoided when possible in unit tests.

In my case I ended up wrapping the client here:
https://github.com/allada/turbo-cache/blob/90cff230ebb5e7982d780f767aa0b0dc85d87b20/cas/worker/worker_api_client_wrapper.rs#L13
And example test of mock being used:
https://github.com/allada/turbo-cache/blob/5daa4ff29373593daad58292d14fbbe2478fc5d7/cas/worker/tests/local_worker_test.rs#L84
(test_context.client.expect_connect_worker means the client is expected to call connect_worker via the GRPC client)

[LucioFranco]

TCP connections generally really hard to deal with due to port collisions and thus should be generally avoided when possible in unit tests.

This why you should use tokio::io::duplex stream from the mock example. This allows you to create an in-memory connection to be able to test on.