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

FusedStream impl for ZipLatestWith missing a case #1

Open
extremeandy opened this issue Apr 4, 2023 · 1 comment
Open

FusedStream impl for ZipLatestWith missing a case #1

extremeandy opened this issue Apr 4, 2023 · 1 comment

Comments

@extremeandy
Copy link

In the main poll_next implementation of ZipLatestWith there are three conditions that could result in termination:

(StreamState::Nothing, _) if this.stream.is_done() => (
    Poll::Ready(None),
    StreamState::Nothing,
    StreamState::Nothing,
),
(_, StreamState::Nothing) if this.other_stream.is_done() => (
    Poll::Ready(None),
    StreamState::Nothing,
    StreamState::Nothing,
),
_ if this.stream.is_done() && this.other_stream.is_done() => (
    Poll::Ready(None),
    StreamState::Nothing,
    StreamState::Nothing,
),

However, the FusedStream::is_terminated implementation only covers the first two of these cases:

matches!(
    (&self.state, self.stream.is_done()),
    (StreamState::Nothing, true)
) || matches!(
    (&self.other_state, self.other_stream.is_done()),
    (StreamState::Nothing, true)
)

I propose that the is_terminated implementation needs to be changed to this (or something logically equivalent):

matches!(
    (&self.state, self.stream.is_done()),
    (StreamState::Nothing, true)
) || matches!(
    (&self.other_state, self.other_stream.is_done()),
    (StreamState::Nothing, true)
) || (self.stream.is_done() && self.other_stream.is_done())
@stephaneyfx
Copy link
Owner

Sorry for the delayed response.

The condition where both streams are done is already covered by the check in is_terminated. In other words, before calling is_terminated, the following holds:

self.stream.is_done() && self.other_stream.is_done() ⇒ matches!(stream.state, StreamState::Nothing) && matches!(stream.other_state, StreamState::Nothing)

Let's assume self.stream.is_done() && self.other_stream.is_done(). Fuse::is_done returns true only if polled at least once, implying ZipLatestWith was polled at least once. In the match expression to compute the new state, the first arm could not have been chosen because it implies a new item was just yielded, which implies is_done returns false for the corresponding stream. If the second or third arm was chosen, both states were set to Nothing. The fifth arm could not have been chosen because the fourth arm has a wildcard with a predicate that is our hypothesis, so it would have been chosen and would have set both states to Nothing. Thus, the hypothesis that both streams are done before calling is_terminated implies that both states are Nothing, which implies the existing implementation of is_terminated already handles that case.

Does that make sense?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants