-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Update the future/task API #57992
Changes from 6 commits
d9a4b22
01a704c
9e6bc3c
f005e1c
e1ec814
363e992
8e7ef03
a1c4cf6
1ef34a5
871338c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
/// | ||
|
@@ -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 | ||
|
@@ -42,16 +43,16 @@ pub trait Future { | |
/// Once a future has finished, clients should not `poll` it again. | ||
/// | ||
/// 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Below it reads:
Per https://github.com/rust-lang/rfcs/pull/2592/files#r250460257, clarify that callers of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
@@ -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`), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} | ||
} | ||
|
||
|
@@ -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) | ||
} | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference here is that
Iterator::next
doesn't imply in any way (in fact, the opposite) thatNone
+None
isn't a valid sequence. Meanwhile, "should notpoll
it again" is emphatic about what should not happen, so a reader may assume this is always true.There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 returningPoll::Pending
without scheduling a wakeup, or anything else that doesn't break memory safety. So, an executor or wrapping future callingpoll
again afterReady
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.There was a problem hiding this comment.
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. WithIterator
you have an easy option of just returningNone
again, so while polling it again is a bug you have less chance of bringing your system down. WithFuture
you can't easily returnReady
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.There was a problem hiding this comment.
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).