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 - should this really be a separate trait? #1894

Open
Matthias247 opened this issue Oct 1, 2019 · 1 comment
Open

FusedFuture - should this really be a separate trait? #1894

Matthias247 opened this issue Oct 1, 2019 · 1 comment
Labels
A-future Area: futures::future

Comments

@Matthias247
Copy link
Contributor

select! is awesome. But we require FusedFutures for it. That currently introduces a few downsides:

  • Libraries must all depend futures-core in order to make their futures support select! without going the extra .fuse() way. Lots of libraries which just build some fundamental futures might otherwise not need that.
  • We have everywhere multiple trait bounds. One for Future and one for FusedFuture.
  • It makes it not convenient to create fuse wrappers around Futures which might not might not be FusedFutures. .fuse() actually is broken for cases where the inner future is already a FusedFuture and is terminated. Then it shouldn't be called again, but the Fuse will tell us it's ok. The problem might get solved via specialization, if we can let fuse() return something different depending on whether the receiver is already a FusedFuture or not. But doesn't seem to be on the near-term roadmap.

One thing which I thought would be great when working on the select! improvements is to also add the ability to automatically fuse futures if they are provided as expressions instead of a path. So that this would work without adding any .fuse():

async fn make_fut() {
    5
}

select! {
    x = make_fut() => x, // no .fuse()
}

which doesn't work with the current approach.

Therefore I wondered we could somehow make this work by moving the fusing concept somewhere else. The how is more of a brainstorm that a good proposal. But what I thought could work is adding is_terminated and another method that tells us whether the Future naturally supports fusing to Future itself, and provide default implementations for backward compatibility. Those could look like:

trait Future {
    // ... existing stuff

    /// This equals the default behavior of futures, that they can be polled after termination
    fn is_terminated(&self) -> { false } 
    /// Whether the Future implementation provides an exact termination status
    fn supports_termination_status(&self) -> bool { false }
}

We could then implement a generically correct .fuse() -> Fuse with:

struct Fuse<T> {
    inner: T,
    is_terminated: bool,
}

impl<T> Fuse<T> {
    fn is_terminated(&self) {
        (inner.supports_termination_status() && inner.is_terminated()) || self.is_terminated
    }

    fn supports_termination_status(&self) {
        true
    }
}

Could it make sense to think about this before FusedFuture gets released as the canonical solution? Or would the general strategy be to rely on this as a separate trait and wait for new Rust language features like specialization to fill the gaps?

@tmiasko
Copy link
Contributor

tmiasko commented Oct 2, 2019

I think it is misjudgement in FusedFuture trait definition that it allows for
is_terminated() to be true before future had returned Poll::Ready.

Allowing a future to terminate without producing a value crates a new distinct
way to terminate it. For reasons already described it is unrealistic to expect
that all wrappers will propagate such a termination status correctly. If a
future might not produce the value T, it should use Option<T> or a
Result<T> instead, rather than trying to express it through FusedFuture
API.

On the other hand if FusedFuture::is_terminated() were to indicate whether a
future returned Poll::Ready already, then using .fuse() on a future that
implements FusedFuture would preserve the meaning of termination.

Note that the case where a user fuses a future that already returned
Poll:Ready is clearly a misuse of the API (in similar fashion to fusing an
iterator that already returned None). In contrast currently it is impossible to
avoid using a .fuse() on a future that is terminated, since termination is
outside user control and might change in concurrent fashion e.g., future might
indicate termination when another thread closes the channel used by the future.

@taiki-e taiki-e modified the milestone: futures-0.4 Dec 17, 2020
@taiki-e taiki-e added the A-future Area: futures::future label Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-future Area: futures::future
Projects
None yet
Development

No branches or pull requests

3 participants