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.

Sign up for GitHub

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

Jump to bottom

Ambiguous "pointers to data on the stack" #1677

Closed
proski opened this issue Jan 8, 2024 · 2 comments
Closed

Ambiguous "pointers to data on the stack" #1677

proski opened this issue Jan 8, 2024 · 2 comments
Assignees
@djmitche

Comments

@proski
Copy link
Contributor

proski commented Jan 8, 2024

https://google.github.io/comprehensive-rust/async/pitfalls/pin.html

I realize it's a complex topic, but would it be possible to use more clear language for the text that is already there?

When you await a future, all local variables (that would ordinarily be stored on a stack frame) are instead stored in the Future for the current async block.

Why is the second "Future" capitalized?

Is it correct that the first future that the second future are different?

If your future has pointers to data on the stack, those pointers might get invalidated.

This can be interpreted in two ways:

  • The future has pointers on the stack. Those pointers point to some data.
  • The future has pointers. Those pointers point to data on the stack.

How can the pointers get invalidated? Does the execution continue with the stack at a different address? Does some other code have write access to the future as it's awaiting?

This is unsafe.

Maybe it should be written as "This would be unsafe" as it not a real danger for safe Rust code.

Therefore, you must guarantee that the addresses your future points to don’t change.

This probably rules out the stack at a a different address, but I'm guessing here.

Who is that mischievous monkey that tries to move objects around?

That is why we need to “pin” futures.

Looking at the example, it's the future we call (timeout_fut) that needs to be pinned. Yet its address is in the worker() scope. So it's not the case of a future that has pointers and needs to be pinned. One future has pointers (our code) but we are pinning another future (timeout_fut).

Using the same future repeatedly in a select! often leads to issues with pinned values.

That's vague and scary. I think it means that the future cannot be replaced with another one and needs to be reset in place if possible. Thankfully, tokio::time::sleep can be reset. That could be mentioned in the notes below.
@djmitche djmitche self-assigned this Jan 10, 2024
@djmitche
Copy link
Collaborator

Thanks! These are good comments! I've got a bunch of issues like this to work through so this will go on the list. If you'd like to make a PR to fix this, that would be ❤️

@proski proski mentioned this issue Jan 12, 2024
Merged
mgeisler added a commit that referenced this issue Jan 16, 2024
@proski @mgeisler
Better explanation why futures need to be pinned (#1687)
0a1c30e 
Attempt to address #1677.

Expert review is needed. The new text is my best guess based on the
original text and other explanations I could find online.

A few things to note:

* I'm trying to distinguish the future we return and the future we
await. My assumption is that the stack contents goes to the future the
code returns, not the future the code is awaiting.
* Readers could be worried if they need to pin the code they write. I'm
reassuring them that the borrow checks would normally catch bad
references.
* I'm intentionally avoiding the words that something is unsafe (or
would be unsafe). The async Rust is safe.
* I'm trying to be clear that `Pin` is a protective wrapper around a
pointer, not a mechanism that changes the pointer or the pointed object.
* Likewise, I don't want to give an impression that an unpinned pointer
to a future is inherently unsafe or invalid. It just cannot be used to
poll the future.
* I dropped the vague mention of the "issues", as it probably refers to
the issue with replacing a future (as opposed to resetting it in place).
It's already mentioned in the notes further on this page. It affects
pinning on stack only, `Box::pin()` can be replaced.

Co-authored-by: Martin Geisler <[email protected]>
@djmitche
Copy link
Collaborator

Solved in #1687.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@djmitche djmitche

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@djmitche @proski