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

Improve ergonomics when passing Clients as function arguments #110

Open
daniel5151 opened this issue Oct 31, 2019 · 10 comments
Open

Improve ergonomics when passing Clients as function arguments #110

daniel5151 opened this issue Oct 31, 2019 · 10 comments
Labels
A-tonic C-enhancement Category: New feature or request E-help-wanted Call for participation: Help is requested to fix this issue. T-docs Improvements or additions to documentation

Comments

@daniel5151
Copy link
Contributor

daniel5151 commented Oct 31, 2019

Feature Request

Motivation

At the moment, any function that takes a Client as a parameter requires specifying some pretty scary-looking type bounds:

async fn foo<T>(mut client: MyClient<T>)
where
    T: tonic::client::GrpcService<tonic::body::BoxBody>,
    T::ResponseBody: tonic::codegen::Body + tonic::codegen::HttpBody + Send + 'static,
    T::Error: Into<tonic::codegen::StdError>,
    <T::ResponseBody as tonic::codegen::HttpBody>::Error: Into<tonic::codegen::StdError> + Send,
    <T::ResponseBody as tonic::codegen::HttpBody>::Data: Into<bytes::Bytes> + Send,
{ ... }

It would be nice to somehow simplify these sorts of declarations.

Proposal

I'm not entirely sure how to work around this issue!

A heavyweight solution might be to write a proc macro that automagically includes these type bounds on a function. e.g:

#[tonic::clientbounds(T)]
async fn foo<T>(mut client: MyClient<T>) { ... }

Alternatives

There are probably other, simpler solutions, but I can't think of any great ones off the top of my head. That said, I don't have a thorough understanding of tonic internals, so maybe there is an easy fix.

@alce
Copy link
Collaborator

alce commented Oct 31, 2019

An easy workaround is to use a concrete type instead of the type parameter. Currently, I believe the only concrete type that can be used to construct a client is a Channel, so you could do:

async fn foo(mut client: MyClient<Channel>) {...}

Here's an example:
https://github.com/hyperium/tonic/blob/master/tonic-examples/src/routeguide/client.rs#L17

@daniel5151
Copy link
Contributor Author

daniel5151 commented Oct 31, 2019

Huh, that totally solves my particular issue. Not sure why I didn't think of that earlier!
Thanks! 😄

That said, there are times when it might be useful to write transport-independent functions. For example, a API might require first calling client.update_foo(), and if that fails, falling back to client.create_foo(). It would be nice to easily write a helper function update_or_create_foo<C>(client: FooClient<C>) that orchestrates those gRPC calls.

@alce
Copy link
Collaborator

alce commented Oct 31, 2019

I agree, you raise a good point. I believe there are plans (and maybe even some initial work) to provide different transports, in which case we may need to find a different solution.

daniel5151 added a commit to daniel5151/iotedge that referenced this issue Nov 1, 2019
These bounds work, but will result in some broken code once we switch to
a proper sockets-based transport.

I've opened an issue upstream, since it would be nice to keep the more
generic code, while somehow avoiding boilerplate.

hyperium/tonic#110
@LucioFranco
Copy link
Member

@daniel5151 yeah, so I tried to get https://docs.rs/tonic/0.1.0-alpha.5/tonic/client/trait.GrpcService.html to do as much of that work as possible but it seemed that rust was not happen. Sounds like this would be a good thing to revisit.

@LucioFranco LucioFranco added A-tonic T-docs Improvements or additions to documentation C-enhancement Category: New feature or request E-help-wanted Call for participation: Help is requested to fix this issue. labels Nov 2, 2019
daniel5151 added a commit to daniel5151/iotedge that referenced this issue Nov 4, 2019
These bounds work, but will result in some broken code once we switch to
a proper sockets-based transport.

I've opened an issue upstream, since it would be nice to keep the more
generic code, while somehow avoiding boilerplate.

hyperium/tonic#110
daniel5151 added a commit to daniel5151/iotedge that referenced this issue Nov 6, 2019
These bounds work, but will result in some broken code once we switch to
a proper sockets-based transport.

I've opened an issue upstream, since it would be nice to keep the more
generic code, while somehow avoiding boilerplate.

hyperium/tonic#110
@abbec
Copy link

abbec commented Nov 23, 2021

So, what @alce said (about Channel being the only thing you can create a client with) is not really true anymore and with Interceptors this quickly becomes unwieldy since you can get types like: Client<tonic::codegen::InterceptedService<HttpStatusInterceptor, [closure@src/registry.rs:83:9: 126:10]>>. Even returning this from a function is a no-go :)

@davidpdrsn
Copy link
Member

Interceptors can be made using named types by implementing the Interceptor trait. There are a few examples here. It's not the most ergonomic thing ever but at least its possible and can be cleaned up by defining your own type alias.

@abbec
Copy link

abbec commented Nov 23, 2021

Sure, but that will only get rid of the closure part so it is (as you said) still not really "ergonomic" :)

@domesticsimian
Copy link

domesticsimian commented Sep 12, 2023

In 2023 I don't think much has changed in terms of making naming client types any easier. Is there any interest in having the generated client implement the generated service trait?

If so, I'm happy to contribute.

@blinsay
Copy link

blinsay commented Feb 13, 2024

I'm running into this while trying to use Tower middleware. Using a service builder means that I now have a pretty deeply nested type instead of a channel. The following example spits outs a Timeout<InterceptedService<InterceptedService<Channel, {closure@agent.rs:140:44}>, ...>>>.

    let channel = ServiceBuilder::new()
        .timeout(Duration::from_secs(30))
        .layer(tonic::service::interceptor(my_cool_interceptor(
            ctx.clone(),
        )))
        .layer(tonic::service::interceptor(|req| Ok(req)))
        .service(channel);

Some (generated?) documentation about the bounds to include when passing a client to a fn would be awesome.

@a10y
Copy link

a10y commented Mar 13, 2024

Just chiming in, I'm also hitting this issue in the context of trying to store the tonic client inside of my own struct.

pub struct SchemaClient<T> {
	inner: SchemaServiceClient<T>
}

I then use a tower ServiceBuilder to setup the channel. I want to be able to configure some layers to be added/removed based on on some configs, but changing the layers seems to change my T which makes it really hard to store one of these things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tonic C-enhancement Category: New feature or request E-help-wanted Call for participation: Help is requested to fix this issue. T-docs Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

8 participants