Skip to content

FusedFuture, FusedStream, and select! macro changes #1259

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

Merged
merged 1 commit into from
Oct 19, 2018
Merged

FusedFuture, FusedStream, and select! macro changes #1259

merged 1 commit into from
Oct 19, 2018

Conversation

cramertj
Copy link
Member

This is still very much WIP in order to improve ergonomics of fusing and using the select! macro, but I figured I'd open this PR to see what folks think.

Closes #1219

@cramertj cramertj requested a review from aturon September 12, 2018 07:18
/// or dropped rather than being `poll`ed again.
pub trait FusedFuture {
/// Returns `false` if the underlying future should no longer be polled.
fn can_poll(&self) -> bool;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this is a good name as it is not clear that it implies the future being complete. can_poll initially read to me as a hint for being ready.

is_complete(), is_final, ... would be much better.

Copy link
Member Author

@cramertj cramertj Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried so many darn names for this thing! I had has_completed() for a while, but IMO it's misleading because it sounds like a "complete" future would be one that is done working and ready to return a value, and for streams it's even more unclear what "complete" means, because it's not the point at which it has yielded every element-- it's the point at which the stream has already yielded None.

There's also the interesting case that I was trying to be compatible with where we want to support FuturesUnordered having a mode where can_poll returns false when there are no elements left, rather than repeatedly resolving to None (which is often an uninteresting case). e.g.:

let mut running_tasks = FuturesUnordered::new();
running_tasks.no_none_yield(); // bikeshed away
loop {
    let mut recv = rx.next().fuse();
    select! {
        future(recv as msg) => {
            running_tasks.push(async { ... })
        }
        next(running_tasks as completed_task) => { ... }
        all complete => { return ... }
    }
}

It may be we make a thin wrapper around FuturesUnordered called e.g. TaskSet or something for this, but the point is that for whatever type this is, can_poll refers to something different from "has yielded None already".

I'd love it if you or anyone else has ideas for less confusable names.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_terminated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'd be fine with is_terminated, where "terminated" is something like "has reached a state where polling would yield redundant or inconsistent results, or otherwise misbehave". Does that sound about right to you?

@cramertj
Copy link
Member Author

Updated. Please review. I'm working on a followup PR to reorganize the structure of the select! macro and allow taking streams directly.

@cramertj cramertj requested a review from Nemo157 October 13, 2018 02:13
.idea/misc.xml Outdated
@@ -0,0 +1,9 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this file be checked in?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nah, thanks for catching that.

/// or dropped rather than being `poll`ed again.
pub trait FusedFuture {
/// Returns `true` if the underlying future should no longer be polled.
fn has_terminated(&self) -> bool;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe is_ is more idiomatic than has_

There are a number of is_ fns in the repo already, but I did not find a fn prefixed with has_ (this would be the first.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done.

///
/// `is_terminated` will return `true` if a future should no longer be polled.
/// Usually, this state occurs after `poll` (or `try_poll`) returned
/// `Poll::Ready`. However, `is_terminated` may also return `false` if a future
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This false seems like it should be true?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup (all the booleans got flipped around from an earlier version of this PR).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

/// select! {
/// future(a as a) => total += a,
/// future(b as b) => total += b,
/// all complete => break,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find all complete a little confusing, it makes me think of a branch where all are currently complete and ready to handle, not that they all had completed and been consumed in the past. I wonder if there’s a similarly succinct way to say this that implies that better.

Copy link
Member Author

@cramertj cramertj Oct 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I puzzled on this a bit, but I think we're going to want to change this in the future anyways as we add cases for next(...) etc., so I don't think we need to make a final decision now. If you have ideas, please say so!

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.

3 participants