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

Add a Stream::try_iter() method #70

Merged
merged 5 commits into from
Feb 18, 2024
Merged

Add a Stream::try_iter() method #70

merged 5 commits into from
Feb 18, 2024

Conversation

notgull
Copy link
Member

@notgull notgull commented Apr 28, 2023

This PR adds a try_iter method to StreamExt that consumes all of the values until it gets Pending, at which point it returns None. The goal is to get a method similar to Receiver::try_iter() on the async_channel::Receiver type that would allow users to consume all of the values in the channel without waiting.

@taiki-e
Copy link
Collaborator

taiki-e commented Apr 28, 2023

I'm confused about this is named _iter. Implementation has no relationship to the Iterator trait.

@notgull
Copy link
Member Author

notgull commented Apr 28, 2023

I basically just took the name from libstd's try_iter. Maybe a better name for this would be s.ready() or s.immediate()?

@taiki-e
Copy link
Collaborator

taiki-e commented Apr 28, 2023

poll_immediate and now_or_never in futures are somewhat similar to this, but I don't think they are exactly the same.

Well, this doesn't even require implementing Stream in the first place. Implementing Iterator might actually work in more contexts.

@notgull
Copy link
Member Author

notgull commented Apr 28, 2023

Well, this doesn't even require implementing Stream in the first place. Implementing Iterator might actually work in more contexts.

Unfortunately we still need a Context to pass down to the underlying Stream, just like for future::poll_once(). We could add functionality to create a "no-op waker" for this if we wanted to.

@taiki-e
Copy link
Collaborator

taiki-e commented Apr 28, 2023

Unfortunately we still need a Context to pass down to the underlying Stream, just like for future::poll_once().

Oh, good point. I forgot that because now_or_never uses noop_context.

Comment on lines +3191 to +3305
match self.stream.poll_next(cx) {
Poll::Ready(x) => Poll::Ready(x),
Poll::Pending => Poll::Ready(None),
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
match self.stream.poll_next(cx) {
Poll::Ready(x) => Poll::Ready(x),
Poll::Pending => Poll::Ready(None),
}
Poll::Ready(match self.stream.poll_next(cx) {
Poll::Ready(x) => x,
Poll::Pending => None,
})

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this looks nice

@fogti
Copy link
Member

fogti commented Aug 17, 2023

Usage of this would probably introduce unnecessary spurious wake-ups due to the lack of smth. like noop_context.

@notgull
Copy link
Member Author

notgull commented Aug 17, 2023

Yes, it would be nice to have a noop_waker or a panic_waker for futures that are guaranteed to not return Pending. Unfortunately defining that using only safe code requires an unnecessary allocation.

@fogti
Copy link
Member

fogti commented Aug 17, 2023

Could the allocation be cached (similar to the caching in block_on)?

@notgull
Copy link
Member Author

notgull commented Aug 17, 2023

It could, but you would still need an atomic operation to clone it out of the thread local. While this is acceptable overhead for block_on, I imagine that try_iter() would be called much more frequently.

I don't think there's anything wrong with just passing try_iter() to stream::block_on, just like poll_once() is passed to future::block_on or spin_on.

1 similar comment
@notgull

This comment was marked as duplicate.

@notgull notgull requested a review from fogti February 18, 2024 01:19
@fogti
Copy link
Member

fogti commented Feb 18, 2024

I think the concern about the name still applies here. I'd propose try_stream in analogue to try_iter, but idk if that's already in use...

src/stream.rs Outdated
@@ -1759,6 +1759,54 @@ pub trait StreamExt: Stream {
}
}

/// Runs this stream until it returns `Poll::Pending`, whereupon it yields `None`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stream isn't necessarily fused, right? I don't think "until" does capture what happens correctly... It might also be a good idea to provide a way to access the inner stream, in case one wants that....

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might also be a good idea to provide a way to access the inner stream, in case one wants that....

I've added accessor methods. However there are many other streams in this crate that don't provide access to their underlying streams. I'm not sure if it's a good idea to add those accessor methods there as well...

Definitely out of scope for this PR though.

@fogti
Copy link
Member

fogti commented Feb 18, 2024

Also, what about pinning? How does this interact with that?

notgull and others added 4 commits February 18, 2024 12:36
- Rename try_iter to try_stream.
- Mention non-fuse status in docs.
- Add accessor methods for inner stream.

Signed-off-by: John Nunley <dev@notgull.net>
@notgull
Copy link
Member Author

notgull commented Feb 18, 2024

Also, what about pinning? How does this interact with that?

It only takes Unpin streams. However it's possible to make it take a !Unpin stream by first pinning the stream to the stack.

let x = stream::once_future(async { 1 });
pin!(x);
let y = x.try_stream();

@fogti
Copy link
Member

fogti commented Feb 18, 2024

Error: cannot find function block_on in module futures_lite::future

Signed-off-by: John Nunley <dev@notgull.net>
@fogti fogti merged commit d4230bd into master Feb 18, 2024
7 checks passed
@fogti fogti deleted the notgull/try-iter2 branch February 18, 2024 20:54
@notgull notgull mentioned this pull request Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants