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

Concurrent client requests #33

Closed
frol opened this issue Oct 3, 2019 · 12 comments
Closed

Concurrent client requests #33

frol opened this issue Oct 3, 2019 · 12 comments
Labels
C-question Category: Further information is requested

Comments

@frol
Copy link

frol commented Oct 3, 2019

Feature Request

Motivation

It is common to have a need to do concurrent client requests, but it seems that Grpc client dispatcher is implemented to handle one call at a time, so the following example does not work by design:

use futures::join;

pub mod hello_world {
    tonic::include_proto!("helloworld");
}

use hello_world::{client::GreeterClient, HelloRequest};

#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
    let mut client = GreeterClient::connect("http://[::1]:50051")?;

    let request1 = tonic::Request::new(HelloRequest { name: "hello".into() });
    let res1 = client.say_hello(request1);
    
    let request2 = tonic::Request::new(HelloRequest { name: "hello".into() });
    let res2 = client.say_hello(request2);
    
    println!("RESPONSES={:?}", join!(res1, res2));

    Ok(())
}
error[E0499]: cannot borrow `client` as mutable more than once at a time
  --> tonic-examples/src/helloworld/client.rs:17:16
   |
14 |     let res1 = client.say_hello(request1);
   |                ------ first mutable borrow occurs here
...
17 |     let res2 = client.say_hello(request2);
   |                ^^^^^^ second mutable borrow occurs here
18 |
19 |     println!("RESPONSES={:?}", join!(res1, res2));
   |                                      ---- first borrow later used here

Proposal

It seems that there is a need for multiplexing and a pool of clients.

@zackangelo
Copy link

@frol I believe that the .clone() operation on a tonic client should be relatively lightweight (just a handle to the underlying channel). You should be able to dispatch a request in parallel by cloning the client before executing its method.

@LucioFranco
Copy link
Member

@zackangelo is correct!

So pretty much this comes down to how tower handles back pressure. Internally we need a &mut self within the future to keep checking if our inner service is ready to accept the next request. To do this we need to borrow the service exclusively because only one access to that service can submit the request when its ready. To fix this internally we use a buffer channel to allow you to multiplex this concept of polling ready and handling back pressure. Hopefully this explains it a bit better.

@frol
Copy link
Author

frol commented Oct 4, 2019

Thank you for the thorough explanation!

Just to confirm, I was able to use .clone() just fine:

use futures::join;

pub mod hello_world {
    tonic::include_proto!("helloworld");
}

use hello_world::{client::GreeterClient, HelloRequest};

#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
    let main_client = GreeterClient::connect("http://[::1]:50051")?;

    let mut client = main_client.clone();
    let res1 = client.say_hello(tonic::Request::new(HelloRequest { name: "hello1".into() }));
    
    let mut client = main_client.clone();
    let res2 = client.say_hello(tonic::Request::new(HelloRequest { name: "hello2".into() }));
    
    println!("RESPONSES={:?}", join!(res1, res2));

    Ok(())
}

Could you help me with a solution to fetch a list of resources concurrently (e.g. I have a vector of tonic::Requests)? I have tried to use futures::future::join_all:

pub mod hello_world {
    tonic::include_proto!("helloworld");
}

use hello_world::{client::GreeterClient, HelloRequest};

#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
    let main_client = GreeterClient::connect("http://[::1]:50051")?;

    let mut client = main_client.clone();
    let res1 = client.say_hello(tonic::Request::new(HelloRequest { name: "hello1".into() }));
    
    let mut client = main_client.clone();
    let res2 = client.say_hello(tonic::Request::new(HelloRequest { name: "hello2".into() }));
    
    println!("RESPONSES={:?}", futures::future::join_all(vec![res1, res2]).await);

    Ok(())
}
error[E0597]: `client` does not live long enough
 --> tonic-examples/src/helloworld/client.rs:17:16
  |
17|     let res2 = client.say_hello(tonic::Request::new(HelloRequest { name: "hello2".into() }));
  |                ^^^^^^ borrowed value does not live long enough
...
22| }
  | -
  | |
  | `client` dropped here while still borrowed
  | borrow might be used here, when `res1` is dropped and runs the destructor for type `impl core::future::future::Future`

And even if that would work, I would still need to store the clients somewhere in a list, right?

@LucioFranco LucioFranco added the C-question Category: Further information is requested label Oct 4, 2019
@alce
Copy link
Collaborator

alce commented Oct 10, 2019

@frol this works:

#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
    let main_client = GreeterClient::connect("http://[::1]:50051")?;

    let mut client1 = main_client.clone();
    let mut client2 = main_client.clone();

    let res1 = client1.say_hello(tonic::Request::new(HelloRequest {
        name: "hello".into(),
    }));

    let res2 = client2.say_hello(tonic::Request::new(HelloRequest {
        name: "world".into(),
    }));

    let responses = futures::future::try_join_all(vec![res1, res2]).await?;

    println!("RESPONSE={:?}", responses);

    Ok(())
}

@blittable
Copy link
Contributor

blittable commented Oct 18, 2019

Super-helpful... thank you.

If it was in a loop? I'm generating the req/resp from the protobufs, and tonic::response::Response - I cannot clone it.

This definitely inspired me to test the streaming approach, which works swimmingly. But, a batch of requests and handling their responses, does this require an Arc<Mutex> and pinning around the data-structure?

@alce
Copy link
Collaborator

alce commented Oct 18, 2019

@blittable see #44 (comment) for a possible solution

@LucioFranco LucioFranco added this to the 0.1 milestone Dec 12, 2019
@LucioFranco
Copy link
Member

I think we can close this, feel free to reopen if you have any more questions :)

@LucioFranco LucioFranco removed this from the 0.1 milestone Dec 12, 2019
@ettersi
Copy link

ettersi commented Mar 4, 2022

Rust and tonic noob here, but if client.clone() is cheap and required frequently, wouldn't it be easier if client types had the Copy trait and client calls would take self by value instead of mutable reference?

@davidpdrsn
Copy link
Member

@ettersi Copy is for types that can be copied by simplying copying their byte representation, such as i32. Two i32s are identical if the contain the same bytes. Whereas something like Box<String> is different. You cannot get a clone of the box by just copying the boxes bytes on the stack. There is memory on the heap to deal with. So Copy doesn't simply mean "cheap to clone".

@ettersi
Copy link

ettersi commented Mar 4, 2022

Right, I forgot that it has to be a bitwise copy. Sorry about the spam, then.

@gabrik
Copy link

gabrik commented Sep 1, 2023

Quick question, given that the Clone is lightweight, why does the client need &mut self instead of &self ?

@horacimacias
Copy link

@gabrik see previous info #33 (comment)

hydra-yse added a commit to breez/breez-sdk-liquid that referenced this issue Oct 17, 2024
We ensure the client is cloned as it is a lightweight clone and enables
concurrency. See hyperium/tonic#33
hydra-yse added a commit to breez/breez-sdk-liquid that referenced this issue Oct 17, 2024
We ensure the client is cloned as it is a lightweight clone and enables
concurrency. See hyperium/tonic#33
hydra-yse added a commit to breez/breez-sdk-liquid that referenced this issue Oct 21, 2024
We ensure the client is cloned as it is a lightweight clone and enables
concurrency. See hyperium/tonic#33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-question Category: Further information is requested
Projects
None yet
Development

No branches or pull requests

9 participants