rust
rust copied to clipboard
Tracking Issue for `waker_getters`
Feature gate: #![feature(waker_getters)]
This is a tracking issue for getters of data and vtable pointers for RawWaker
, and the method to get RawWaker
from Waker
.
Public API
// core::task
impl RawWaker {
pub fn data(&self) -> *const ();
pub fn vtable(&self) -> &'static RawWakerVTable;
}
impl Waker {
pub fn as_raw(&self) -> &RawWaker;
}
Steps / History
- [x] Implementation: #91828
- [ ] Final comment period (FCP)
- [ ] Stabilization PR
Unresolved Questions
- Do we also need
fn into_raw(self) -> RawWaker
forWaker
? https://github.com/rust-lang/rust/issues/87021#issuecomment-1091899591
Maybe we could stabilize this? Note sure what I could do next. @rust-lang/libs-api
I am concerned about as_raw
, it's not clear to me how it is intended to be used. It takes Waker
by reference and returns a RawWaker
, which you can then decompose into a vtable and data pointer. However I don't see how you can then re-construct a reference to the original Waker
: doing so with from_raw
will create a second Waker
which will call the drop
function in the vtable when dropped. This can lead to a double-drop once the original Waker
is dropped as well.
@Amanieu
I am concerned about as_raw, it's not clear to me how it is intended to be used. It takes Waker by reference and returns a RawWaker
It returns &RawWaker
, not RawWaker
, thus it's a getter rather than an ownership transfer. Its purpose is to allow getting or checking the underlying data. Reconstructing a [Raw]Waker
from the result pointers is indeed incorrect, and unnecessary due to the existence of Waker::clone
.
If we do want reconstruction, I'd consider adding another fn into_raw(Waker) -> RawWaker
, which also consists with Waker::from_raw
. But I think it's less useful, because we can only access &mut Context
and get a &Waker
. We don't own Waker without explicitly cloning it somewhere.
Then I'm afraid that I don't understand how as_raw
is intended to be used. What can you actually do with the returned vtable and data? I can understand passing it through FFI, but then you eventually have to pass it back to Rust code to use it as a Waker
.
@Amanieu
Then I'm afraid that I don't understand how as_raw is intended to be used.
It's effectively downcast_ref
for Waker
. It allows validating Waker VTables, ad-hoc optimization, or retrieving data from known Wakers, where the async runtime only supports Wakers from itself. I found an example in,
https://github.com/embassy-rs/embassy/blob/be55d2a39eb399d693d11f37dfa4b87cd2bd3c7a/embassy-executor/src/raw/waker.rs#L42
I can understand passing it through FFI, but then you eventually have to pass it back to Rust code to use it as a Waker.
In case of passing through FFI, we need do &Waker
-> [*const (); 2]
-> &Waker
[^1] without ownership transfer. The last step can be done via reconstructing ManuallyDrop<Waker>
and using the reference to inner. If we want to transfer the ownership via, to say fn into_raw(Waker) -> RawWaker
, we need an extra Waker::clone
during every poll
.
Note that the ManuallyDrop<Waker>
- &Waker
pattern above can be found in,
https://github.com/tokio-rs/tokio/blob/c693ccd210c7a318957b0701f4a9632b2d545d6a/tokio/src/util/wake.rs#L18-L22
[^1]: Since the VTable representation is repr(Rust). Here we actually need to create a new repr(C) VTable for forwarding method calls, rather than to pass the pointer through FFI directly. This is doable currently.
I feel that a better story is needed for getting a &Waker
from a &RawWaker
. Perhaps something like this?
impl Waker {
unsafe fn with_raw<T>(raw: &RawWaker, f: impl FnOnce(&Waker) -> T) -> T;
}
Also I do think that we should add into_raw
for consistency with from_raw
.
I feel that a better story is needed for getting a
&Waker
from a&RawWaker
. Perhaps something like this?impl Waker { unsafe fn with_raw<T>(raw: &RawWaker, f: impl FnOnce(&Waker) -> T) -> T; }
This API seems to be the only choice since we cannot let temporary Waker
instance escape, but it would be less flexible for the tokio
case. It's not possible to return a impl Deref<Target = Waker>
via this API. For the FFI case, it seems to work though.
Also this API is more like a helper, which is able to be implemented outside std (RawWaker::data
cannot, due to repr(Rust)
). Should it also be a part of waker_getters
?
I think it should be inlcuded as the counterpart to Waker::as_raw
, otherwise it isn't clear how the RawWaker
returned by as_raw
can be safely used. This is important for the doc examples for these functions.
For tokio, it seems to only be used here. It should be easy to refactor that to use with_raw
.
@Amanieu
Noticing that Waker
is repr(transparent)
, it should be safe to transmute
from &RawWaker
to &Waker
I think?
Then the with_raw
you suggested can be replaced by,
impl Waker {
pub unsafe fn from_raw_ref(raw: &RawWaker) -> &Waker { transmute(raw) }
}
~~I have a pretty simple use case for this: checking if two Wakers are the same.~~
~~The Future contract requires wake to be called on the Waker from the latest poll, so on every poll a Future that keeps its Waker stored have to replace its stored Waker with one newly cloned from the Context. Waker::as_raw
would allow us to first compare the two Wakers, avoiding replacement (and the atomic operations involved) if they are the same.~~
Didn't realize will_wake
does this already! Thanks @Thomasdezeeuw
Because Waker
has private fields, the #[repr(transparent)]
is an implementation detail and not an API guarantee that users can rely on. It isn't shown in rustdoc for this reason.
The problem with from_raw_ref
is that it effectively turns this into a public guarantee: it might be something we can commit to but it would have a higher barrier for acceptance.
To add another use case is storing the waker atomically. You can split the data and vtable into two AtomicU64
and store those atomically separately (with some careful programming) or in a single AtomicU128
(on 64 bit archs at least).
I have a pretty simple use case for this: checking if two Wakers are the same.
The Future contract requires wake to be called on the Waker from the latest poll, so on every poll a Future that keeps its Waker stored have to replace its stored Waker with one newly cloned from the Context.
Waker::as_raw
would allow us to first compare the two Wakers, avoiding replacement (and the atomic operations involved) if they are the same.
@wishawa wote that task::Waker::will_wake
exists for this.
I think it might be natural to think that a Waker
could do more than just wake()
. Like how a Box<dyn Error>
might be a specific kind of error with some fields we would care about.
I can imagine as situation where you might want to check if the current Waker
is a FooWaker
in order to extract a field from it, eg a TaskID.
(I am exploring an idea at the moment to hack in a go like context.Context
typemap into core::task::Context
through this mechanism.)
In order to do this, I could verify that waker.raw_waker().vtable() == &FooWakerVTable
.
(in a similar respect to Error
and source
, it would be interesting to have a waker
chain, especially with the recent push to structured concurrency with many nested sub-executors, but that is for a separate issue and another time)
Hi, now that stabby
is out, I'm eagerly awaiting accessors for the contents of the vtable and the date pointer, so that they may be passed over the ffi frontier in an ffi safe manner without having to wrap the waker in a stable trait object at every poll, which is what stabby is currently forced to do to safely support futures being passed across the ffi frontier.
Note that since the vtable isn't repr(c), and the functions use the default calling convention, access to the vtable but not the functions in contains is not sufficient: I'd need accessors to at least one of the wakes, clone and drop.
As a second point, is there a good reason why the function pointers use the (unstable) rust calling convention? This means the function pointers couldn't be passed across ffi even if they were accessible...
Anything I could do to help with the process?
Hi again,
I've started a new RFC (https://github.com/rust-lang/rfcs/pull/3404) which, while orthogonal code-wise, has the same end-purpose of allowing the safe passage of wakers across the FFI.
A key issue that waker_getters
don't address is that the vtable is composed of extern "rust"
function pointers, which can't safely cross the FFI bounday. My proposed solution involves modifying the current layout of RawWakerVTable
, which would make accessing the vtable's inner function pointers fallible.
I just wanted this stated somewhere to avoid stabilizing accessors that would prevent the vtable from being made truly FFI-safe without breaks in the accessors API. For example, an eventual RawWakerVTable::get_wake_fn(&self) -> unsafe fn(*const ())
would prevent proposing layouts for the vtable that may not contain said function pointer. Should such accessors be proposed, I strongly suggest considering making them of the form RawWakerVTable::get_wake_fn(&self) -> Option<unsafe fn(*const ())>
I think it might be natural to think that a
Waker
could do more than justwake()
. Like how aBox<dyn Error>
might be a specific kind of error with some fields we would care about.I can imagine as situation where you might want to check if the current
Waker
is aFooWaker
in order to extract a field from it, eg a TaskID. (I am exploring an idea at the moment to hack in a go likecontext.Context
typemap intocore::task::Context
through this mechanism.)In order to do this, I could verify that
waker.raw_waker().vtable() == &FooWakerVTable
.
We ended up doing the exact same thing. These getters allow us to provide executor-specific extensions on std::task::Context
, by verifying that a specific context is indeed one of our contexts (kind of RTTI).
Most async-runtimes have global executors that you use to spawn tasks. But with feature(waker_getters)
you can actually implement features like Executor::spawn(f: impl Future, cx: std::task::Context) -> Executor::Task
which would use the passed std::task::Context
to access state of the calling future and allow spawning another future on the same executor / scheduling-class / etc. You can also use it to modify scheduling priorities / properties of the calling future, and much more.
Without this you need a lookup-tree to get your internal state from a public Context
, even though the data is already stored in the context.
One way to construct a value within a function and return its reference is to have its memory location passed in. It's a little gross, but for example:
impl Waker {
unsafe fn from_raw<'a, 'b: 'a>(
raw: &'b RawWaker,
output_mem: &'a mut MaybeUninit<Waker>,
) -> &'a Waker {
// MaybeUninit doesn't drop
output_mem.write(Waker::from_raw(RawWaker::new(raw.data(), raw.vtable())));
output_mem.assume_init_ref()
}
}
EDIT: fixed the code
Would it make sense to have the API look like this?
// core::task
impl RawWaker {
pub fn data(&self) -> *const ();
pub fn vtable(&self) -> &'static RawWakerVTable;
}
impl Waker {
pub unsafe fn clone_from_raw(waker: &RawWaker) -> Waker;
pub fn as_raw(&self) -> &RawWaker;
pub fn into_raw(self) -> RawWaker;
}
Or a slightly different take:
// core::task
impl RawWaker {
pub fn data(&self) -> *const ();
pub fn vtable(&self) -> &'static RawWakerVTable;
}
impl Waker {
// Maybe named something different?
pub fn raw_with<T>(&self, f: impl FnOnce(&RawWaker) -> T) -> T;
pub fn into_raw(self) -> RawWaker;
}
My issue with this feature is that it's not really usable with alloc::task::Wake
. Could we add the following TryFrom
implementations?
impl<W: Wake + Send + Sync + 'static> TryFrom<Waker> for Arc<W> {/*...*/}
impl<'a, W: Wake + Send + Sync + 'static> TryFrom<&'a, Waker> for &'a Arc<W> {/*...*/}
(for the symmetry with the already existing From
implementation)
Should it be moved in another feature?
What's left to move forward with this? It would enable Context
extension experimentation outside of std, which could help accelerate async runtime interop.
(NOT A CONTRIBUTION)
This would be a useful API & I'd like to see it stabilized. I don't think any of the discussion on this issue should be blocking on stabilizing the APIs already under this feature.
My motivation is for a tightly bound executor/reactor: I want to check in the reactor if a registered waker has the vtable for "my" executor, so that I can avoid making the virtual wake call and directly re-enqueue the task. I don't need to pass the Waker through FFI, I don't need to get a waker back from a RawWaker, these are not necessary features for this API to be useful.
Because Waker has private fields, the #[repr(transparent)] is an implementation detail and not an API guarantee that users can rely on. It isn't shown in rustdoc for this reason.
I don't think the conversion from &RawWaker to &Waker is needed for this API to be useful, but I also find it extremely hard to imagine Waker ever gaining any other fields. We added Context specifically to have a place to add fields that wasn't Waker. I think T-libs should guarantee the layout of Waker.
@rust-lang/libs-api: @rfcbot fcp merge
I propose that we stabilize the following 3 accessors, which have been available since Rust 1.60-nightly (2 years).
// core::task
impl RawWaker {
pub fn data(&self) -> *const ();
pub fn vtable(&self) -> &'static RawWakerVTable;
}
impl Waker {
pub fn as_raw(&self) -> &RawWaker;
}
The use case is described succinctly and convincingly in https://github.com/rust-lang/rust/issues/96992#issuecomment-1343487771.
It's effectively downcast_ref for Waker. It allows validating Waker VTables, ad-hoc optimization, or retrieving data from known Wakers, where the async runtime only supports Wakers from itself.
Here is code in embassy-executor
that needs downcasting, and does it in a nasty way today to work around the lack of stable accessors. The workaround involves assuming implementation details of the standard library's data structures and compiler's struct layout.
fn task_from_waker(waker: &Waker) -> TaskRef {
///🤞
struct WakerHack {
data: *const (),
vtable: &'static RawWakerVTable,
}
let hack: &WakerHack = unsafe { mem::transmute(waker) };
if hack.vtable != &VTABLE {
panic!("Found waker not created by the Embassy executor. `embassy_time::Timer` only works with the Embassy executor.");
}
unsafe { TaskRef::from_ptr(hack.data as *const TaskHeader) }
}
By making stable accessors available, this would be:
fn task_from_waker(waker: &Waker) -> TaskRef {
let raw_waker = waker.as_raw(); // <--
if raw_waker.vtable() != &VTABLE { // <--
panic!("...");
}
let ptr = raw_waker.data(); // <--
unsafe { TaskRef::from_ptr(ptr as *const TaskHeader) }
}
The importance of this use case is reinforced by https://github.com/rust-lang/rust/issues/96992#issuecomment-1900106571, and https://github.com/rust-lang/rust/issues/96992#issuecomment-1880141049.
Alternatives
I would be open to considering making public fields on RawWaker
, instead of accessors. This is in fact what is shown in the core::task
stabilization RFC. As far as I can tell, https://github.com/rust-lang/rfcs/pull/2592#issuecomment-458944053 is what precipitated the change to private fields under the possibility that adding a 3rd field Option<TypeId>
would turn out to be useful, which seems obsolete as a rationale.
// core::task
pub struct RawWaker {
pub data: *const (),
pub vtable: &'static RawWakerVTable,
}
impl RawWaker {
pub const fn new(data: *const (), vtable: &'static RawWakerVTable) -> Self; // (already stable)
}
Or, move the accessors to Waker
and phase out the use of RawWaker
. I tried looking into why Waker
and RawWaker
are separate types and failed to find a good reason. My guess is this pre-dates the separation of Waker
from Context
, and might no longer be well motivated now that Context
serves as our extension point going forward (for things like LocalWaker
support) and Waker
will remain permanently just a RawWaker
as articulated in https://github.com/rust-lang/rust/issues/96992#issuecomment-1900106571.
// core::task
pub struct Waker {
data: *const (),
vtable: &'static RawWakerVTable,
}
impl Waker {
pub const unsafe fn new(data: *const (), vtable: &'static RawWakerVTable) -> Self;
pub fn data(&self) -> *const ();
pub fn vtable(&self) -> &'static RawWakerVTable;
#[deprecated]
pub unsafe fn from_raw(waker: RawWaker) -> Self;
}
Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:
- [x] @Amanieu
- [x] @BurntSushi
- [x] @dtolnay
- [ ] @joshtriplett
- [ ] @m-ou-se
Concerns:
- Alternative 1: public RawWaker fields (https://github.com/rust-lang/rust/issues/96992#issuecomment-1902780569)
- Alternative 2: phase out RawWaker (https://github.com/rust-lang/rust/issues/96992#issuecomment-1902780569)
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 concern Alternative 1: public RawWaker fields @rfcbot concern Alternative 2: phase out RawWaker
:+1: for going with alternative 1. (Please feel free to check my box for an FCP that proposes alternative 1.)
@rfcbot fcp cancel
We talked about this in today's libs-api meeting and think we're on board with giving RawWaker
public data
and vtable
fields, rather than a .data()
and .vtable()
accessor method
Let me go ahead and FCP that (Alternative 1 from above).
@dtolnay proposal cancelled.
@rust-lang/libs-api: @rfcbot fcp merge
I propose stabilizing the following counterproposal:
// core::task
impl Waker {
pub fn as_raw(&self) -> &RawWaker; // unstable since 1.60-nightly
}
pub struct RawWaker {
pub data: *const (), // change from private to public
pub vtable: &'static RawWakerVTable, // change from private to public
}
and deleting these 2 unstable accessors currently tracked by this tracking issue:
impl RawWaker {
pub fn data(&self) -> *const ();
pub fn vtable(&self) -> &'static RawWakerVTable;
}
RawWaker
was originally stabilized with private fields related to a plan to maybe add a 3rd field Option<TypeId>
(https://github.com/rust-lang/rfcs/pull/2592#issuecomment-458944053).
An Option<TypeId>
field was envisioned to support downcasting Waker based on the type of object pointed to by data
.
Instead, modern implementations such as embassy have done downcasting based on the value of vtable
, not the type of the object pointed by data
. This is just as good and no separate TypeId
is needed. See https://github.com/rust-lang/rust/issues/96992#issuecomment-1902780274 for links to the relevant code.
Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:
- [x] @Amanieu
- [ ] @BurntSushi
- [x] @dtolnay
- [x] @joshtriplett
- [ ] @m-ou-se
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.
Is there a downside to using public accessors? It seems like the more conservative route. And are there compelling use cases specifically for exposing the fields?
(I don't have any use case in mind for adding another field to RawWaker
. I am only coming at this from the angle of preserving for the future unless there's a good reason not to.)