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

Add SendFut::into_inner. #126

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NathanSWard
Copy link

This is the first step for adding a send_timeout_async and send_deadling_async.
This provides the ability to consume a SendFut returning its contained value if the value has not yet been sent.

@zesterer
Copy link
Owner

This looks good to me! I initially turned my nose up at the name into_inner, but on second thoughts I think you're right that this does actually follow the conventions set by other types in the Rust ecosystem.

@zesterer
Copy link
Owner

At least now, building a timeout on top is a relatively trivial job. In terms of implementing that, I think the most lightweight and sensible solution - based on a cursory reading - is to pull in the async-io crate as an optional dependency and use its timers (perhaps behind some sort of async_timeout feature in flume). What do you think?

@NathanSWard
Copy link
Author

Yep, it mostly just depends on how you want the API to look either
A) having the function be marked async

pub async fn send_timeout_async(&self, message: T, dur: Duration) -> Result<(), ErrorType>;

B) having the function return some concrete type that implements Future

pub fn send_timeout_async(&self, message: T, dur: Duration) -> TimeoutFut;

In the case of A we could just use async_std::future::timeout I believe, and then in case B, async-io would be a good solution yes!

@zesterer
Copy link
Owner

I think B is probably the way to go. It's an underrated thing, but being able to name the type of a feature is occasionally very important, particularly if you're building an API on top of the thing.

@NathanSWard
Copy link
Author

@zesterer Do you want these as separate MRs, or should I add the timeout functions here in this MR?

@NathanSWard
Copy link
Author

It looks like the actual implementation of the _async functions make take a bit of extra work.
Notably, the Hook type has a todo comment: // TODO: Investigate some sort of invalidation flag for timeouts.

When writing tests, for the implementation, there is a panic coming from the fire_recv function which always unwraps the inner value.

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

Successfully merging this pull request may close these issues.

2 participants