Storing with_interceptor clients is substantially more challenging in 0.5

[Sushisource]

Feature Request

Crates

tonic

Motivation

Previously, I could simply store a FooServiceClient<tonic::transport::Channel> without the need for generics or boxing in a struct, and use that. I do so to encapsulate access to the client, partly for mocking reasons, partly for enforcing certain invariants or making the API simpler.

Now, the type is something like FooServiceClient<tonic::codegen::InterceptedService<Channel, impl Fn<(tonic::Request<()>,)>>> which is a much bigger pain to store.

Proposal

Kind of like #671, but not quite, it would be nice if there was simply a FooServiceClient trait with all the rpc methods on it that the generated structs would implement (using async_trait, maybe opt-in). This would make storage and mocking much simpler.

Alternatives

Beyond having a full client trait, a trait that combines all the bounds in the big where clause on generated clients would also make this much easier. I'm referencing:

    impl<T> FooServiceClient<T>
    where
        T: tonic::client::GrpcService<tonic::body::BoxBody>,
        T::ResponseBody: Body + Send + Sync + 'static,
        T::Error: Into<StdError>,
        <T::ResponseBody as Body>::Error: Into<StdError> + Send,

[davidpdrsn]

I think the right solution for this is #713. Then you can define interceptors as nameable types without using closures.

[Sushisource]

I think the right solution for this is #713. Then you can define interceptors as nameable types without using closures.

Yeah that looks like a great way to solve the problem.

[mattoni]

What's the correct way to use #713 to have a client implementation as a struct field?

[yotamofek]

What's the correct way to use #713 to have a client implementation as a struct field?

@mattoni you mean something like this?

struct MyInterceptor {}

impl Interceptor for MyInterceptor {
    fn call(&mut self, request: tonic::Request<()>) -> Result<tonic::Request<()>, Status> {
        todo();
    }
}

struct ClientWrapper<S: Service> {
    client: InterceptedService<S, MyInterceptor>,
}

[davidpdrsn]

This was fixed in #713 and released as part of tonic 0.5.1. I also added some examples here https://github.com/hyperium/tonic/blob/master/examples/src/interceptor/client.rs. Let me know if you have more questions!

[mattoni]

Awesome, thanks for the examples @davidpdrsn . One quick question, I noticed you're passing the struct in exactly here:

  let client: GreeterClient<InterceptedService<Channel, MyInterceptor>> =
      GreeterClient::with_interceptor(channel, MyInterceptor);

However, when attempting that I get the following:

I've rebuilt with the latest tonic-build: 0.5.1 and still get the same thing. The generated service signature is this:

and my AuthService is implemented as this:

How can I get it to follow your example?

Thanks!

[davidpdrsn]

Are you certain you've updated to tonic 0.5.1 (not tonic-build)? It looks like you're still on 0.5.0.

[mattoni]

Looks like my old build artifacts were still cached even though I had done a cargo clean? Anyway, working now. Cheers!

[mattoni]

Actually I spoke too soon. I switched to using the git dependency directly and it's still having the same issue in the generated code. The actual interceptor definition looks correct, but generated params are still wrong:

here are the deps I have listed. Did a cargo clean and a fresh cargo build and got the error.

[davidpdrsn]

No idea whats wrong. The code on GitHub looks correct.

[davidpdrsn]

Are you able to reproduce it in a smaller example? If so, can make a repo with the code, then I'll take a look.

[mattoni]

So I found the issue - I set tonic-build to the git repo version and it worked. looks like 0.5.1 generates the wrong code.

[davidpdrsn]

omg you're right... I'm sorry. This is my fault. When I made the release yesterday I forgot to ship tonic-build. Gonna get it out today.

[davidpdrsn]

@mattoni Thanks for noticing. Tonic and tonic-build 0.5.2 are on crates.io now. I've tested things myself and everything seems to be working.

[mattoni]

Thanks so much! It works perfectly now.

[lacasaprivata2]

One thing that is much harder now as a tradeoff is capturing the variable outside the function

[lacasaprivata2]

tough.....

type Intercept = Box<dyn Fn(Request<()>) -> Result<Request<()>, Status>>;
pub fn api(actor_id: &String) -> CatalogApiClient<InterceptedService<Channel, Intercept>> {
  let actor = actor_id.clone();
  CatalogApiClient::with_interceptor(channel::io.clone(), Box::new(move |mut ostream: Request<()>| {
    tokenize(&actor, &mut ostream);
    Ok(ostream)
  }))
}

[davidpdrsn]

Please try and be more constructive than "gross". Actual people worked hard on this feature.

You're right that its a tradeoff. The new interceptor design made new things possible that hadn't been possible before. The feature you're asking about is still possible but requires typing a few more characters. Seems like a good tradeoff to me.

In that specific case I think implementing the Interceptor trait would be easier for you.

[lacasaprivata2]

I was referring to my own code; interceptor trait included above

Many approaches

(1) define Boxed closure (2) add to metadata map w/ a hack the input data then transform in middleware