eclipse-uprotocol / up-rust

uProtocol Language Specific Library for Rust
Apache License 2.0
11 stars 9 forks source link

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

Open sophokles73 opened 7 months ago

sophokles73 commented 7 months ago

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 commented 7 months ago

@AnotherDaniel @PLeVasseur WDYT?

AnotherDaniel commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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.