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 Stream wrappers in tokio-stream #3343

Merged
merged 10 commits into from Jan 4, 2021
Merged

Add Stream wrappers in tokio-stream #3343

merged 10 commits into from Jan 4, 2021

Conversation

Darksonn
Copy link
Contributor

@Darksonn Darksonn commented Dec 25, 2020

This PR builds on #3342. When that PR is merged, the diff should become more easily readable.

In this PR I add wrappers for every Tokio type that previously implemented Stream and has an exposed poll method. Notably, the poll method is missing on Signal, so that type is not included in this PR.

@Darksonn Darksonn added the A-tokio-stream Area: The tokio-stream crate label Dec 25, 2020
tokio-stream/src/wrappers/interval.rs Outdated Show resolved Hide resolved
tokio/src/fs/read_dir.rs Outdated Show resolved Hide resolved
Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

The implementation LGTM.

However, I'm not sure if it's better to put everything in a single module (wrappers) or in multiple modules per feature (net, io, fs). What do you think?

@Darksonn
Copy link
Contributor Author

I think the main place where we risk name overlaps is with receivers of various types, but those are all in sync.

@sunng87 sunng87 mentioned this pull request Dec 28, 2020
@ipetkov
Copy link
Member

ipetkov commented Dec 28, 2020

Notably, the poll method is missing on Signal, so that type is not included in this PR.

@Darksonn the Signal types (both for Unix and Windows) have poll_recv methods which have the exact signature of Stream::poll_next, so we should be able to delegate the Stream impl to them

@Darksonn
Copy link
Contributor Author

Yes, but I forgot to make them public, so we need a PR for that first.

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

LGTM

}
}

impl AsMut<Interval> for IntervalStream {
Copy link
Member

Choose a reason for hiding this comment

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

Just checking are there any issues exposing a mut reference while this stream is pinned?

Copy link
Contributor Author

@Darksonn Darksonn Jan 4, 2021

Choose a reason for hiding this comment

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

It's impossible to get an &mut S after having pinned S unless you use unsafe (or if S: Unpin of course), so you can't call this method if you have pinned it.

Though arguably we may want to add an as_pin_mut for non-Unpin wrappers.

Copy link
Member

Choose a reason for hiding this comment

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

good point, this should be fine for now then. I was thinking about the wrong thing, we are not doing any special unpinning here so we can expose the &mut T

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-stream Area: The tokio-stream crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants