google / tarpc

An RPC framework for Rust with a focus on ease of use.
MIT License
3.09k stars 189 forks source link

Future returned by `Serve::serve()` is not `Send` #421

Open ditsing opened 5 months ago

ditsing commented 5 months ago

I'm trying to write a function that starts a server with a clonable Serve.

async fn start_tarpc_server<ServeFn>(
    addr: SocketAddr,
    serve: ServeFn,
) -> std::io::Result<()>
    where
        ServeFn:
        tarpc::server::Serve + Send + 'static + Clone,
        ServeFn::Req: Send + 'static + serde::de::DeserializeOwned,
        ServeFn::Resp: Send + 'static + serde::ser::Serialize,
{
    tokio::spawn(serve.serve());
    Ok(())
}

That turns out to be impossible because I cannot call tokio::spawn() on the future returned by Serve::serve. The future is not guaranteed to be Send. Same applies to any future that wraps a serve() future.

This is a well-known problem caused by using aysnc fn in public traits. There is a solution described in the same article. Can we apply the solution here?

tikue commented 5 months ago

Hi, thanks for raising this issue! In the example service, it does spawn the serve futures. Any idea what's different in yours? I suspect the issue is that your code is trying to be generic over multiple services?

ditsing commented 5 months ago

Yes, it is because I'm trying to make a generic function. Although in all generated trait implementations, the future returned by serve() are Send, the signature of serve() itself does not require that. So merely by looking at Serve, the compiler cannot decide whether or not the future is Send.

tikue commented 5 months ago

How large of a library are you writing that is generic over tarpc service functions? And how many tarpc services do you expect to use the library? I'm trying to gauge whether it'd be practical to just write the code once per service.

ditsing commented 5 months ago

Writing code for each service indeed would work for me. I only have two concrete service type to support.

Feel free to close as "working as intended".

tikue commented 5 months ago

Thanks for the details! I'll keep this issue open, because I would like to fix it eventually, potentially just waiting for Rust to offer a more complete solution.

ShaneMurphy2 commented 3 months ago

@tikue what do you think about adding in trait-variant? It fixes this use case (I tested it in my fork), and is officially suggested by the Rust lang team.

Only hiccup is that usage of that macro would conflict with this blanket impl, which I'm not sure of a solution too without specialization or negative bounds.

I'm happy to work through a PR 🙂

tikue commented 3 months ago

@ShaneMurphy2 I'd be happy to review a PR if you want to work on it! Does trait variant need to be used on both Serve and Stub?

ShaneMurphy2 commented 3 months ago

@ShaneMurphy2 I'd be happy to review a PR if you want to work on it! Does trait variant need to be used on both Serve and Stub?

In my opinion it's a little early to have async fn in traits without trait-variant, so I think any public trait should use it.

tikue commented 3 months ago

For the blanket impl of Stub, you could probably add a fn to Serve that converts it to Stub.

ShaneMurphy2 commented 3 months ago

For the blanket impl of Stub, you could probably add a fn to Serve that converts it to Stub.

Are you thinking a signature like this in Serve:

async fn into_stub(self) -> impl Stub;
ShaneMurphy2 commented 3 months ago

I have a WIP branch up here: https://github.com/ShaneMurphy2/tarpc/tree/trait-variant