stream::Race never finishes

smol-rs / futures-lite

Futures, streams, and async I/O combinators.

Apache License 2.0

439 stars 25 forks source link

stream::Race never finishes #40

Open connec opened 3 years ago

connec commented 3 years ago

I'm trying to use stream::Race to join a couple of timer-based streams that periodically emit events before terminating. Sadly, the current implementation of stream::Race will never terminate as it will always return Poll::Pending if no sub-stream polls Ready(Some(_)): https://github.com/smol-rs/futures-lite/blob/0ba0f064cc2e07190b01d15930167b62d585fa14/src/stream.rs#L2312

I'm happy to make a PR if this would be considered a bug, but I figured I'd check first since it could break users who rely on its non-termination.

jbr commented 2 years ago

I just ran into this again — it should either be documented clearly that Race will never end even if the component streams do (both/either), or this should be fixed

taiki-e commented 2 years ago

I think the current implementation is wrong in 2 ways.

It should use fuse to avoid polling after the underlying stream returns None as it can cause panic.
If both underlying streams return None, then it should return None.

An example that implements this correctly is futures: https://github.com/rust-lang/futures-rs/blob/0.3.15/futures-util/src/stream/select.rs

So, I tend to fix the current behavior as a bug.

If you need the current behavior, you can use explicit .race(stream::pending()). (It also works with similar functions in other crates.)
Since the future is not polled after completion, the current implementation of future::race is fine.

jbr commented 2 years ago

The specific use case I had needed to terminate the stream when either of the streams terminated, not both. It seems like there are valid use cases and intuitive explanations for either "finish when both streams finish" and "finish when either stream finishes," depending on the type of content in the stream. Is there precedent for configuring this sort of thing on the Race stream? Or should there be a different name for those two different combinators?

fogti commented 1 year ago

so hm it might be a good idea to introduce FusedRace or such, which does have the described behavior (perhaps it could support both the "finish when both streams finish" and "finish when either stream finishes" via an enum argument)