Skip to content
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

Tracking Issue for waker_getters #96992

Closed
3 tasks done
oxalica opened this issue May 12, 2022 · 57 comments · Fixed by #129919
Closed
3 tasks done

Tracking Issue for waker_getters #96992

oxalica opened this issue May 12, 2022 · 57 comments · Fixed by #129919
Labels
A-async-await Area: Async & Await AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. I-async-nominated Nominated for discussion during an async working group meeting. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. WG-async Working group: Async & await
Milestone

Comments

@oxalica
Copy link
Contributor

oxalica commented May 12, 2022

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

Unresolved Questions

@oxalica oxalica added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels May 12, 2022
@oxalica
Copy link
Contributor Author

oxalica commented Sep 8, 2022

Maybe we could stabilize this? Note sure what I could do next. @rust-lang/libs-api

@Amanieu
Copy link
Member

Amanieu commented Dec 8, 2022

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.

@oxalica
Copy link
Contributor Author

oxalica commented Dec 8, 2022

@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.

@Amanieu
Copy link
Member

Amanieu commented Dec 8, 2022

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.

@oxalica
Copy link
Contributor Author

oxalica commented Dec 8, 2022

@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

Footnotes

  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.

@Amanieu
Copy link
Member

Amanieu commented Dec 8, 2022

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.

@oxalica
Copy link
Contributor Author

oxalica commented Dec 10, 2022

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?

@Amanieu
Copy link
Member

Amanieu commented Dec 10, 2022

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.

@oxalica
Copy link
Contributor Author

oxalica commented Dec 17, 2022

@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) }
}

@wishawa
Copy link

wishawa commented Dec 28, 2022

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

@Amanieu
Copy link
Member

Amanieu commented Dec 29, 2022

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.

@Thomasdezeeuw
Copy link
Contributor

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.

@conradludgate
Copy link
Contributor

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)

@p-avital
Copy link

p-avital commented Mar 27, 2023

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?

@p-avital
Copy link

p-avital commented Mar 29, 2023

Hi again,

I've started a new RFC (rust-lang/rfcs#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 ())>

@dvdhrm
Copy link
Contributor

dvdhrm commented May 8, 2023

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.

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.

@jkarneges
Copy link
Contributor

jkarneges commented Oct 30, 2023

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

@AldaronLau
Copy link
Contributor

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;
}

@wyfo
Copy link

wyfo commented Dec 1, 2023

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?

workingjubilee added a commit to workingjubilee/rustc that referenced this issue Dec 13, 2023
…ng_issue_number, r=workingjubilee

fix `waker_getters` tracking issue number

The feature currently links to the closed issue rust-lang#87021. Make it link to the tracking issue rust-lang#96992 instead.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 13, 2023
Rollup merge of rust-lang#118873 - lukas-code:fix_waker_getter_tracking_issue_number, r=workingjubilee

fix `waker_getters` tracking issue number

The feature currently links to the closed issue rust-lang#87021. Make it link to the tracking issue rust-lang#96992 instead.
@jkarneges
Copy link
Contributor

What's left to move forward with this? It would enable Context extension experimentation outside of std, which could help accelerate async runtime interop.

@withoutboats
Copy link
Contributor

(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.

@dtolnay
Copy link
Member

dtolnay commented Jan 21, 2024

@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 #96992 (comment).

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 #96992 (comment), and #96992 (comment).

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, rust-lang/rfcs#2592 (comment) 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 #96992 (comment).

// 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;
}

@dtolnay
Copy link
Member

dtolnay commented Feb 17, 2024

👍 Thank you, good catch. I guess we will not be deprecating.

bors pushed a commit to rust-lang-ci/rust that referenced this issue Feb 18, 2024
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Feb 23, 2024
@rfcbot
Copy link

rfcbot commented Feb 23, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@madsmtm
Copy link
Contributor

madsmtm commented Feb 24, 2024

I... realize that this stabilization process has gone on for a while, so totally understand if you don't want to bother with re-doing it, but:

Given that RawWaker is clearly still useful, and may even be used in LocalWaker, I believe that Alternative 2 #96992 (comment), stabilizing the fields of RawWaker, feels like the more coherent solution.

(A very hypothetical example: if the RawWakerVTable fields ever become public, if I end up manually calling the clone function, I have to construct a Waker to be able to access the fields of the RawWaker returned from that function, which seems backwards).


I'd also like to add, if you don't want to commit to a specific number of fields in RawWaker, you could consider adding #[non_exhaustive].

@traviscross traviscross added the AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. label Feb 26, 2024
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Feb 29, 2024
@jkarneges
Copy link
Contributor

This will be merged soon.

It looks like there was a vote to merge, but this issue isn't a PR and it remains open. What's next here?

@wyfo
Copy link

wyfo commented Apr 6, 2024

As I have already mentioned, this feature is not really usable with alloc::task::Wake.

Could it be possible to add the following implementation?

impl<W: Wake + Send + Sync + 'static> Arc<W> {
    const fn waker_vtable() -> &'static RawWakerVTable { ... }
}

It would allow to match the vtable returned by RawWaker::vtable.

@conradludgate
Copy link
Contributor

this feature is not really usable with alloc::task::Wake.

Does Waker::from(&arc_wake).as_raw().vtable() not work?

@wyfo
Copy link

wyfo commented Apr 7, 2024

Does Waker::from(&arc_wake).as_raw().vtable() not work?

Yes it works, but it requires to already have an allocated arc_wake, which makes the thing unusable.

The goal is to compare the vtable of a random waker to know if I can cast its data pointer to my own waker implementation. I don't want to rely for this comparison on a lazily allocated waker to get its vtable (especially as this lazy waker may requires some parameters random to be initialized). In short, this is unusable.
That's too bad, because Wake is a convenient implementation shortcut, but I have to rewrite it myself to use this feature.

zjp-CN added a commit to zjp-CN/embassy that referenced this issue Jun 9, 2024
Since rust-lang/rust#96992 has stalled,
to prevent potential unsoundness caused by transmuting to &WakerHack,
we can use nightly waker_getters APIs by gating it behind nightly
feature in embassy-executor without waiting for it to be stablized.
@Hawk777
Copy link

Hawk777 commented Jun 9, 2024

If I understand correctly, the FCP has happened and ended with a disposition to merge. Is there some reason why the FCP checkbox in the first comment is still unticked? In terms of the stabilization PR, is that just waiting for someone to do it, or is there some missing prerequisite?

@Amanieu
Copy link
Member

Amanieu commented Jul 8, 2024

This is just waiting for someone to submit a stabilization PR.

@kevinmehall
Copy link
Contributor

kevinmehall commented Sep 2, 2024

Submitted a stabilization PR: #129919

kevinmehall added a commit to kevinmehall/rust that referenced this issue Sep 3, 2024
kevinmehall added a commit to kevinmehall/rust that referenced this issue Sep 3, 2024
Per the `waker_getters` FCP:
rust-lang#96992 (comment)

Docs largely copied from `RawWaker::new`.
@bors bors closed this as completed in c7c3ada Sep 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 5, 2024
Rollup merge of rust-lang#129919 - kevinmehall:waker-getters, r=dtolnay

Stabilize `waker_getters`

Tracking issue: rust-lang#96992

FCP completed on the tracking issue a while ago. It's not clear whether the libs-api team wanted the `RawWaker` methods moved to `Waker` or went back to the current API after further discussion. `@Amanieu` [wrote "This is just waiting for someone to submit a stabilization PR."](rust-lang#96992 (comment)) so I'm doing just that in hopes of nudging this along.

Edit: Moved the `data` and `vtable` methods from `RawWaker` to `Waker` and added `Waker::new` per rust-lang#96992 (comment)

```rs
impl Waker {
  pub const unsafe fn new(data: *const (), vtable: &'static RawWakerVTable) -> Self;
  pub fn data(&self) -> *const ();
  pub fn vtable(&self) -> &'static RawWakerVTable;
}
```

Closes rust-lang#96992
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Sep 6, 2024
Stabilize `waker_getters`

Tracking issue: #96992

FCP completed on the tracking issue a while ago. It's not clear whether the libs-api team wanted the `RawWaker` methods moved to `Waker` or went back to the current API after further discussion. `@Amanieu` [wrote "This is just waiting for someone to submit a stabilization PR."](rust-lang/rust#96992 (comment)) so I'm doing just that in hopes of nudging this along.

Edit: Moved the `data` and `vtable` methods from `RawWaker` to `Waker` and added `Waker::new` per rust-lang/rust#96992 (comment)

```rs
impl Waker {
  pub const unsafe fn new(data: *const (), vtable: &'static RawWakerVTable) -> Self;
  pub fn data(&self) -> *const ();
  pub fn vtable(&self) -> &'static RawWakerVTable;
}
```

Closes #96992
@traviscross traviscross added this to the 1.83.0 milestone Sep 6, 2024
kaspar030 added a commit to kaspar030/embassy that referenced this issue Sep 9, 2024
waker_getters have been stabilized for 1.83.
See rust-lang/rust#96992.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. I-async-nominated Nominated for discussion during an async working group meeting. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. WG-async Working group: Async & await
Projects
None yet
Development

Successfully merging a pull request may close this issue.