-
Notifications
You must be signed in to change notification settings - Fork 7
Support for passing async fn into advertise_service
#108
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
Comments
If you're blocked on this you still have the option of calling |
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 |
@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. |
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 If you look at implementing this: I'd recommend |
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. |
Rust 1.85 is officially out with async closures are fully supported!!! We're ready to start work on this one. |
Do we have an established MSRV target? It's pretty common for the ecosystem to support current stable and the two previous versions. |
README currently calls out 1.75, but I don't think we've got that in the Cargo.tomls yet. My thought here is that we build this feature out under a feature flag ["async-closures"], and when ~1.87 drops we enable that feature as default. I'm open to opinions on the API, but it feels like it is worth keeping the non-async versions around? Have both |
Okay I took a stab at building this out (#227), but did NOT have a lot of luck. Ran into the following issues:
Where you'd like to write: let service = async move |request: std_srvs::SetBoolRequest| -> std::result::Result<
std_srvs::SetBoolResponse,
Box<dyn std::error::Error + Send + Sync>,
> {
tx.send(request.data).await.unwrap();
Ok(std_srvs::SetBoolResponse {
success: true,
message: "You set my bool!".to_string(),
})
}; You instead have to write: let service = move |request: std_srvs::SetBoolRequest| {
let tx = tx.clone();
Box::pin(async move {
tx.send(request.data).await.unwrap();
Ok(std_srvs::SetBoolResponse {
success: true,
message: "You set my bool!".to_string(),
})
})
}; I may have totally screwed something up in the implementation, so would love other eyes on my prototype. But, based on how this initial try wen't I'm not sure I want to move forward with this. |
Just idle speculation as I'm catching this quickly in the email, but would
a possible API be to accept a Fn which accepts a Tokio runtime handle as an
argument and then the user can do something async if they'd like? Then the
runtime context can call tokio::spawn_blocking in case it's CPU heavy.
…On Mon, Mar 31, 2025, 12:09 Carter ***@***.***> wrote:
Okay I took a stab at building this out (#227
<#227>), but did NOT have a
lot of luck. Ran into the following issues:
1. AsyncFn is not dyn compatible, so storing a list of AsyncFns in
some kind of data structure ends up being a massive pain in the ass.
Instead you have to do a bunch of shenanigans to hide the async closures in
a manually created closure that returns a future. This made writing the
feature very challenging, but was surmountable.
2. An async closure which captures ANY context by move is not Fn and
is instead FnOnce. This is because multiple futures can be spawned
from the async closure in parallel and they can't all have ownership over
the moved context at the same time. These leads to fairly horrible
ergonomics for users of the feature.
Where you'd like to write:
let service = async move |request: std_srvs::SetBoolRequest| -> std::result::Result<
std_srvs::SetBoolResponse,
Box<dyn std::error::Error + Send + Sync>,
> {
tx.send(request.data).await.unwrap();
Ok(std_srvs::SetBoolResponse {
success: true,
message: "You set my bool!".to_string(),
})
};
You instead have to write:
let service = move |request: std_srvs::SetBoolRequest| {
let tx = tx.clone();
Box::pin(async move {
tx.send(request.data).await.unwrap();
Ok(std_srvs::SetBoolResponse {
success: true,
message: "You set my bool!".to_string(),
})
})
};
I may have totally screwed something up in the implementation, so would
love other eyes on my prototype. But, based on how this initial try wen't
I'm not sure I want to move forward with this.
—
Reply to this email directly, view it on GitHub
<#108 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACNGDPGHU7CBLWWRBN7XQL32XFSDZAVCNFSM6AAAAABKNP7Y22VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONRWG4YTGOJQGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
[image: Carter12s]*Carter12s* left a comment (RosLibRust/roslibrust#108)
<#108 (comment)>
Okay I took a stab at building this out (#227
<#227>), but did NOT have a
lot of luck. Ran into the following issues:
1. AsyncFn is not dyn compatible, so storing a list of AsyncFns in
some kind of data structure ends up being a massive pain in the ass.
Instead you have to do a bunch of shenanigans to hide the async closures in
a manually created closure that returns a future. This made writing the
feature very challenging, but was surmountable.
2. An async closure which captures ANY context by move is not Fn and
is instead FnOnce. This is because multiple futures can be spawned
from the async closure in parallel and they can't all have ownership over
the moved context at the same time. These leads to fairly horrible
ergonomics for users of the feature.
Where you'd like to write:
let service = async move |request: std_srvs::SetBoolRequest| -> std::result::Result<
std_srvs::SetBoolResponse,
Box<dyn std::error::Error + Send + Sync>,
> {
tx.send(request.data).await.unwrap();
Ok(std_srvs::SetBoolResponse {
success: true,
message: "You set my bool!".to_string(),
})
};
You instead have to write:
let service = move |request: std_srvs::SetBoolRequest| {
let tx = tx.clone();
Box::pin(async move {
tx.send(request.data).await.unwrap();
Ok(std_srvs::SetBoolResponse {
success: true,
message: "You set my bool!".to_string(),
})
})
};
I may have totally screwed something up in the implementation, so would
love other eyes on my prototype. But, based on how this initial try wen't
I'm not sure I want to move forward with this.
—
Reply to this email directly, view it on GitHub
<#108 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACNGDPGHU7CBLWWRBN7XQL32XFSDZAVCNFSM6AAAAABKNP7Y22VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONRWG4YTGOJQGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
^Yeah I think an API that provides a runtime handle, or frankly an API which sets up a pair of channels and makes it easy for the user to spawn a tokio::task() to be the service are both more likely to end up ergonomic than actually taking in an async closure... Would be worth doing a bit of an API study and seeing what other systems have come up with. |
Okay I took a second swing at this based and ultimately decided that having a single Inside the user's service callback they can use To support this change all service functions need to be executing inside a I'd like to continue to improve documentation for this, but I think having an example and some bread-crumbs is good enough for now. |
Roslibrust v0.14.0 published with examples on how to do this. |
Uh oh!
There was an error while loading. Please reload this page.
In the
service_server.rs
example, I've noticed that there is:and
my_service
is a simplefn
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.
The text was updated successfully, but these errors were encountered: