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 Future::poll_once #92116

Closed
wants to merge 1 commit into from
Closed

Conversation

ibraheemdev
Copy link
Member

@ibraheemdev ibraheemdev commented Dec 20, 2021

This PR adds Future::poll_once, which returns the output of polling a future once.

let f = ready(1);
assert_eq!(f.poll_once().await, Poll::Ready(1));

let mut f = pending();
assert_eq!(f.poll_once().await, Poll::Pending);

I'm not sure what a good name for this would be (try_await?). It isn't polling once any more than Future::poll is. Maybe keeping it as a macro/free-standing function is the best way to resolve the ambiguity?

Prior Art: https://docs.rs/futures/latest/futures/macro.poll.html

(Can someone on the team cc/ @rust-lang/wg-async-foundations?)

@rust-highfive
Copy link
Collaborator

r? @scottmcm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 20, 2021
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
Building stage0 std artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.18s
Copying stage0 std from stage0 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
 Documenting core v0.0.0 (/checkout/library/core)
error: unresolved link to `poll_once`
  |
  |
8 | /// This `struct` is created by [`poll_once()`]. See its
  |                                   ^^^^^^^^^^^ no item named `poll_once` in scope
  |
  = note: `-D rustdoc::broken-intra-doc-links` implied by `-D warnings`
  = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
error: could not document `core`

Caused by:
Caused by:
  process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustdoc --edition=2018 --crate-type lib --crate-name core library/core/src/lib.rs --target x86_64-unknown-linux-gnu -o /checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/doc --error-format=json --json=diagnostic-rendered-ansi --markdown-css rust.css --markdown-no-toc -Z unstable-options --resource-suffix 1.59.0 --index-page /checkout/src/doc/index.md -C metadata=4599449636879869 -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/release/deps --cfg=bootstrap -Zsymbol-mangling-version=legacy -Dwarnings '-Wrustdoc::invalid_codeblock_attributes' --crate-version '1.59.0-nightly
  (0faca5355
  2021-12-20)' '-Zcrate-attr=doc(html_root_url="https://doc.rust-lang.org/nightly/")'` (exit status: 1)


command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "rustdoc" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "-p" "core" "-Zskip-rustdoc-fingerprint" "--" "--markdown-css" "rust.css" "--markdown-no-toc" "-Z" "unstable-options" "--resource-suffix" "1.59.0" "--index-page" "/checkout/src/doc/index.md"


Build completed unsuccessfully in 0:00:07

@taiki-e
Copy link
Member

taiki-e commented Dec 20, 2021

If you use this function without considering the cancellation safety of the underlying future, it may cause various bugs (rust-lang/futures-rs#2452 (review)). So, I do not agree with adding this without caveats.

@taiki-e
Copy link
Member

taiki-e commented Dec 20, 2021

cc @rust-lang/wg-async-foundations

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Dec 20, 2021

I don't mind this feature too much, but it is worth noting the applications of this are definitely limited. We never added this functionality to async-std, and I don't recall anyone requesting we add this either. In particular the F: Unpin bound limits what this can be used for in practice.

I agree with @taiki-e that the "drop in-place" nature of poll_once has the potential to lead to bugs: state is often contained within a future, and destructors are run on drop. This indeed means that using poll_once by someone who doesn't understand Rust's futures poll model can lead to logic bugs, and needs to be warned against.

Overall it seems poll_once would be most useful for showing how futures internals work (example: stdlib docs), or (in limited cases) testing futures manually authored futures state machines. Whether this is enough to carry this feature I'm unsure about.

@nrc nrc added A-async-await Area: Async & Await T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 20, 2021
@nrc
Copy link
Member

nrc commented Dec 20, 2021

I agree with both concerns (level of motivation and drop/cancellation safety). I think at the least, the docs need expanding to properly explain what this function does, why it is different to poll (which as you say, also only polls once), and what can go wrong with cancellation. I think it needs better motivation here before we add it to std, in particular, why this has to be in std rather than in a crate, given that it seems both dangerous and somewhat niche in its usage.

@scottmcm scottmcm added I-libs-api-nominated Nominated for discussion during a libs-api team meeting. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Dec 20, 2021
@scottmcm
Copy link
Member

Marking this for team discussion, since I don't have enough context to know whether to take this.

@dtolnay dtolnay added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 20, 2021
@joshtriplett
Copy link
Member

I'd love to see a specific use case for this as well.

@cramertj
Copy link
Member

I don't think we should begin piecemeal importing combinators like this into std. I think there should be a design RFC first on if and how to transition combinator methods from futures-rs and tokio into std. Until then, functions like this can exist in those external crates.

@m-ou-se m-ou-se removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Dec 22, 2021
@joshtriplett joshtriplett added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Dec 22, 2021
@JohnCSimon
Copy link
Member

Ping from triage:
@ibraheemdev can you post your status on this PR and address the comments from the reviewers?

@taiki-e
Copy link
Member

taiki-e commented Jan 23, 2022

@yoshuawuyts This and #93176 are different features, not like a superset.

the way this works is instead of calling poll_once which consumes self, you can instead pin! the future on the stack, and then call poll repeatedly as desired.

Note that you need to have a feature to get the context in the async function/block to do the same in this way.

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Jan 24, 2022

@yoshuawuyts This and #93176 are different features, not like a superset.

I see. I've gone and deleted my comment further confusion. Apologies for the noise.

@nikomatsakis
Copy link
Contributor

I don't know that this should be in std, but I did encounter a case where I wanted it. Specifically, I have some functionality that sometimes blocks and sometimes doesn't, depending on its input. That function needs to be async, but in the cases where it doesn't block, it gets called from non-async functions. I manually invoke poll and then assert that it returns Ready in these cases to bridge the gap. It required me to pin which is fine but mildly annoying.

/// documentation for more.
#[unstable(feature = "future_poll_once", issue = "92115")]
pub struct PollOnce<F> {
pub(crate) future: F,
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla Jan 31, 2022

Choose a reason for hiding this comment

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

Idea to support ?Unpin:

Suggested change
pub(crate) future: F,
pub(crate) future: Option<F>,

(and a impl<F> Unpin for PollOnce<F>)

Comment on lines +25 to +31
F: Future + Unpin,
{
type Output = Poll<F::Output>;

fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
Poll::Ready(Pin::new(&mut self.future).poll(cx))
}
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla Jan 31, 2022

Choose a reason for hiding this comment

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

Suggested change
F: Future + Unpin,
{
type Output = Poll<F::Output>;
fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
Poll::Ready(Pin::new(&mut self.future).poll(cx))
}
F: Future,
{
type Output = Poll<F::Output>;
fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
let future = pin!(self.future.take().expect("Polled after completion"));
Poll::Ready(future.poll(cx))
}

If the dependency on a macro is deemed unsatisfactory to begin with, we could always hand-roll a Pin::new_unchecked(). This is also a nice instance to showcase the safe .pinned() (🚲 🏠) closure-based combinator:

self.future
    .take()
    .expect("Polled after completion")
    .pinned(|future| Poll::Ready(future.poll(cx)))

where
Self: Sized,
{
PollOnce { future: self }
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla Jan 31, 2022

Choose a reason for hiding this comment

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

Suggested change
PollOnce { future: self }
PollOnce { future: Some(self) }

@JohnCSimon
Copy link
Member

Ping from triage:
@ibraheemdev Can you please address the comments from the reviewers?

@ibraheemdev
Copy link
Member Author

I'd love to see a specific use case for this as well.

One that recently came up was using async in the context of a GUI/TUI event loop. One might want to periodically check a resource's status without blocking (yielding).

@yoshuawuyts
Copy link
Member

One might want to periodically check a resource's status without blocking (yielding).

But poll_once consumes self? I'm not sure how that is compatible with repeatedly checking a resource? If you could, providing a concrete example for how this works would be helpful.

@Darksonn
Copy link
Contributor

I mean, since Pin<&mut F> implements future, you could do pinned_fut.as_mut().poll_once() to do it repeatedly.

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Feb 28, 2022

Oh I see. It's starting to make more sense to me why this might sometimes be useful. I think it'd be helpful if we could include examples of this usage as part of the docs examples.

Also after some github searching I found that there is also prior art for poll_once in futures_lite::future::poll_once. Along with futures that then makes two libraries which provide APIs for this. Some places which seem to be using some variant of poll_once are:

@scottmcm scottmcm assigned joshtriplett and unassigned scottmcm Mar 20, 2022
@JohnCSimon
Copy link
Member

@ibraheemdev
Ping from triage: Can you please post the status of this PR? This has broken tests.
Thank you.

@ibraheemdev
Copy link
Member Author

Waiting for a decision on whether this should be added before putting in more work @JohnCSimon

@JohnCSimon JohnCSimon added the I-needs-decision Issue: In need of a decision. label Apr 11, 2022
@joshtriplett
Copy link
Member

joshtriplett commented Apr 27, 2022

We discussed this in today's @rust-lang/libs-api meeting. We didn't have an objection to adding this, since it does have uses (per #92116 (comment) ). But we'd like confirmation that this is the correct API; should it be non-consuming by default, rather than taking self?

Also, is this the right name?

cc @yoshuawuyts

@joshtriplett joshtriplett removed the I-needs-decision Issue: In need of a decision. label Apr 27, 2022
@yoshuawuyts
Copy link
Member

yoshuawuyts commented Apr 27, 2022

But we'd like confirmation that this is the correct API; should it be non-consuming by default, rather than taking self?

I think the current API is correct as written. If the method did not consume self, I believe the self-type should be pinned (e.g. self: Pin<&mut Self>) so that we can guarantee the future won't move between calls - consuming the value is one way to guarantee this 1.

It's interesting to think of what this might look like. Now that we seem to have a promising way to stack-pin types (std::pin::pin!) it may not be too bad to require it. This is what the use case outlined in #92116 (comment) would look like under both models:

#![feature(pin_macro)]
use std::task::Poll;
use std::pin::Pin;

// current proposed API:
let pinned_fut = pin!(future::ready(()));
assert_eq!(pinned_fut.as_mut().poll_once(), Poll::Ready(()));

// using a pinned `Self`-type in the signature:
let pinned_fut = pin!(future::ready(()));
assert_eq!(pinned_fut.poll_once(), Poll::Ready(())); // note the omission of `as_mut` here.

Going even further though: I think the main innovation of poll_once is that it surfaces the underlying std::task::Context type from within an async fn. With that in mind, if we had a function which would just return that, we wouldn't need to use poll_once and could instead call Future::poll with the passed context:

#![feature(pin_macro)]
use std::task::{self, Poll};
use std::pin::Pin;

// using a function to get the underlying `Context` object:
let pinned_fut = pin!(future::ready(()));
let cx = task::context().await; // this method is made up, it just returns the underlying `Context`
assert_eq!(pinned_fut.poll(cx), Poll::Ready(()));

The main benefit I see with this approach is that it would compose well with other poll_ methods such as AsyncIterator::poll_next. But whether this is the right API to pursue instead of poll_once I don't feel capable of accurately judging at the moment.

Footnotes

  1. I'm half-remembering things here. I think this is right, but I've been out sick for a month now, which means I'm less confident than I might usually be. If folks reading along could double check this is in fact correct, I'd appreciate that.

@Darksonn
Copy link
Contributor

I agree that it should consume self. When you consume self, you support both the use-cases of consuming it and not consuming it (by calling it on a &mut F or Pin<&mut F>), whereas if you don't consume self, the use-case where you want to call it on an !Unpin future requires pinning it even if you're just consuming it.

@ibraheemdev
Copy link
Member Author

Also, is this the right name?

I don't really think so because of the ambiguity between this and the synchronous now_or_never which uses a noop waker. As @yoshuawuyts pointed out, there's been discussion about a task::context! intrinsic (can't be a method because of the lifetime) that exposes the Context of the async fn's future. That could make things more consistent and eliminate the need for both methods:

let f = pin!(f);
f.poll(task::context!()); // poll_once
f.poll(task::noop_context()); // now_or_never

@c410-f3r
Copy link
Contributor

What is the status of this feature?

@JohnCSimon
Copy link
Member

What is the status of this feature?

@ibraheemdev - can you comment on this?

@JohnCSimon
Copy link
Member

@ibraheemdev
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Jul 24, 2022
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jul 24, 2022
@c410-f3r
Copy link
Contributor

If it is OK to the OP, I will re-create this PR.

@ibraheemdev
Copy link
Member Author

@c410-f3r see #92116 (comment), I think this is a better way forward.

@c410-f3r
Copy link
Contributor

@c410-f3r see #92116 (comment), I think this is a better way forward.

I was hoping to experiment this feature as an user (not as a Rustc developer) but since the API requires more discussion (which is a bit strange for nightly), then I give up due to the lack of personal energy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.