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

FusedFuture and FusedStream #1219

Closed
cramertj opened this Issue Aug 20, 2018 · 16 comments

Comments

Projects
None yet
7 participants
@cramertj
Copy link
Collaborator

cramertj commented Aug 20, 2018

The select macro would be far more useful if it was usable inside of loops, like this:

pin_mut!(future_1, future_2, my_stream);
let mut state = ...;
loop {
    let x = select! {
        future_1 => { ...transform state... }
        future_2 => { ...transform state... }
        next(my_stream) as elem => { ... transform state... }
        done => { ... }
    };
    ...some more code doing something with x and state...
}

However, the Stream and Future traits require that they are not polled after completion, and they provide no way to check if completion has already occurred. It's possible to track this extra bit of state in a boolean or something as part of the macro, but then the looping functionality has to be built into the macro, meaning that (1) there would have to be separate select! and select_loop! macros and (2) any shared code or controlflow in the "always" branch would have to be duplicated across branches:

pin_mut!(future_1, future_2, my_stream);
let mut state = ...;
select_loop! {
    future_1 => { let x = ...transform state...; ...some more code doing something with x and state... }
    future_2 => { let x = ...transform state...; ...some more code doing something with x and state... }
    next(my_stream) as elem => { let x = ...transform state...; ...some more code doing something with x and state... }
    done => { let x = ...transform state...; ...some more code doing something with x and state... }
}

Another option would be to add FusedFuture and FusedStream with has_completed() methods, implement FusedFuture and FusedStream for many of the combinators which support them (in particular the fuse() combinator). This is likely more ergonomic for users familiar with how the system works, but could be confusing for new users who now have one more thing to understand (fusing, pinning, etc...).

Yet another option would be to require that all futures track this bit of state and add a has_completed method to the Future and Stream traits. This requires less involvement from end-users, but again increases the already-high ergonomic burden on manual-futures implementors.

Personally, I lean towards the FusedFuture/FusedStream approach as it adds minimal complexity for users who don't care about the select! combinator while still retaining the manual control-flow flexibility of select! + loop rather than select_loop!. However, I'm by no means attached to this idea, and I recognize that it pushes another point of complexity onto end users, which is disappointing. I'd like to hear what y'all think.

cc @seanmonstar @carllerche @withoutboats @aturon @Nemo157 @MajorBreakfast @cavedweller @tkilbourn @smklein

@MajorBreakfast

This comment has been minimized.

Copy link
Member

MajorBreakfast commented Aug 21, 2018

To clarify: The proposed FusedFuture and FusedStream are traits? Would it be Future<Output = ..> + FusedFuture or FusedFuture<Output = ..>? What about Try...?

@cavedweller

This comment has been minimized.

Copy link

cavedweller commented Aug 21, 2018

I'm leaning toward the has_completed() style. I think in practice that method will be rather easy to implement and it won't confuse new users with Fused* Traits.

@Nemo157

This comment has been minimized.

Copy link
Member

Nemo157 commented Aug 21, 2018

If it’s going to be on the main Future trait and required of all implementors would extending Poll to include a Poll::Completed variant instead make more sense?

I think I’m on the side of extra traits for this, I have some futures that don’t currently keep track of when they’re done and basically just restart if polled again after being ready, tracking this extra state could get annoying. Would async change to producing FusedFutures, or would users need to fuse them (I believe it already tracks this so it can panic when polled again)?

@cramertj

This comment has been minimized.

Copy link
Collaborator Author

cramertj commented Aug 21, 2018

@MajorBreakfast

The proposed FusedFuture and FusedStream are traits? Would it be Future<Output = ..> + FusedFuture or FusedFuture<Output = ..>? What about Try...

I had imagined it as something more like trait FusedAsync { fn has_completed(&self) -> bool; }. If you need it as a bound, you could specify T: Future<Output = u8> + FusedAsync or T: TryStream + FusedAsync (WIP naming, obviously).

@cavedweller

I'm leaning toward the has_completed() style. I think in practice that method will be rather easy to implement and it won't confuse new users with Fused* Traits.

To clarify: you're referring to the additional method on Future and Stream traits (as well as TryFuture, TryStream, etc.)? This is the hardest to implement as it would require changing all existing Future and Stream types right off of the bat, but that's not to say that it isn't still the right choice.

@Nemo157

If it’s going to be on the main Future trait and required of all implementors would extending Poll to include a Poll::Completed variant instead make more sense?

Yeah, I thought about that, but I'm concerned that there might be a potential performance regression due to the extra branch everywhere, and there's a lot of code that uses Poll where Completed doesn't make any sense (e.g. poll_read and poll_write methods). Those might be minor enough concerns that we could ignore them, though.

@MajorBreakfast

This comment has been minimized.

Copy link
Member

MajorBreakfast commented Aug 21, 2018

About single FusedAsync trait: I also considered that there could be a single fused trait, however I think if we decide to do this we need two: One for Future and one for Stream. Types that implement both Future and Stream need a 1:1 correspondence. It has to be clear whether the trait impl corresponds to Future, Stream or both.

About Poll::Completed: This doesn't let us check whether the future/stream has completed without polling it.

About integrating it into the Future/Stream traits: The main advantage is of course that the signatures are kept simple because all futures/streams would have the has_completed() method. The main disadvantages are that it makes them unlike Iterator (which also has only one method) and that every manual impl has to implement it.

I think I lean towards the manual fuse technique:

pin_mut!(future_1, future_2, my_stream);
fuse!(future_1, future_2, my_stream); // Returns `future::Fused`s with `has_completed` methods
loop {
    select! { ...

It's simple because it doesn't require special traits and manual future impl also don't have to care about it. Unless there's a performance reason, I think that is the best course of action.

@cramertj

This comment has been minimized.

Copy link
Collaborator Author

cramertj commented Aug 21, 2018

@MajorBreakfast can you clarify what you mean by "manual fuse technique"? Do you mean requiring the types to be wrapped in a literal Fuse structure? I'm opposed to this because it means that every developer would have to fuse every future/stream, creating unnecessary wrappers (since many/most futures are already capable of tracking their completed-ness).

WRT FusedAsync vs. FusedFuture and FusedStream-- I don't really have an opinion here. It's rare for types to implement both Future and Stream, and it seems like it would be even rarer for them to implement has_completed in distinct ways for each, but it doesn't seem like we'd really lose anything by splitting the trait either, so I'm fine w/ that approach.

@seanmonstar

This comment has been minimized.

Copy link
Collaborator

seanmonstar commented Aug 21, 2018

That select loop looks to me that it's a join, right? Join already keeps track of which futures have finished. If there's subtle differences, it seems like this seemingly niche case could make use of the existing fuse() combinators.

  • Adding a required method to Future doesn't (to me) seem to pull its weight, and is odd that a similar trait, Iterator doesn't require it.
  • Adding another variant to the Poll enum will likely hurt optimizations, requiring all polls to handle a 3rd branch.
@MajorBreakfast

This comment has been minimized.

Copy link
Member

MajorBreakfast commented Aug 21, 2018

Do you mean requiring the types to be wrapped in a literal Fuse structure

@cramertj Yes. That solution is certainly the simplest option and I doubt that the impact on performance will ever be measurable. Yes, the state is technically duplicated, but who cares if it doesn't have an impact on performance. It might even be faster if these flags were stored in sequence as an array due to cache locality. A special select_loop! macro could take care of this.

@cramertj

This comment has been minimized.

Copy link
Collaborator Author

cramertj commented Aug 21, 2018

@seanmonstar

That select loop looks to me that it's a join, right? Join already keeps track of which futures have finished. If there's subtle differences, it seems like this seemingly niche case could make use of the existing fuse() combinators.

No, it's not the same as join-- there needs to be a separate state transformation once each future has completed or stream element has been received, often including spawning new tasks to run. This is super common in Fuchsia code right now that uses state-machine style async fns.

Adding a required method to Future doesn't (to me) seem to pull its weight, and is odd that a similar trait, Iterator doesn't require it.

Yeah, the Iterator equivalent is roughly FusedIterator.

Adding another variant to the Poll enum will likely hurt optimizations, requiring all polls to handle a 3rd branch.

+1

@cramertj

This comment has been minimized.

Copy link
Collaborator Author

cramertj commented Aug 21, 2018

@MajorBreakfast

A special select_loop! macro could take care of this.

I proposed that at the start of this conversation, but I'm opposed to that due to the limitations it places on control flow.

@cramertj

This comment has been minimized.

Copy link
Collaborator Author

cramertj commented Aug 25, 2018

Does anyone have any more thoughts on this? I'm still leaning in the direction of FusedFuture and FusedStream, at least as a starting point for experimentation, but I'd love some more ideas. I'll put together a PR unless anyone has objections.

@MajorBreakfast

This comment has been minimized.

Copy link
Member

MajorBreakfast commented Aug 25, 2018

Here are some more random thoughts: In order for this trait to be meaningful, every time we write this:

type MyFuture<'a> = impl Future<Output = T> + 'a; // Notation from RFC 2515
fn foo() -> MyFuture<'a> {
    async { ... }
}

we would need to write this instead:

type MyFuture<'a> = impl Future<Output = T> + FusedFuture + 'a;
fn foo() -> MyFuture<'a> {
    async { ... }
}

otherwise we "forget" about the "fusedness" even though every type returned by an async fn implements it. Send magically adds itself to the trait list because it's an auto trait, but FusedFuture won't. Also, another example for this with a trait definition:

trait Computer {
    type Boot<'a>: Future<Output = ()> + 'a; // Should `FusedFuture` be listed here?
    fn boot(&mut self) -> Self::Boot<'_>;
}

impl Computer for MacBook {
    type Boot<'a> = impl Future<Output = ()> + 'a;  // And here?
    fn boot(&mut self) -> Self::Boot<'_> { async { ... } }
}

What I'm saying is that having to mention this trait everywhere will become tedious and omitting it means that we're back to the status quo.

(Note: I hope that I didn't become confused and wrote something syntactically incorrect. @cramertj feel free to edit this post)


Also, we need to come up with a different name that fits. A "fuse" in electrical engineering means protection against high currents. For FusedIterator it means that next will return None instead of panicing, in particular it does not declare a method. This trait here OTOH does something different: It describes a StatefulFuture, a future that knows whether it has completed or is still going and it declares a method for checking this state.

@smklein

This comment has been minimized.

Copy link

smklein commented Aug 25, 2018

@MajorBreakfast Naming nitpick: Do we expect the direction of Futures to evolve in a direction that they may have additional states other than "Complete" and "Incomplete"?

"StatefulFuture" may be an overloaded term compared to something more boolean. All Futures are state machines (outside of this particular context around completion); I'm hopeful that we can convey a name that encapsulates "can this future be queried for completion".

As @cramertj pointed out, many Futures are already aware of this context without attaching additional state. The real distinction is that proposed has_completed(&self) -> bool fn, which really answers the question "is it safe to try polling this future again".

CheckableFuture? QueryableFuture? CompletionFuture? SelfAwareFuture?

(Not really confident these are better names, just throwing out some ideas...)

@cramertj

This comment has been minimized.

Copy link
Collaborator Author

cramertj commented Aug 27, 2018

I'm -1 to any name that doesn't involve Fuse. This is a term familiar to Rust users through the existing FusedIterator trait and FusedIterator methods. While the exact function signatures are not the same, they both serve to accomplish the same goal and I don't think it's useful to the average user to distinguish between the two.

As to adding FusedFuture as a bound everywhere, IMO if you're calling arbitrary methods you probably wouldn't want to require the user provide Fused and just do it yourself, but I agree this is annoying. This is another case where pushing it as a requirement into the Future trait would be nicest.

@tkilbourn

This comment has been minimized.

Copy link

tkilbourn commented Aug 27, 2018

Is there a way to position the design such that we could use a default implementation on Future so that you only need to override it when you need it?

@cramertj

This comment has been minimized.

Copy link
Collaborator Author

cramertj commented Aug 27, 2018

@tkilbourn You could imagine something like a default method fn has_completed(&self) -> Option<bool>; that defaults to None if no completion is available or not, or a will_know_when_completed method that returns a bool if has_completed is correctly implemented / available, but then you don't know until runtime whether or not you need to be tracking an extra bit for has_completed or not, and there's no way to store that information in the future itself (via requiring a bound like F: FusedFuture).

A close approximation to this approach is the .fuse() combinator for futures (discussed above) which wraps the future in an Option and sets it to None upon completion, returning true from has_completed only if the value is None. The future that results from calling fuse() would be a Fuse<YourOldFuture> (much like the other combinators we have today) and that type would always implement FusedFuture. The unfortunate part of this is that, when a user encounters a future that needs to be fused, they must do so manually since the fusing itself has to happen outside of the select! macro in order to support the looping use-case (for the same reason we can't just store a per-future boolean in the select! macro).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.