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

Tighter contract for FusedFuture #2111

Closed
Lucretiel opened this issue Apr 2, 2020 · 5 comments
Closed

Tighter contract for FusedFuture #2111

Lucretiel opened this issue Apr 2, 2020 · 5 comments
Labels
A-future Area: futures::future

Comments

@Lucretiel
Copy link
Contributor

Lucretiel commented Apr 2, 2020

The current contract for FusedFuture is not strict enough to be useful in practice. I'm running into this issue while writing first_ok, as described in #2110/#2031. For reference, this is a future that polls a list of futures and returns the first Ok result, dropping the rest, or if none return Ok, the last Err result. It's similar to select_ok, except that it doesn't return the incomplete futures after running.

My original thought was that, to avoid the overhead of manually tracking which futures have completed, I could use FusedFuture, and allow my consumers to use fut.fuse() to fuse futures that don't already implement it. However, FusedFuture::is_terminated is allowed to return true even if it hasn't yet returned a ready.

I'd like to propose either strengthening the current contract or adding a new trait or new method so that is_terminated (or a separate, new method, perhaps is_completed) ONLY and ALWAYS returns true if the future has polled Ready. The existing functionality could be retained as a separate method, as it could be used for optimizing pollers. A contract change would be breaking, but would probably lead to less confusion long-term than introducing a new trait, because of existing associations with FusedIterator.

See #1894 for related discussion.

Lucretiel added a commit to Lucretiel/futures-rs that referenced this issue Apr 2, 2020
- Removed FusedIterator; .fuse() is equivalent.
- FirstOk is more explicit about its use of FusedIterator.
- This design will have to wait for rust-lang#2111 to be resolved; currently the
  contract of FusedFuture is not strict enough for our needs.
@cramertj
Copy link
Member

cramertj commented Apr 3, 2020

Hm, there are definitely things today that would return true from is_terminated even before returning Ready, such as future::pending.

@Lucretiel
Copy link
Contributor Author

Follow-up: I'd be open to adding a new trait for this; my concern is that FusedFuture has a tight association with FusedIterator, which strictly implies that the underlying asset is "done" (returned Ready(T) or returned None). My view is that the existing FusedFuture should be given a softer name, like FutureWillFinish.

All that being said, I understand the need to not break backwards compatibility without sufficient cause, so I'd be totally open to a new trait called something like DidComplete

@cramertj
Copy link
Member

Yeah, I understand that concern. I'm also pretty hesitant about propagating a second trait all throughout our codebase. cc @taiki-e @Nemo157 for their thoughts.

@taiki-e
Copy link
Member

taiki-e commented Jun 10, 2020

We can't propagate implementation properly until the (min) specialization is stable, so I think it's preferable to remove FusedFuture in the next major(minor) version. (i.e., always use .fuse())

@taiki-e
Copy link
Member

taiki-e commented Feb 22, 2021

Closing in favor of #2207, which propose the removal of FusedFuture. (See #2207 (comment) for a detailed description.)

@taiki-e taiki-e closed this as completed Feb 22, 2021
exrook pushed a commit to exrook/futures-rs that referenced this issue Apr 4, 2021
- Removed FusedIterator; .fuse() is equivalent.
- FirstOk is more explicit about its use of FusedIterator.
- This design will have to wait for rust-lang#2111 to be resolved; currently the
  contract of FusedFuture is not strict enough for our needs.
exrook pushed a commit to exrook/futures-rs that referenced this issue Apr 4, 2021
- Removed FusedIterator; .fuse() is equivalent.
- FirstOk is more explicit about its use of FusedIterator.
- This design will have to wait for rust-lang#2111 to be resolved; currently the
  contract of FusedFuture is not strict enough for our needs.
exrook pushed a commit to exrook/futures-rs that referenced this issue Apr 5, 2021
- Removed FusedIterator; .fuse() is equivalent.
- FirstOk is more explicit about its use of FusedIterator.
- This design will have to wait for rust-lang#2111 to be resolved; currently the
  contract of FusedFuture is not strict enough for our needs.
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