Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd Wake trait for safe construction of Wakers. #68700
Conversation
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 comment has been minimized.
This comment has been minimized.
|
r? @rkruppe (rust_highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
|
r? @cramertj (maybe?) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
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 |
| /// 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`] |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
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 |
| 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); |
This comment has been minimized.
This comment has been minimized.
| /// [`Arc`] and calls `wake` on the clone. | ||
| #[unstable(feature = "wake_trait", issue = "0")] | ||
| fn wake_by_ref(self: &Arc<Self>) { | ||
| self.clone().wake(); |
This comment has been minimized.
This comment has been minimized.
Nemo157
Jan 31, 2020
Contributor
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?
This comment has been minimized.
This comment has been minimized.
withoutboats
Jan 31, 2020
•
Author
Contributor
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.
This comment has been minimized.
This comment has been minimized.
cramertj
Jan 31, 2020
Member
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.
This comment has been minimized.
This comment has been minimized.
withoutboats
Feb 2, 2020
•
Author
Contributor
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).
This comment has been minimized.
This comment has been minimized.
|
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 |
This comment has been minimized.
This comment has been minimized.
|
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 |
This comment has been minimized.
This comment has been minimized.
|
@rfcbot fcp merge See first post for a full explanation |
This comment has been minimized.
This comment has been minimized.
rfcbot
commented
Feb 2, 2020
•
|
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. |
This comment has been minimized.
This comment has been minimized.
|
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 |
This comment has been minimized.
This comment has been minimized.
|
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 |
This comment has been minimized.
This comment has been minimized.
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. |
withoutboats commentedJan 31, 2020
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.