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

Multiple Service Definition Traits #499

Open
MohenjoDaro opened this issue Dec 3, 2024 · 3 comments
Open

Multiple Service Definition Traits #499

MohenjoDaro opened this issue Dec 3, 2024 · 3 comments

Comments

@MohenjoDaro
Copy link

Having multiple service definition traits on a single server/listener doesn't seem to be supported, though there's nothing mentioned about it in the docs. Basically adding a additional trait like this to the lib

#[tarpc::service]
pub trait Ping {
    /// Returns a greeting for name.
    async fn ping() -> Result<(), String>;
}

and adding it to the server as well like

impl Check for HelloServer {
    async fn ping(self, _: context::Context) -> Result<(), String> {
        Ok(())
    }
}

will cause an error with the serve() code since it doesn't know which trait you want to serve unless you specify. This can cause weird behaviors where having the same function in service definition A and B only calls the function in A and never B, or a function in B failing because it doesn't exist in A, and the server is serving A and not B. And since the function is named .serve() for all the service definitions, it's hard to tell which one if being served by the listener when looking at the code

I don't know if this is actually an issue/bug or not, whether it's gone unnoticed so far, or what. But, I thought I'd bring it up since it doesn't appear to be mentioned in the docs, and took me awhile to figure out this was the cause of my problem since I have my service definitions spread out more than I should it seems

@tikue
Copy link
Collaborator

tikue commented Dec 3, 2024

Hi! Can you show the whole code, including the code where .serve() is called?

@MohenjoDaro
Copy link
Author

I used the example-service code from the repo, but it will be easier for me to say the changes than make a repo for it
In the lib.rs file, I added

#[tarpc::service]
pub trait World {
    /// Returns a greeting for name.
    async fn hello(name: String) -> String;
}

#[tarpc::service]
pub trait Check {
    /// Returns a greeting for name.
    async fn ping() -> Result<(), String>;
}

In the server.rs file I added

#[derive(Clone)]
struct HelloServer(SocketAddr);

impl World for HelloServer {
    async fn hello(self, _: context::Context, name: String) -> String {
        let sleep_time =
            Duration::from_millis(Uniform::new_inclusive(1, 10).sample(&mut thread_rng()));
        time::sleep(sleep_time).await;
        format!("Hello, {name}! You are connected from {}", self.0)
    }
}

impl Check for HelloServer {
    async fn ping(self, _: context::Context) -> Result<(), String> {
        Ok(())
    }
}

I didn't change anything with the serve(). It's still

        .map(|channel| {
            let server = HelloServer(channel.transport().peer_addr().unwrap());
            channel.execute(server.serve()).for_each(spawn)
        })

And this throws an error on .serve() since you have to choose which listener serve() to use. If you pick one, then hte error goes away and you use that listener trait

On the client, change the WorldClient to CheckClient, then you can use ping, however you run into an error due to the server using a different trait

This makes sense when you understand how the listener works, however since HelloServer(channel.transport().peer_addr().unwrap()); (imo) looks like it should have access to all its impl serve() functions, you can run into the issue of the server listening to one trait, and the client sending another

Hopefully this is clear enough

@tikue
Copy link
Collaborator

tikue commented Dec 6, 2024

Got it, thanks! I guess there are two sides to this:

  1. improving the documentation to clarify that today only one service can use a transport, and showing some workarounds.
  2. the feature request to support multiple services using the same transport.

#2 is not planned to be supported, but #1 is definitely doable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants