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

Fuse, and therefore other adapters like Peekable, are incompatible with FuturesUnordered #2678

Open
sunshowers opened this issue Dec 27, 2022 · 1 comment
Labels
A-stream Area: futures::stream S-needs-implementation Status: Implementation work is needed.

Comments

@sunshowers
Copy link
Contributor

sunshowers commented Dec 27, 2022

Hi there! So this is a really fun issue I spent some time debugging. I had written an adapter that sits on top of a Peekable<FuturesUnordered<T>>. I had some code that did something like:

// self.stream is a Peekable<FuturesUnordered<T>>

match self.stream.poll_next(ctx) {
    // do some stuff here
}
self.stream.get_mut().push(new_future);
self.stream.poll_peek(ctx);

I found that if the inner FuturesUnordered dropped to having no futures from the first poll_next, then poll_peek always returns Poll::Ready(None) after that.

This is because Peekable uses the Fuse adapter internally (source code link).

I worked around it by writing my own version of Peekable that didn't use Fuse internally. Note that FuturesUnordered already implements FusedStream so I didn't have to change anything about Peekable.

Potential solutions

I discussed this issue with a couple of folks and here are some thoughts we had:

  1. Easiest, non-breaking change: add a reset_fuse method to the Fuse adapter, as well as any other adapters that use a Fuse internally. That would let me use get_mut() followed by reset_fuse() to inform the adapter that it should start polling for values again. (There would also be an advocacy component here -- if you write an adapter that uses one that has reset_fuse internally, you must also expose a reset_fuse method.)

  2. Remove the Fuse adapter from Peekable and all other adapters that use Fuse, and replace it with internal logic. This makes a lot of logical sense to me, but it is a breaking change because it won't be possible to maintain the FusedStream for Peekable<St> where St: Stream impl.

    However, it would still be possible to write impl FusedStream for Peekable<St> where St: FusedStream. Also, other people might build their own adapters that use Fuse anyway, so some other workaround like reset_fuse is necessary for ecosystem cohesion.

  3. Add a FusedStreamExt trait and a whole new series of methods peekable_fused etc that work on FusedStreams. This would solve the problem, since FuturesUnordered implements FusedStream, but this seems excessive.

  4. Revisit the definition of FusedStream as a breaking change. FusedStream really isn't like FusedIterator at all -- this was quite surprising to me and to other people I discussed the issue with. I'm not sure what the semantics would be but requiring workarounds like reset_fuse feels like a little bit of a code smell.

@taiki-e
Copy link
Member

taiki-e commented Dec 27, 2022

Peekable's use of Fuse is due to handling stream implementations that do not handle polls well after termination. (#72 (comment), see also docs) That said, it is possible to implement it without using Fuse, as was done with the standard library a few months after that Alex's comment (rust-lang/rust@6c2a456), and it would make sense to adopt that approach.

As for Fused{Future,Stream}, I plan to remove it in the next breaking release. (#2207 (comment))

@taiki-e taiki-e added the A-stream Area: futures::stream label Dec 27, 2022
@taiki-e taiki-e added this to the futures-0.4, futures-core-1.0 milestone Dec 27, 2022
@taiki-e taiki-e added the S-needs-implementation Status: Implementation work is needed. label Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stream Area: futures::stream S-needs-implementation Status: Implementation work is needed.
Projects
None yet
Development

No branches or pull requests

2 participants