[doc] poll_fn: explain how to pin captured state safely

[danielhenrymantilla]

Usage of Pin::new_unchecked(&mut …) is dangerous with poll_fn, even though the !Unpin-infectiousness has made things smoother. Nonetheless, there are easy ways to avoid the need for any unsafe altogether, be it through Box::pinning, or the pin! macro. Since the latter only works within an async context, showing an example artificially introducing one ought to help people navigate this subtlety with safety and confidence.

Preview

Screen.Recording.2023-04-05.at.15.18.03.mov

@rustbot label +A-docs

[rustbot]

r? @joshtriplett

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[danielhenrymantilla]

r? libs

[cuviper]

I don't feel comfortable reviewing pin-related things...

r? libs

[taiki-e]

This is certainly necessary for this function to work as yield_once_then, but IIUC the explanation about yield_once_then is unrelated to the purpose of this section and distracting.

Is it possible to make the example simpler? Personally, I think an example that simply polls the future in a closure would be sufficient.

[danielhenrymantilla]

Regarding "distraction", I have now moved the detailed comment outside the code snippet, also deduplicating it.

We now have:

poll_fn(move |cx| if mem::take(&mut should_yield) {
    cx.waker().wake_by_ref();
    Poll::Pending
} else {
    pinned_fut.as_mut().poll(cx)
})

which is quite lightweight 🙂

Regarding simplicity, however, I disagree that this example should be further simplified, because at that point there is not that much incentive to use poll_fn to begin with, nor resemblance to actual async code. Documentation of practical functions such as this one are the best situation to educate people about not only the usefulness/handiness of poll_fn, but also of certain pitfalls when manually implementing poll, such as the Pending-without-wake mistake.

[kpreid]

(Drive-by nitpicking; don't take this too seriously.)

Arguably, this example is already too simple — because there is an enclosing async block, and should_yield never becomes true after it is false, it is possible to rewrite

let mut pinned_fut = pin!(fut);
poll_fn(move |cx| if mem::take(&mut should_yield) {
    cx.waker().wake_by_ref();
    Poll::Pending
} else {
    pinned_fut.as_mut().poll(cx)
}).await

as

poll_fn(move |cx| if mem::take(&mut should_yield) {
    cx.waker().wake_by_ref();
    Poll::Pending
} else {
    Poll::Ready(())
}).await;
fut.await    // no need to pin!()

and this is (in my opinion) better-factored code, showing how to take maximum advantage of async {} rather than writing any more manual-polling code than you have to. I could worry that people learning async internals will read this code and wonder why not to write the latter.

My first idea for an example that actually needs the pin! and poll would be one which yields after a time interval has elapsed (adding yield points for better cooperative multitasking), but that's got perhaps a bit too much complexity.

[danielhenrymantilla]

Yeah, that's a fair point; what about a naïve select, then?

That could be simple whilst legitimately polling the inner futures:

  /// Resolves to the first future that completes. In the event of a tie, `a` wins.
  fn naive_select<T>(
      a: impl Future<Output = T>,
      b: impl Future<Output = T>,
  ) -> impl Future<Output = T>
  {
+     async {
          let (mut a, mut b) =
-             (Box::pin(a), Box::pin(b))
+             (pin!(a), pin!(b))
          ;
          future::poll_fn(move |cx| {
              match (a.as_mut().poll(cx), b.as_mut().poll(cx)) {
                  (Poll::Ready(r), _) | (_, Poll::Ready(r)) => Poll::ready(r),
                  _ => Poll::Pending,
              }
          })
+         .await
+     }
  }

[kpreid]

I like that example much better.

One further nitpick, though: it doesn't just let a win, it also polls b and discards the result if any, which good selects don't do and which will introduce cancellation bugs (e.g. if you are selecting among two message channels, this will drop some messages from the second). It's easy to fix at the cost of 3 more lines (but might be more readable anyway):

if let Poll::Ready(r) = a.as_mut().poll(cx) {
    Poll::Ready(r)
} else if let Poll::Ready(r) = b.as_mut().poll(cx) {
    Poll::Ready(r)
} else {
    Poll::Pending
}

(Thanks for taking my input. I'm bothering to write all this because in my experience, beginners will read examples and assume that everything in them is good practice even when it's not about the thing the example is for.)

[danielhenrymantilla]

Started implementing this as of 9541287.

FWIW, this thus puts and end to the wake() noise/documentation question, since it's no longer relevant.

• (I feel like a companion PR just implementing yield_once using poll_fn could be a nice thing to add nonetheless (but without pin! nor the _then part, to keep it simple), since I do still believe that the wake specificites ought to have examples. But I won't push for it myself, nor mention it anymore)

[thomcc]

This was tough to review, but ultimately looks fine to me. Thanks a great deal to @kpreid and @taiki-e for their help with this review.

@bors r+ rollup

[bors]

📌 Commit 9541287e315d15cb851e58f598b39fc3cf597723 has been approved by thomcc

It is now in the queue for this repository.

[danielhenrymantilla]

@thomcc now that you have reviewed all the discussion and so forth (thanks!), I'd like to squash the commits to keep the history (c)lean (since, according to rust-lang/bors#25, it is not possible to instruct bors to do it).

EDIT: Done (for those curious, old history)

[thomcc]

@bors r+

[bors]

📌 Commit 94f7a79 has been approved by thomcc

It is now in the queue for this repository.