Skip to content

Proposal: add stream::OptionStream #2737

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

Open
dodomorandi opened this issue Apr 4, 2023 · 2 comments
Open

Proposal: add stream::OptionStream #2737

dodomorandi opened this issue Apr 4, 2023 · 2 comments
Labels
A-stream Area: futures::stream C-feature-request S-needs-api-design Status: Before implementing this, a discussion or decision on the new API is needed.

Comments

@dodomorandi
Copy link

dodomorandi commented Apr 4, 2023

I would like to propose the addition of a new abstraction: OptionStream. I would expect it to have the same Item as the underlying stream when it's built from an Option::Some variant, and to always return Poll::Pending Poll::Ready(None) (see my comment below) when it's built from Option::None.

The abstraction is useful when you need to enable/disable a particular stream (i.e. an IntervalStream) while keeping the same stream type.

Here Here you can find a simple implementation, in case it could make things more clear.

@taiki-e
Copy link
Member

taiki-e commented Apr 4, 2023

Except that the behavior of the None case is different from the existing OptionFuture (and rust-lang/libs-team#197), I think this is reasonable.

@taiki-e taiki-e added C-feature-request A-stream Area: futures::stream labels Apr 4, 2023
@dodomorandi
Copy link
Author

Ok, I think I got it wrong the first time I thought about this, now I am pretty convinced that the stream should return a Poll::Ready(None) when it's internal variant is None, not Poll::Pending. But let me explain my reasoning.

In theory, the None variant of OptionFuture should compose like the future is not there, in reasonably ways. Let's take in fact what one would expect from the operations join and select (or race, if you consider the concepts behind futures-concurrency):

  • joining a future and an None variant of an OptionFuture is expected to resolve as soon as the first future is Ready. The fact that a None OptionFuture returns a Poll::Ready(None) exactly matches this ideal model.
  • selecting the same two futures is less intuitive, because the None variant of the OptionFuture will always be handled immediately. According to the logic of "should behave like the OptionNone does not exist", the current approach does not exactly match the intuition.

Let's see what happens instead when using the same approach for the stream, using the merge, the chain and the zip operations from futures-concurrency:

  • merging a stream and a None variant of an OptionStream should just produce the first stream. This behavior is obtained when a stream immediately return Poll::Ready(None) and when it returns Poll::Pending as well. However, the main difference is that in the latter case the merged stream will never end, which would be unexpected.
  • chaining the two streams produces a similar effects: the elements belongs to the first stream, but if Poll::Pending is returned by the None variant, the chain will never end. Therefore returning Poll::Ready(None) is the more reasonable way in this case.
  • zipping the two streams is a little bit controversial. If Poll::Pending is returned with the None variant, the zip operation will wait forever. on the other hand if Poll::Ready(None) is returned no element is obtained by the zipped stream. I tend to believe the latter behavior is more expected than the first one, but in the end it looks like that performing a zip operation with an OptionStream that is None can be a bit meaningless.

After this small analysis, it looks like the None variant of the OptionStream should return a Poll::Ready(None) (and not a Poll::Pending as I initially proposed). Let me know what you think 😊.

@taiki-e taiki-e added the S-needs-api-design Status: Before implementing this, a discussion or decision on the new API is needed. label Jul 20, 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 C-feature-request S-needs-api-design Status: Before implementing this, a discussion or decision on the new API is needed.
Projects
None yet
Development

No branches or pull requests

2 participants