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

UTransport::receive should support specifying max-time-to-wait #45

Open
sophokles73 opened this issue Feb 20, 2024 · 4 comments
Open

UTransport::receive should support specifying max-time-to-wait #45

sophokles73 opened this issue Feb 20, 2024 · 4 comments

Comments

@sophokles73
Copy link
Contributor

It is unclear how the UTransport::receive function handles situations where no message is (immediately) available on the given topic, i.e. when will the returned future be completed?

async fn receive(&self, topic: UUri) -> Result<UMessage, UStatus>;

FMPOV the function should accept an argument indicating for how long the client wants to wait, e.g. 0 = fail immediately if no message is available, and t > 0 = fail after t milliseconds without a being message available:

async fn receive(&self, topic: UUri, timeout: u16) -> Result<UMessage, UStatus>;
@sophokles73
Copy link
Contributor Author

@AnotherDaniel @PLeVasseur WDYT?

@AnotherDaniel
Copy link
Contributor

Probably one of the cases where actual API usage data might be interesting ('does anyone really ever use a value different from 0?').
Not against adding it, also not against making a mental note of a potential need but holding off for more hands-on requirements. Otoh - are you already implementing something at the moment where you need this, I guess?

@PLeVasseur
Copy link
Contributor

Interesting thought... Hmmm. Similar to Daniel, I'm wondering if this is something that should be baked into the API.

It seems like the Rust async ecosystem (async-std and tokio) offers solutions to this that'd keep it outside of the uProtocl API.

e.g.

async-std:

use std::time::Duration;

use async_std::future;

let never = future::pending::<()>();
let dur = Duration::from_millis(5);
assert!(future::timeout(dur, never).await.is_err());

tokio

use tokio::time::timeout;
use tokio::sync::oneshot;

use std::time::Duration;

let (tx, rx) = oneshot::channel();

// Wrap the future with a `Timeout` set to expire in 10 milliseconds.
if let Err(_) = timeout(Duration::from_millis(10), rx).await {
    println!("did not receive value within 10 ms");
}

I'm wondering if we maybe start with suggesting these routes in the docs and experimenting with them as needed and see how common this is, in order to consider inclusion.

WDYT?

@sophokles73
Copy link
Contributor Author

Your code examples would only work if the receive method would wait indefinitely for a message to be available. Client code could then decide not to wait any longer for completion of the future. However, if the receive function itself had been implemented by means of a new async task polling the topic, then we would also run into the risk of leaking these tasks because we would also need to make sure that when client code stops waiting for the completion, we also stop the underlying task waiting for the message.

Many messaging systems therefore provide this functionality in their own client polling API as well.

Similar to Daniel, I'm wondering if this is something that should be baked into the API.

For the moment, we should be able to simply assume that if no message is available immediately, we simply return with an error. However, we will need to document this behavior properly in the function's doc comment.

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