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

Support for passing async fn into advertise_service #108

Open
entire opened this issue Aug 23, 2023 · 5 comments
Open

Support for passing async fn into advertise_service #108

entire opened this issue Aug 23, 2023 · 5 comments
Labels
enhancement New feature or request

Comments

@entire
Copy link
Contributor

entire commented Aug 23, 2023

In the service_server.rs example, I've noticed that there is:

    let _handle = client
        .advertise_service::<std_srvs::SetBool>("/my_set_bool", my_service)
        .await?;

and my_service is a simple fn here, but many services are often going to require async/await type functions to run within the function. Is there plan to support async fn here?

If not, I'm happy to look into creating a PR to add support for this especially because I'd like to use async functions within my service callback that I pass in.

@ssnover
Copy link
Collaborator

ssnover commented Aug 23, 2023

If you're blocked on this you still have the option of calling tokio::spawn here.

@Carter12s
Copy link
Collaborator

I think it is definitely something we'd like to support if possible. It shouldn't be too hard to try experimenting with our internal type erasure to see if this works, but I'm suspicious we're going to hit some limitation of async fn in rust that might require a more significant overhaul. If you'd like to dig on on PR trying to implement it @entire that would be awesome. If not, I'll probably take a look at it in the next week or so.

@Carter12s Carter12s added the enhancement New feature or request label Aug 23, 2023
@entire
Copy link
Contributor Author

entire commented Aug 24, 2023

@ssnover agreed - i think it would just be nicer overall though for the functions to be async in general though.

@Carter12s - thanks, makes sense. let me take a look at it and I'll take a stab at it if it seems relatively not difficult.

@ssnover
Copy link
Collaborator

ssnover commented Sep 7, 2023

Having glanced at this for a bit, the main challenge is our paradigm around type erasure with the closure. Async closures are unstable, which means the message: &str parameter will have to find another way in.

If you look at implementing this: I'd recommend tokio::spawn with a channel. Wrap the tokio::JoinHandle in an abort_on_drop::ChildTask and add that to the struct that gets store in the services map along with the sender side of the channel for messages.

@Carter12s
Copy link
Collaborator

Update!

Async closures have merged! rust-lang/rust#132706

We should be able to build this out once the next version of rust ships (could build it out on nightly right now).

Once built we can put it behind a feature flag to not affect out min rust version too much. But super excited this is coming.

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

No branches or pull requests

3 participants