-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Conversation
This comment has been minimized.
This comment has been minimized.
r? @cramertj |
@cramertj I also started updating future-rs. However I currently had some build problems there, and it's a lot of stuff to change. Will look again tomorrow evening. |
@@ -42,16 +42,16 @@ pub trait Future { | |||
/// Once a future has finished, clients should not `poll` it 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.
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.
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.
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 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.
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. 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.
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).
/// 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 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.
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 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 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.
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.
done
/// [`LocalWaker::wake`] implementations have the ability to be more | ||
/// efficient, however, so when thread safety is not necessary, | ||
/// [`LocalWaker`] should be preferred. | ||
/// | ||
/// # Panics | ||
/// | ||
/// Once a future has completed (returned `Ready` from `poll`), |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above.
} | ||
|
||
impl fmt::Debug for LocalWaker { | ||
impl fmt::Debug for 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.
This would also be more useful by just #[derive(Debug)]
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 thought about it, but it fear this will end up super noisy. I think lots of user-structs will store Waker
in them and #[derive(Debug)]
. Those would then all the show 5 different pointers that have no meaning for the average user.
The main reason why I think it might be useful is that users don't have access to RawWaker
, so they can't get the inner information.
Maybe we should then only print the data
and vtable
pointer for RawWaker
, and not the inner function pointers. I think if I would need to ever debug those things, the main thing I would be if if Waker
s refer to the same task or not, and with a lower probability of they use the same vtable. The function pointers are are mostly worthless without names.
In general those things might also be more ones that one would investigate with a real in-memory-debugger than with print debugging.
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 assume adding something like debug_fmt: Option<unsafe fn(*const (), f: &mut fmt::Formatter) -> fmt::Result>
to RawWakerVTable
wouldn't be worth it? (and if RawWakerVTable
is made non_exhaustive
could be added later anyway).
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.
IMO printing the address of the data and vtable pointers seems helpful, but I would leave the substructure out.
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.
@cramertj's suggestion seems like a good have-your-🍰-eat-it approach.
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 pointer printing for Waker
. And yes, I don't think the fmt method is worth it.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☔ The latest upstream changes (presumably #58081) made this pull request unmergeable. Please resolve the merge conflicts. |
This change updates the future and task API as discussed in the stabilization RFC at rust-lang/rfcs#2592. Changes: - Replacing UnsafeWake with RawWaker and RawWakerVtable - Removal of LocalWaker - Removal of Arc-based Wake trait
Co-Authored-By: Matthias247 <matthias.einwag@live.com>
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/test/run-pass/async-await.rs
Outdated
unsafe fn increase_refcount<T: ArcWake>(data: *const()) { | ||
// Retain Arc by creating a copy | ||
let arc: Arc<T> = Arc::from_raw(data as *const T); | ||
let arc_clone = arc.clone(); |
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.
nit: if cloning the arc panics, the arc will be dropped, decreasing the refcount.
src/test/run-pass/futures-api.rs
Outdated
struct Counter { | ||
local_wakes: AtomicUsize, | ||
nonlocal_wakes: AtomicUsize, | ||
macro_rules! waker_vtable { |
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.
Rather than redefining Wake
and this waker_vtable
macro in every separate test, i'd have one file define it and path-include that file.
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 know, not happy with it either. However I only have no idea how the build and the conventions in this folder work. And since it takes 2 hours to get here, I couldn't really the spend time investigating it.
If anyone wants to push a commit which changes that part it would be great.
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.
#[path = "./path/to/waker_api.rs"] mod waker;
should work
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 moved it to the auxilary directory which seems to be used by other tests. But maybe that's wrong. If noone can tell where exactly it should go I will revert that.
This looks great! left a few small nits, but I'm excited to get this in :) |
After talking to @Centril I realized we could also keep the |
@cramertj , @Centril I like that idea, since it means we could extend |
@Matthias247 sounds good to me! |
@cramertj Updated. I was wondering if the function should be |
@GuillaumeGomez Sorry, I just it might save some work, and felt bad about removing tests. Let me know briefly if you still prefer the removal, then I will go for it. |
I'd prefer that you removed the test (and please open an issue). I just opened #58330 so that we can test it without depending on the std. |
@cramertj , @Centril Please retry the merge @GuillaumeGomez Alright, done. I opened the issue at #58331 |
@Matthias247 Thanks a lot! |
@bors r+ |
📌 Commit 1ef34a5 has been approved by |
☔ The latest upstream changes (presumably #58341) made this pull request unmergeable. Please resolve the merge conflicts. |
@cramertj: can you please try again? |
@bors r+ |
📌 Commit 871338c has been approved by |
Update the future/task API This change updates the future and task API as discussed in the stabilization RFC at rust-lang/rfcs#2592. Changes: - Replacing UnsafeWake with RawWaker and RawWakerVtable - Removal of LocalWaker - Removal of Arc-based Wake trait
Rollup of 8 pull requests Successful merges: - #57451 (suggestion-diagnostics: as_ref improve snippet) - #57856 (Convert old first edition links to current edition one) - #57992 (Update the future/task API) - #58258 (Reduce the size of `hir::Expr`.) - #58267 (Tweak "incompatible match arms" error) - #58296 (Hidden suggestion support) - #58301 (Enable comparing fat pointers) - #58308 (Extract block to insert an intrinsic into its own function) Failed merges: r? @ghost
This change updates the future and task API as discussed in the stabilization RFC at rust-lang/rfcs#2592.
Changes: