Simpler selection futures #2112
Conversation
- 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.
| The remaining test failures seem unrelated to these changes |
| This looks good to me, though the dependence on #2111 worries me because I don't feel like I have a good handle on the best way to resolve that tension. Thanks for all your hard work here, though-- these combinators do look conveniently simpler. |
taiki-e left a comment
taiki-e left a comment There was a problem hiding this comment.
Sorry for the delay reviewing. I have a few suggestions, but this looks good overall.
| unsafe_pinned!(future1: F1); | ||
| unsafe_pinned!(future2: F2); |
There was a problem hiding this comment.
Please use pin_project instead of pin_utils::unsafe_pinned. (Since #2128, we have been using it.)
| pub struct FirstAll<F> { | ||
| // Critical safety invariant: after FirstAll is created, this vector can | ||
| // never be reallocated, in order to ensure that Pin is upheld. | ||
| futures: Vec<F>, |
There was a problem hiding this comment.
I recommend to use Pin<Box<[F]>> + this helper function, instead of Vec<F> + Pin::new_unchecked
| // Critical safety invariant: after FirstAll is created, this vector can | ||
| // never be reallocated, nor can its contents be moved, in order to ensure | ||
| // that Pin is upheld. | ||
| futures: Vec<F>, |
| | ||
| impl<T, E, F> Future for FirstOk<F> | ||
| where | ||
| F: Future<Output = Result<T, E>> + FusedFuture, |
There was a problem hiding this comment.
I recommend to use Fuse instead of relying on FusedFuture, as I said in #2111.
There was a problem hiding this comment.
I considered this, and if we remove FusedFuture entirely (like you suggested) then we can go this route, but I went with the trait here because many futures are "inherently" fused, so in the interest of zero-cost abstractions I thought it made more sense to just have the user call .fuse() only if they need to
There was a problem hiding this comment.
so in the interest of zero-cost abstractions I thought it made more sense to just have the user call
.fuse()only if they need to
Patterns like newtype are certainly not zero-cost, but Fuse just adds a flag to indicate if the future is complete and I think the actual cost is very small.
Another reason I don't want to add new features that rely on Fused* traits is that they are very likely to be removed in the next major version. (see #2207)
Even if we add features that rely on Fused* traits, they will be changed to use Fuse or MaybeDone. And if users forget to remove .fuse() on update to new version, it will eventually be even more inefficient.
taiki-e added the S-waiting-on-author 3 participants
This pull request adds
first,first_all, andfirst_ok. These are selection functions, similar to the existingselectand friends, which (unlike select) don't return the incomplete futures after finishing (simply discards them instead). This allows them to have implementations that are simpler, have less overhead, and do not requireUnpinon their contents.This pull request is in progress:
first!macroselect!macro.firstfirst_allfirst_ok,first_ok_fusedFusedIterator(requires Tighter contract for FusedFuture #2111)Fixes #2031 and #2110