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

Explain pinning in README #26

Closed
wants to merge 1 commit into from
Closed

Conversation

kornelski
Copy link

Fixes #25

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.

Left a few suggestions, let me know what you think :)

pin_mut!(s); // needed for iteration
// Stream must be pinned before iteration. It can be done with Box::pin,
// but the pin_mut macro is more efficient. See "Pinning" section.
pin_mut!(s);
Copy link
Member

Choose a reason for hiding this comment

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

minor nit, could we possibly change this to https://docs.rs/tokio/0.2.11/tokio/macro.pin.html happy to do this in another PR but would be good to point to the tokio version.

@@ -32,7 +32,9 @@ async fn main() {
}
};

pin_mut!(s); // needed for iteration
// Stream must be pinned before iteration. It can be done with Box::pin,
Copy link
Member

Choose a reason for hiding this comment

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

I would probably say something along the lines of stack pinning via tokio::pin is much more efficient but this can also be achieved by hoisting the item via Box::pin.

@@ -60,7 +62,7 @@ fn zero_to_three() -> impl Stream<Item = u32> {
#[tokio::main]
async fn main() {
let s = zero_to_three();
pin_mut!(s); // needed for iteration
pin_mut!(s); // Pinning is needed for iteration
Copy link
Member

Choose a reason for hiding this comment

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

Since, impl Stream above doesn't retrun a impl Stream + Unpin we must ensure that the stream is Unpin to await something on it.

Might be good to explain that.

@@ -135,6 +137,20 @@ fn bind_and_accept(addr: SocketAddr)
}
```

## Pinning

If you wrap the stream in `Box::pin`, the callers won't need to pin it before iterating it. This is more convenient for the callers, but adds a cost of using dynamic dispatch.
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to explain that Unpin gets auto added in impl Trait just like how Send gets leaked since that could be confusing.

@carllerche
Copy link
Member

Should we just get this merged?

@kornelski kornelski closed this Sep 12, 2022
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

Successfully merging this pull request may close these issues.

Box::pin vs pin_mut!(s);
3 participants