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

Update the future/task API #57992

Merged
merged 10 commits into from
Feb 14, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/liballoc/boxed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ use core::ops::{
CoerceUnsized, DispatchFromDyn, Deref, DerefMut, Receiver, Generator, GeneratorState
};
use core::ptr::{self, NonNull, Unique};
use core::task::{LocalWaker, Poll};
use core::task::{Waker, Poll};

use crate::vec::Vec;
use crate::raw_vec::RawVec;
Expand Down Expand Up @@ -896,7 +896,7 @@ impl<G: ?Sized + Generator> Generator for Pin<Box<G>> {
impl<F: ?Sized + Future + Unpin> Future for Box<F> {
type Output = F::Output;

fn poll(mut self: Pin<&mut Self>, lw: &LocalWaker) -> Poll<Self::Output> {
F::poll(Pin::new(&mut *self), lw)
fn poll(mut self: Pin<&mut Self>, waker: &Waker) -> Poll<Self::Output> {
F::poll(Pin::new(&mut *self), waker)
}
}
4 changes: 0 additions & 4 deletions src/liballoc/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,6 @@ mod macros;

pub mod alloc;

#[unstable(feature = "futures_api",
reason = "futures in libcore are unstable",
issue = "50547")]
pub mod task;
// Primitive types using the heaps above

// Need to conditionally define the mod from `boxed.rs` to avoid
Expand Down
130 changes: 0 additions & 130 deletions src/liballoc/task.rs

This file was deleted.

48 changes: 20 additions & 28 deletions src/libcore/future/future.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
use marker::Unpin;
use ops;
use pin::Pin;
use task::{Poll, LocalWaker};
use task::{Poll, Waker};

/// A future represents an asynchronous computation.
///
Expand All @@ -19,13 +19,14 @@ use task::{Poll, LocalWaker};
/// final value. This method does not block if the value is not ready. Instead,
/// the current task is scheduled to be woken up when it's possible to make
/// further progress by `poll`ing again. The wake up is performed using
/// `cx.waker()`, a handle for waking up the current task.
/// the `waker` argument of the `poll()` method, which is a handle for waking
/// up the current task.
///
/// When using a future, you generally won't call `poll` directly, but instead
/// `await!` the value.
#[must_use = "futures do nothing unless polled"]
pub trait Future {
/// The result of the `Future`.
/// The type of value produced on completion.
type Output;

/// Attempt to resolve the future to a final value, registering
Expand All @@ -42,16 +43,16 @@ pub trait Future {
/// Once a future has finished, clients should not `poll` it again.
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed in https://github.com/rust-lang/rfcs/pull/2592/files#r232498609, clarify that it isn't guaranteed that clients won't poll again, that it isn't guaranteed that panics will occur if they do, and that this assumption shouldn't be used for memory safety.

Copy link
Member

@Nemo157 Nemo157 Jan 30, 2019

Choose a reason for hiding this comment

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

This has identical behaviour/requirements to Iterator::next, which has even less docs around behaviour after completion. Should this clarity be required there as well?

(EDIT: lol, looking back I was the one saying this should have more clarity. I guess I'm on the side that the iterator docs should probably be improved as well.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Returns None when iteration is finished. Individual iterator implementations may choose to resume iteration, and so calling next() again may or may not eventually start returning Some(Item) again at some point.

The difference here is that Iterator::next doesn't imply in any way (in fact, the opposite) that None + None isn't a valid sequence. Meanwhile, "should not poll it again" is emphatic about what should not happen, so a reader may assume this is always true.

Copy link
Contributor

Choose a reason for hiding this comment

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

A non-intrusive change to the language here would just be to weaken "should not poll again" to something like "is recommended to not poll again".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm torn on this one. In the section that @Centril commented about lower, I think no guarantees are made at all for clients polling again after Ready was returned. Incl. memory safety guarantees.
As far as I remember some future implementations might have exploited this for performance reasons.

We can obviously change and enforce this, but I wouldn't like to change the semantics inside this CR.

Copy link
Member

Choose a reason for hiding this comment

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

It must be memory safe since this function isn't unsafe. Other than that, there are no guarantees, most futures will panic, but some may just implicitly reset and run again, or get stuck returning Poll::Pending without scheduling a wakeup, or anything else that doesn't break memory safety. So, an executor or wrapping future calling poll again after Ready is returned is a very bad logic bug that has a high chance of causing subsequent errors elsewhere in the system (so panicking is the best option to minimise those subsequent errors), but can't be allowed to compromise memory safety.

Copy link
Member

Choose a reason for hiding this comment

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

I think part of the difference from Iterator is what implementations are likely to do. With Iterator you have an easy option of just returning None again, so while polling it again is a bug you have less chance of bringing your system down. With Future you can't easily return Ready again (unless the value is trivial), so it's mostly guaranteed that polling again will either panic or do something unexpected which will cause later issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the comment about memory safety to the other section (which refers to polling after Ready).

///
/// When a future is not ready yet, `poll` returns `Poll::Pending` and
/// stores a clone of the [`LocalWaker`] to be woken once the future can
/// stores a clone of the [`Waker`] to be woken once the future can
/// make progress. For example, a future waiting for a socket to become
/// readable would call `.clone()` on the [`LocalWaker`] and store it.
/// readable would call `.clone()` on the [`Waker`] and store it.
/// When a signal arrives elsewhere indicating that the socket is readable,
/// `[LocalWaker::wake]` is called and the socket future's task is awoken.
/// `[Waker::wake]` is called and the socket future's task is awoken.
/// Once a task has been woken up, it should attempt to `poll` the future
/// again, which may or may not produce a final value.
///
/// Note that on multiple calls to `poll`, only the most recent
/// [`LocalWaker`] passed to `poll` should be scheduled to receive a
/// [`Waker`] passed to `poll` should be scheduled to receive a
/// wakeup.
///
/// # Runtime characteristics
Copy link
Contributor

Choose a reason for hiding this comment

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

Below it reads:

An implementation of poll should strive to return quickly, and must never block.

Per https://github.com/rust-lang/rfcs/pull/2592/files#r250460257, clarify that callers of poll for an arbitrary F: Future may not assume this to be true for memory safety purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that information is provided implicitly. I think this together with the next sentences clarifies to the reader that this is purely about performance. I'm having a hard time to interpret anything about memory safety into it. We don't document for other methods either that they need to be memory safe, even if they are slow.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would again make the language weaker here and say "is recommended to never block" instead of framing it in a way that suggests a guarantee. It is true that the function is not unsafe, but reinforcing the non-guarantee seems helpfully non-misleading to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Expand All @@ -67,44 +68,35 @@ pub trait Future {
/// typically do *not* suffer the same problems of "all wakeups must poll
/// all events"; they are more like `epoll(4)`.
///
/// An implementation of `poll` should strive to return quickly, and must
/// *never* block. Returning quickly prevents unnecessarily clogging up
/// An implementation of `poll` should strive to return quickly, and should
/// not block. Returning quickly prevents unnecessarily clogging up
/// threads or event loops. If it is known ahead of time that a call to
/// `poll` may end up taking awhile, the work should be offloaded to a
/// thread pool (or something similar) to ensure that `poll` can return
/// quickly.
///
/// # [`LocalWaker`], [`Waker`] and thread-safety
///
/// The `poll` function takes a [`LocalWaker`], an object which knows how to
/// awaken the current task. [`LocalWaker`] is not `Send` nor `Sync`, so in
/// order to make thread-safe futures the [`LocalWaker::into_waker`] method
/// should be used to convert the [`LocalWaker`] into a thread-safe version.
/// [`LocalWaker::wake`] implementations have the ability to be more
/// efficient, however, so when thread safety is not necessary,
/// [`LocalWaker`] should be preferred.
/// An implementation of `poll` may also never cause memory unsafety.
///
/// # Panics
///
/// Once a future has completed (returned `Ready` from `poll`),
Copy link
Contributor

Choose a reason for hiding this comment

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

Per https://github.com/rust-lang/rfcs/pull/2592/files#r250460358, clarify that "bad behavior" isn't violating soundness / memory unsafety.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment above.

/// then any future calls to `poll` may panic, block forever, or otherwise
/// cause bad behavior. The `Future` trait itself provides no guarantees
/// about the behavior of `poll` after a future has completed.
/// cause any kind of bad behavior expect causing memory unsafety.
/// The `Future` trait itself provides no guarantees about the behavior
/// of `poll` after a future has completed.
///
/// [`Poll::Pending`]: ../task/enum.Poll.html#variant.Pending
/// [`Poll::Ready(val)`]: ../task/enum.Poll.html#variant.Ready
/// [`LocalWaker`]: ../task/struct.LocalWaker.html
/// [`LocalWaker::into_waker`]: ../task/struct.LocalWaker.html#method.into_waker
/// [`LocalWaker::wake`]: ../task/struct.LocalWaker.html#method.wake
/// [`Waker`]: ../task/struct.Waker.html
fn poll(self: Pin<&mut Self>, lw: &LocalWaker) -> Poll<Self::Output>;
/// [`Waker::wake`]: ../task/struct.Waker.html#method.wake
fn poll(self: Pin<&mut Self>, waker: &Waker) -> Poll<Self::Output>;
}

impl<'a, F: ?Sized + Future + Unpin> Future for &'a mut F {
type Output = F::Output;

fn poll(mut self: Pin<&mut Self>, lw: &LocalWaker) -> Poll<Self::Output> {
F::poll(Pin::new(&mut **self), lw)
fn poll(mut self: Pin<&mut Self>, waker: &Waker) -> Poll<Self::Output> {
F::poll(Pin::new(&mut **self), waker)
}
}

Expand All @@ -115,7 +107,7 @@ where
{
type Output = <<P as ops::Deref>::Target as Future>::Output;

fn poll(self: Pin<&mut Self>, lw: &LocalWaker) -> Poll<Self::Output> {
Pin::get_mut(self).as_mut().poll(lw)
fn poll(self: Pin<&mut Self>, waker: &Waker) -> Poll<Self::Output> {
Pin::get_mut(self).as_mut().poll(waker)
}
}
2 changes: 1 addition & 1 deletion src/libcore/task/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@ mod poll;
pub use self::poll::Poll;

mod wake;
pub use self::wake::{Waker, LocalWaker, UnsafeWake};
pub use self::wake::{Waker, RawWaker, RawWakerVTable};
Loading