Add Wake trait for safe construction of Wakers. #68700
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @rkruppe (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @cramertj (maybe?) |
|
cc @rust-lang/libs |
|
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 |
NickeZ
reviewed
Jan 31, 2020
src/liballoc/task.rs
Outdated
| /// This trait is a memory-safe and ergonomic alternative to constructing a | ||
| /// [`RawWaker`]. It supports the common executor design in which the data | ||
| /// used to wake up a task is stored in an [`Arc`]. Some executors (especially | ||
| /// those for embedded systems) cannot use this API, which is way [`RawWaker`] |
|
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 |
tmiasko
reviewed
Jan 31, 2020
src/liballoc/task.rs
Outdated
| unsafe fn wake_by_ref<W: Wake + Send + Sync + 'static>(waker: *const ()) { | ||
| let waker: Arc<W> = Arc::from_raw(waker as *const W); | ||
| Wake::wake_by_ref(&waker); | ||
| mem::forget(waker); |
There was a problem hiding this comment.
Using ManuallyDrop might be a better choice in the case waker panics.
Nemo157
reviewed
Jan 31, 2020
| /// [`Arc`] and calls `wake` on the clone. | ||
| #[unstable(feature = "wake_trait", issue = "0")] | ||
| fn wake_by_ref(self: &Arc<Self>) { | ||
| self.clone().wake(); |
There was a problem hiding this comment.
futures::task::ArcWake has the opposite defaulted method. Is there a reason to assume that implementations would commonly need to own the Arc instead of being able to wake through a shared reference?
There was a problem hiding this comment.
In my experience working on romio/juliex and reading the tokio source, the standard executor model is to re-enqueue the task on some TLS or global queue (which means it must have ownership), and the standard reactor model is to store an Option<Waker> and then call .take().unwrap().wake(). IMO the default in futures is backwards, and would result in naive implementations making an additional unnecessary ref count increment.
There was a problem hiding this comment.
I don't have a strong opinion about this either way-- I can definitely imagine arguments on either side. The reason I did it the other way in futures-rs was that I imagined implementations which only needed by-reference waking would implement just wake not realizing that that would result in a wake_by_ref method which cloned unnecessarily. In practice, I don't think either case is too big of an issue.
There was a problem hiding this comment.
Yea, either way I think its a chance of an incorrectly implemented executor leading to one extra incr/decr on wakeup - a trivial and easily fixable problem.
But I do think the more likely scenario is that they do need ownership of the arc, rather than that they don't (the only point of it being by arc is to cheaply enqueue on wake up after all), and I thought our choice to name the fns wake and wake_by_ref was tied up in this assumption (that wake would be the default one).
|
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 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 |
cramertj
reviewed
Jan 31, 2020
withoutboats
added
A-async-await
|
@rfcbot fcp merge See first post for a full explanation |
|
Team member @withoutboats has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
rfcbot
added
proposed-final-comment-period
|
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 |
|
Looks great to me. Minor question: is there any use case for a Wake impl that is not Send + Sync + 'static? Did you evaluate having those as supertraits of Wake? @rfcbot reviewed |
No there isn't, but I put them on the bound instead of as supertrait since that's my understanding of how std normally does things. |
|
We had this in the original proposal and removed it. What is the motivation to add it now? Who is the target user group? Wakers are part of an executor system. And building good executors (which include IO, timers, etc) is a not something a bigger group of people will do. I don't see a lot of value in adding this. I certainly observe there is a trend of "build your own executor, because it's so neat to be able to do this in a couple lines of code". And that an addition like this would make it easier to write them. But I don't think that more toy executors really bring the ecosystem forward. |
|
@Matthias247 we removed it with the intention of always adding it once a good constructor API was possible. I don't agree that executors using this API would be "toys," in fact I would recommend this for production executor libraries in most server settings. There should be a safe way to construct a waker, period. |
rfcbot
added
the
final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
rfcbot
removed
the
proposed-final-comment-period
|
@Matthias247 more specifically, to quote the PR description:
So that's why it's happening now instead of in 2019. |
|
@bors r- could you rebase? There's an accidental merge commit. |
bors
added
S-waiting-on-author
Currently, constructing a waker requires calling the unsafe
`Waker::from_raw` API. This API requires the user to manually construct
a vtable for the waker themself - which is both cumbersome and very
error prone. This API would provide an ergonomic, straightforward and
guaranteed memory-safe way of constructing a waker.
It has been our longstanding intention that the `Waker` type essentially
function as an `Arc<dyn Wake>`, with a `Wake` trait as defined here. Two
considerations prevented the original API from being shipped as simply
an `Arc<dyn Wake>`:
- We want to support futures on embedded systems, which may not have an
allocator, and in optimized executors for which this API may not be
best-suited. Therefore, we have always explicitly supported the
maximally-flexible (but also memory-unsafe) `RawWaker` API, and
`Waker` has always lived in libcore.
- Because `Waker` lives in libcore and `Arc` lives in liballoc, it has
not been feasible to provide a constructor for `Waker` from `Arc<dyn
Wake>`.
Therefore, the Wake trait was left out of the initial version of the
task waker API.
However, as Rust 1.41, it is possible under the more flexible orphan
rules to implement `From<Arc<W>> for Waker where W: Wake` in liballoc.
Therefore, we can now define this constructor even though `Waker` lives
in libcore.
This PR adds these APIs:
- A `Wake` trait, which contains two methods
- A required method `wake`, which is called by `Waker::wake`
- A provided method `wake_by_ref`, which is called by
`Waker::wake_by_ref` and which implementors can override if they
can optimize this use case.
- An implementation of `From<Arc<W>> for Waker where W: Wake + Send +
Sync + 'static`
- A similar implementation of `From<Arc<W>> for RawWaker`.
Co-Authored-By: Ashley Mannix <ashleymannix@live.com.au>
Co-Authored-By: Ashley Mannix <ashleymannix@live.com.au>
Co-Authored-By: Ashley Mannix <ashleymannix@live.com.au>
withoutboats
force-pushed
the
wake-trait
branch
from
a9a7c80
to
32f5724
Compare
|
@bors r+ |
|
📌 Commit 32f5724 has been approved by |
bors
added
S-waiting-on-bors
Centril
added a commit
to Centril/rust
that referenced
this pull request
Mar 23, 2020
…oats
Add Wake trait for safe construction of Wakers.
Currently, constructing a waker requires calling the unsafe `Waker::from_raw` API. This API requires the user to manually construct a vtable for the waker themself - which is both cumbersome and very error prone. This API would provide an ergonomic, straightforward and guaranteed memory-safe way of constructing a waker.
It has been our longstanding intention that the `Waker` type essentially function as an `Arc<dyn Wake>`, with a `Wake` trait as defined here. Two considerations prevented the original API from being shipped as simply an `Arc<dyn Wake>`:
- We want to support futures on embedded systems, which may not have an allocator, and in optimized executors for which this API may not be best-suited. Therefore, we have always explicitly supported the maximally-flexible (but also memory-unsafe) `RawWaker` API, and `Waker` has always lived in libcore.
- Because `Waker` lives in libcore and `Arc` lives in liballoc, it has not been feasible to provide a constructor for `Waker` from `Arc<dyn Wake>`.
Therefore, the Wake trait was left out of the initial version of the task waker API.
However, as Rust 1.41, it is possible under the more flexible orphan rules to implement `From<Arc<W>> for Waker where W: Wake` in liballoc. Therefore, we can now define this constructor even though `Waker` lives in libcore.
This PR adds these APIs:
- A `Wake` trait, which contains two methods
- A required method `wake`, which is called by `Waker::wake`
- A provided method `wake_by_ref`, which is called by `Waker::wake_by_ref` and which implementors can override if they can optimize this use case.
- An implementation of `From<Arc<W>> for Waker where W: Wake + Send + Sync + 'static`
- A similar implementation of `From<Arc<W>> for RawWaker`.
This was referenced Mar 23, 2020
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 23, 2020
Rollup of 9 pull requests Successful merges: - rust-lang#68700 (Add Wake trait for safe construction of Wakers.) - rust-lang#69494 (Stabilize --crate-version option in rustdoc) - rust-lang#70080 (rustc_mir: remove extra space when pretty-printing MIR.) - rust-lang#70195 (Add test for issue rust-lang#53275) - rust-lang#70199 (Revised span-to-lines conversion to produce an empty vec on DUMMY_SP.) - rust-lang#70299 (add err_machine_stop macro) - rust-lang#70300 (Reword unused variable warning) - rust-lang#70315 (Rename remaining occurences of Void to Opaque.) - rust-lang#70318 (Split long derive lists into two derive attributes.) Failed merges: r? @ghost
This was referenced Apr 3, 2020
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Apr 3, 2020
…ylan-DPC Fix link in task::Wake docs `task::Wake` was introduced in rust-lang#68700. While I was browsing the docs I noticed a link to `sync::Arc` wasn't resolving correctly. This patch fixes that. Thanks! ## Before  ## Proposed 
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
May 7, 2020
… r=Mark-Simulacrum
Add Arc::{incr,decr}_strong_count
This adds two `unsafe` methods to `Arc`: `incr_strong_count` and `decr_strong_count`. A suggestion to add methods to change the strong count in `Arc` came up in during review in rust-lang#68700 (comment), and from asking a few people this seemed like generally useful to have.
References:
- [Motivation from rust-lang#68700](rust-lang#68700 (comment))
- [Real world example in an executor](https://docs.rs/extreme/666.666.666666/src/extreme/lib.rs.html#13)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, constructing a waker requires calling the unsafe
Waker::from_rawAPI. This API requires the user to manually construct a vtable for the waker themself - which is both cumbersome and very error prone. This API would provide an ergonomic, straightforward and guaranteed memory-safe way of constructing a waker.It has been our longstanding intention that the
Wakertype essentially function as anArc<dyn Wake>, with aWaketrait as defined here. Two considerations prevented the original API from being shipped as simply anArc<dyn Wake>:RawWakerAPI, andWakerhas always lived in libcore.Wakerlives in libcore andArclives in liballoc, it has not been feasible to provide a constructor forWakerfromArc<dyn Wake>.Therefore, the Wake trait was left out of the initial version of the task waker API.
However, as Rust 1.41, it is possible under the more flexible orphan rules to implement
From<Arc<W>> for Waker where W: Wakein liballoc. Therefore, we can now define this constructor even thoughWakerlives in libcore.This PR adds these APIs:
Waketrait, which contains two methodswake, which is called byWaker::wakewake_by_ref, which is called byWaker::wake_by_refand which implementors can override if they can optimize this use case.From<Arc<W>> for Waker where W: Wake + Send + Sync + 'staticFrom<Arc<W>> for RawWaker.