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

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

Open
ditsing opened this issue Jan 28, 2024 · 11 comments
Open

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

ditsing opened this issue Jan 28, 2024 · 11 comments

Comments

@ditsing
Copy link

ditsing commented Jan 28, 2024

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
Copy link
Collaborator

tikue commented Jan 28, 2024

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
Copy link
Author

ditsing commented Jan 28, 2024

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.

ditsing added a commit to ditsing/ruaft that referenced this issue Jan 28, 2024
Skipping tarpc 0.34 which introduced a breaking change. See
google/tarpc#421.
@tikue
Copy link
Collaborator

tikue commented Feb 4, 2024

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
Copy link
Author

ditsing commented Feb 4, 2024

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
Copy link
Collaborator

tikue commented Feb 4, 2024

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
Copy link
Contributor

ShaneMurphy2 commented Mar 15, 2024

@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
Copy link
Collaborator

tikue commented Mar 20, 2024

@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
Copy link
Contributor

@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
Copy link
Collaborator

tikue commented Mar 20, 2024

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

@ShaneMurphy2
Copy link
Contributor

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
Copy link
Contributor

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

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

3 participants