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 LocalWaker #118959

Open
1 of 4 tasks
tvallotton opened this issue Dec 15, 2023 · 12 comments
Open
1 of 4 tasks

Tracking Issue for LocalWaker #118959

tvallotton opened this issue Dec 15, 2023 · 12 comments
Labels
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. WG-async Working group: Async & await

Comments

@tvallotton
Copy link
Contributor

tvallotton commented Dec 15, 2023

Feature gate: #![feature(local_waker)]

This is a tracking issue for support for local wakers on Context. This allows libraries to hold non thread safe data on their wakers, guaranteeing at compile time that the wakers will not be sent across threads. It includes a ContextBuilder type for building contexts.

Public API

impl Context {
    fn local_waker(&self) -> &LocalWaker;
    fn try_waker(&self) -> Option<Waker>;
}

impl ContextBuilder {
   fn from_local_waker(self) -> Self;
   fn from_waker(self) -> Self;
   fn waker(self, waker: Waker);
   fn local_waker(self, waker: LocalWaker);
   fn build(self) -> Context;
}

impl From<&mut Context> for ContextBuilder;

pub trait LocalWake {
    fn wake(self: Rc<Self>);
}

Steps / History

Unresolved Questions

  • Should runtimes be allowed to not define a waker?

Relevant links

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html

@tvallotton tvallotton 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 Dec 15, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 5, 2024
…ulacrum

Add LocalWaker and ContextBuilder types to core, and LocalWake trait to alloc.

Implementation for  rust-lang#118959.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 5, 2024
…ulacrum

Add LocalWaker and ContextBuilder types to core, and LocalWake trait to alloc.

Implementation for  rust-lang#118959.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 5, 2024
…ulacrum

Add LocalWaker and ContextBuilder types to core, and LocalWake trait to alloc.

Implementation for  rust-lang#118959.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 5, 2024
Rollup merge of rust-lang#118960 - tvallotton:local_waker, r=Mark-Simulacrum

Add LocalWaker and ContextBuilder types to core, and LocalWake trait to alloc.

Implementation for  rust-lang#118959.
@tvallotton
Copy link
Contributor Author

tvallotton commented Feb 12, 2024

@zzwxh

If I don't intend to support cross-thread wake-ups, it's best not to force me to provide a dummy Waker that pretends to support cross-thread wake-ups.

I agree, it would be best if users could panic on the call to cx.waker() rather than in the call to waker.wake().

ContextBuilder should allow the asynchronous runtime to explicitly specify which type of Waker it supports (or both), and the Future should have the ability to retrieve this information.

The feature used to work exactly like this, offering a ContextBuilder::from_local_waker method, and a Context::try_waker method. However, too many concerns were raised about compatibility, and I decided that this was not a hill I was willing to die on. Note that it is still possible to reintroduce these methods in the future without breaking changes.

@Thomasdezeeuw
Copy link
Contributor

Context::try_waker does not introduce breaking changes, and I don't understand what concerns anyone has. We just need to deprecate Context::from_raw and Context::waker and use the new API instead.

I'm not sure you know what you're saying here. "Just" deprecating a crucial API for all Futures is bad idea. It will break all existing Futures and creates massive churn.

@tvallotton
Copy link
Contributor Author

@zzwxh

Context::try_waker does not introduce breaking changes

Yes, this is what I said.

We just need to deprecate Context::from_raw and Context::waker and use the new API instead.

No, Context::waker and Context::from_waker are still useful and should not be deprecated, even if we decide to support optional wakers.

@Thomasdezeeuw
Copy link
Contributor

@zzwxh it's not ok that you deleted your comment. It's hard to read back a discussion that way.

bors pushed a commit to rust-lang-ci/rust that referenced this issue Feb 18, 2024
…ulacrum

Add LocalWaker and ContextBuilder types to core, and LocalWake trait to alloc.

Implementation for  rust-lang#118959.
jhpratt added a commit to jhpratt/rust that referenced this issue Apr 3, 2024
Add `Context::ext`

This change enables `Context` to carry arbitrary extension data via a single `&mut dyn Any` field.

```rust
#![feature(context_ext)]

impl Context {
    fn ext(&mut self) -> &mut dyn Any;
}

impl ContextBuilder {
    fn ext(self, data: &'a mut dyn Any) -> Self;

    fn from(cx: &'a mut Context<'_>) -> Self;
    fn waker(self, waker: &'a Waker) -> Self;
}
```

Basic usage:

```rust
struct MyExtensionData {
    executor_name: String,
}

let mut ext = MyExtensionData {
    executor_name: "foo".to_string(),
};

let mut cx = ContextBuilder::from_waker(&waker).ext(&mut ext).build();

if let Some(ext) = cx.ext().downcast_mut::<MyExtensionData>() {
    println!("{}", ext.executor_name);
}
```

Currently, `Context` only carries a `Waker`, but there is interest in having it carry other kinds of data. Examples include [LocalWaker](rust-lang#118959), [a reactor interface](rust-lang/libs-team#347), and [multiple arbitrary values by type](https://docs.rs/context-rs/latest/context_rs/). There is also a general practice in the ecosystem of sharing data between executors and futures via thread-locals or globals that would arguably be better shared via `Context`, if it were possible.

The `ext` field would provide a low friction (to stabilization) solution to enable experimentation. It would enable experimenting with what kinds of data we want to carry as well as with what data structures we may want to use to carry such data.

Dedicated fields for specific kinds of data could still be added directly on `Context` when we have sufficient experience or understanding about the problem they are solving, such as with `LocalWaker`. The `ext` field would be for data for which we don't have such experience or understanding, and that could be graduated to dedicated fields once proven.

Both the provider and consumer of the extension data must be aware of the concrete type behind the `Any`. This means it is not possible for the field to carry an abstract interface. However, the field can carry a concrete type which in turn carries an interface. There are different ways one can imagine an interface-carrying concrete type to work, hence the benefit of being able to experiment with such data structures.

## Passing interfaces

Interfaces can be placed in a concrete type, such as a struct, and then that type can be casted to `Any`. However, one gotcha is `Any` cannot contain non-static references. This means one cannot simply do:

```rust
struct Extensions<'a> {
    interface1: &'a mut dyn Trait1,
    interface2: &'a mut dyn Trait2,
}

let mut ext = Extensions {
    interface1: &mut impl1,
    interface2: &mut impl2,
};

let ext: &mut dyn Any = &mut ext;
```

To work around this without boxing, unsafe code can be used to create a safe projection using accessors. For example:

```rust
pub struct Extensions {
    interface1: *mut dyn Trait1,
    interface2: *mut dyn Trait2,
}

impl Extensions {
    pub fn new<'a>(
        interface1: &'a mut (dyn Trait1 + 'static),
        interface2: &'a mut (dyn Trait2 + 'static),
        scratch: &'a mut MaybeUninit<Self>,
    ) -> &'a mut Self {
        scratch.write(Self {
            interface1,
            interface2,
        })
    }

    pub fn interface1(&mut self) -> &mut dyn Trait1 {
        unsafe { self.interface1.as_mut().unwrap() }
    }

    pub fn interface2(&mut self) -> &mut dyn Trait2 {
        unsafe { self.interface2.as_mut().unwrap() }
    }
}

let mut scratch = MaybeUninit::uninit();
let ext: &mut Extensions = Extensions::new(&mut impl1, &mut impl2, &mut scratch);

// ext can now be casted to `&mut dyn Any` and back, and used safely
let ext: &mut dyn Any = ext;
```

## Context inheritance

Sometimes when futures poll other futures they want to provide their own `Waker` which requires creating their own `Context`. Unfortunately, polling sub-futures with a fresh `Context` means any properties on the original `Context` won't get propagated along to the sub-futures. To help with this, some additional methods are added to `ContextBuilder`.

Here's how to derive a new `Context` from another, overriding only the `Waker`:

```rust
let mut cx = ContextBuilder::from(parent_cx).waker(&new_waker).build();
```
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Apr 3, 2024
Rollup merge of rust-lang#123203 - jkarneges:context-ext, r=Amanieu

Add `Context::ext`

This change enables `Context` to carry arbitrary extension data via a single `&mut dyn Any` field.

```rust
#![feature(context_ext)]

impl Context {
    fn ext(&mut self) -> &mut dyn Any;
}

impl ContextBuilder {
    fn ext(self, data: &'a mut dyn Any) -> Self;

    fn from(cx: &'a mut Context<'_>) -> Self;
    fn waker(self, waker: &'a Waker) -> Self;
}
```

Basic usage:

```rust
struct MyExtensionData {
    executor_name: String,
}

let mut ext = MyExtensionData {
    executor_name: "foo".to_string(),
};

let mut cx = ContextBuilder::from_waker(&waker).ext(&mut ext).build();

if let Some(ext) = cx.ext().downcast_mut::<MyExtensionData>() {
    println!("{}", ext.executor_name);
}
```

Currently, `Context` only carries a `Waker`, but there is interest in having it carry other kinds of data. Examples include [LocalWaker](rust-lang#118959), [a reactor interface](rust-lang/libs-team#347), and [multiple arbitrary values by type](https://docs.rs/context-rs/latest/context_rs/). There is also a general practice in the ecosystem of sharing data between executors and futures via thread-locals or globals that would arguably be better shared via `Context`, if it were possible.

The `ext` field would provide a low friction (to stabilization) solution to enable experimentation. It would enable experimenting with what kinds of data we want to carry as well as with what data structures we may want to use to carry such data.

Dedicated fields for specific kinds of data could still be added directly on `Context` when we have sufficient experience or understanding about the problem they are solving, such as with `LocalWaker`. The `ext` field would be for data for which we don't have such experience or understanding, and that could be graduated to dedicated fields once proven.

Both the provider and consumer of the extension data must be aware of the concrete type behind the `Any`. This means it is not possible for the field to carry an abstract interface. However, the field can carry a concrete type which in turn carries an interface. There are different ways one can imagine an interface-carrying concrete type to work, hence the benefit of being able to experiment with such data structures.

## Passing interfaces

Interfaces can be placed in a concrete type, such as a struct, and then that type can be casted to `Any`. However, one gotcha is `Any` cannot contain non-static references. This means one cannot simply do:

```rust
struct Extensions<'a> {
    interface1: &'a mut dyn Trait1,
    interface2: &'a mut dyn Trait2,
}

let mut ext = Extensions {
    interface1: &mut impl1,
    interface2: &mut impl2,
};

let ext: &mut dyn Any = &mut ext;
```

To work around this without boxing, unsafe code can be used to create a safe projection using accessors. For example:

```rust
pub struct Extensions {
    interface1: *mut dyn Trait1,
    interface2: *mut dyn Trait2,
}

impl Extensions {
    pub fn new<'a>(
        interface1: &'a mut (dyn Trait1 + 'static),
        interface2: &'a mut (dyn Trait2 + 'static),
        scratch: &'a mut MaybeUninit<Self>,
    ) -> &'a mut Self {
        scratch.write(Self {
            interface1,
            interface2,
        })
    }

    pub fn interface1(&mut self) -> &mut dyn Trait1 {
        unsafe { self.interface1.as_mut().unwrap() }
    }

    pub fn interface2(&mut self) -> &mut dyn Trait2 {
        unsafe { self.interface2.as_mut().unwrap() }
    }
}

let mut scratch = MaybeUninit::uninit();
let ext: &mut Extensions = Extensions::new(&mut impl1, &mut impl2, &mut scratch);

// ext can now be casted to `&mut dyn Any` and back, and used safely
let ext: &mut dyn Any = ext;
```

## Context inheritance

Sometimes when futures poll other futures they want to provide their own `Waker` which requires creating their own `Context`. Unfortunately, polling sub-futures with a fresh `Context` means any properties on the original `Context` won't get propagated along to the sub-futures. To help with this, some additional methods are added to `ContextBuilder`.

Here's how to derive a new `Context` from another, overriding only the `Waker`:

```rust
let mut cx = ContextBuilder::from(parent_cx).waker(&new_waker).build();
```
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Apr 3, 2024
Add `Context::ext`

This change enables `Context` to carry arbitrary extension data via a single `&mut dyn Any` field.

```rust
#![feature(context_ext)]

impl Context {
    fn ext(&mut self) -> &mut dyn Any;
}

impl ContextBuilder {
    fn ext(self, data: &'a mut dyn Any) -> Self;

    fn from(cx: &'a mut Context<'_>) -> Self;
    fn waker(self, waker: &'a Waker) -> Self;
}
```

Basic usage:

```rust
struct MyExtensionData {
    executor_name: String,
}

let mut ext = MyExtensionData {
    executor_name: "foo".to_string(),
};

let mut cx = ContextBuilder::from_waker(&waker).ext(&mut ext).build();

if let Some(ext) = cx.ext().downcast_mut::<MyExtensionData>() {
    println!("{}", ext.executor_name);
}
```

Currently, `Context` only carries a `Waker`, but there is interest in having it carry other kinds of data. Examples include [LocalWaker](rust-lang/rust#118959), [a reactor interface](rust-lang/libs-team#347), and [multiple arbitrary values by type](https://docs.rs/context-rs/latest/context_rs/). There is also a general practice in the ecosystem of sharing data between executors and futures via thread-locals or globals that would arguably be better shared via `Context`, if it were possible.

The `ext` field would provide a low friction (to stabilization) solution to enable experimentation. It would enable experimenting with what kinds of data we want to carry as well as with what data structures we may want to use to carry such data.

Dedicated fields for specific kinds of data could still be added directly on `Context` when we have sufficient experience or understanding about the problem they are solving, such as with `LocalWaker`. The `ext` field would be for data for which we don't have such experience or understanding, and that could be graduated to dedicated fields once proven.

Both the provider and consumer of the extension data must be aware of the concrete type behind the `Any`. This means it is not possible for the field to carry an abstract interface. However, the field can carry a concrete type which in turn carries an interface. There are different ways one can imagine an interface-carrying concrete type to work, hence the benefit of being able to experiment with such data structures.

## Passing interfaces

Interfaces can be placed in a concrete type, such as a struct, and then that type can be casted to `Any`. However, one gotcha is `Any` cannot contain non-static references. This means one cannot simply do:

```rust
struct Extensions<'a> {
    interface1: &'a mut dyn Trait1,
    interface2: &'a mut dyn Trait2,
}

let mut ext = Extensions {
    interface1: &mut impl1,
    interface2: &mut impl2,
};

let ext: &mut dyn Any = &mut ext;
```

To work around this without boxing, unsafe code can be used to create a safe projection using accessors. For example:

```rust
pub struct Extensions {
    interface1: *mut dyn Trait1,
    interface2: *mut dyn Trait2,
}

impl Extensions {
    pub fn new<'a>(
        interface1: &'a mut (dyn Trait1 + 'static),
        interface2: &'a mut (dyn Trait2 + 'static),
        scratch: &'a mut MaybeUninit<Self>,
    ) -> &'a mut Self {
        scratch.write(Self {
            interface1,
            interface2,
        })
    }

    pub fn interface1(&mut self) -> &mut dyn Trait1 {
        unsafe { self.interface1.as_mut().unwrap() }
    }

    pub fn interface2(&mut self) -> &mut dyn Trait2 {
        unsafe { self.interface2.as_mut().unwrap() }
    }
}

let mut scratch = MaybeUninit::uninit();
let ext: &mut Extensions = Extensions::new(&mut impl1, &mut impl2, &mut scratch);

// ext can now be casted to `&mut dyn Any` and back, and used safely
let ext: &mut dyn Any = ext;
```

## Context inheritance

Sometimes when futures poll other futures they want to provide their own `Waker` which requires creating their own `Context`. Unfortunately, polling sub-futures with a fresh `Context` means any properties on the original `Context` won't get propagated along to the sub-futures. To help with this, some additional methods are added to `ContextBuilder`.

Here's how to derive a new `Context` from another, overriding only the `Waker`:

```rust
let mut cx = ContextBuilder::from(parent_cx).waker(&new_waker).build();
```
@traviscross traviscross added the WG-async Working group: Async & await label Apr 8, 2024
@raftario
Copy link

raftario commented Sep 9, 2024

It would be nice to start work towards stabilisation for this feature as it currently exists (an opt-in performance optimisation) and move the discussion about optional wakers to its own issue.

@tvallotton
Copy link
Contributor Author

tvallotton commented Oct 16, 2024

@raftario Do you know if there is any user of this feature in the current moment, or if there are crate authors waiting for this feature to be stabilized? I don't think stabilization can be justified without any users.

@raskyld
Copy link

raskyld commented Oct 19, 2024

While it's not an hard blocker for me at the time, I am really interested in this feature!

I am working on a runtime for wasm32-wasip2 which is, by essence, a single-threaded environment.
I have no use of Sync primitives (anyway, the implementation of the standard library for this target replace them with their single-threaded counterpart if I am right) but I am sometimes forced to use them because of APIs making the assumption everyone run in multi-threaded environment.

I am wrapping a LocalPool in my implementation but to respect the safety contract of the RawWaker, I need to make the data points to thread-safe values even though I know, wasm32-wasip2 is single-threaded so, by definition, there isn't "another" thread. Having LocalWaker solves the issue.

However, I am not sure I understand how you would build a local-only Context since, in the builder, you would need to provide a fallback Waker in from_waker and then set the LocalWaker. You would probably use a noop for the thread-safe Waker but use an actual RawWaker with single-threaded data for the LocalWaker. That's really weird but I guess it's fine since you expect the implementor of the Executor to know whether to use Context::waker or Context::local_waker right?

@raftario
Copy link

I'm also personally interested in this as I've been working on a thread-per-core runtime where tasks never leave the thread that spawned them, and it would be much easier if I didn't have to worry about task wakers being sent to other threads, even when none of the futures the runtime itself provides need it. I can also imagine some of the existing thread-per-core runtimes could use this feature quite extensively for similar reasons.

@kpreid
Copy link
Contributor

kpreid commented Oct 20, 2024

However, I am not sure I understand how you would build a local-only Context since, in the builder, you would need to provide a fallback Waker in from_waker and then set the LocalWaker. You would probably use a noop for the thread-safe Waker but use an actual RawWaker with single-threaded data for the LocalWaker. That's really weird but I guess it's fine since you expect the implementor of the Executor to know whether to use Context::waker or Context::local_waker right?

It’s not fine, because the implementor of the executor is not necessarily the implementor of all futures that need to obtain a waker; in particular, these types of futures will be broken:

  • future combinators which create their own wakers to more precisely handle polling (e.g. FuturesUnordered/FuturesOrdered/JoinAll) that wrap the provided waker
  • channel receiver futures
  • and, in general, any other leaf futures that are not specialized for the executor in question

In order for these situations to be detected instead of turning into lost wakeup bugs, you would have to write a Waker that actually panics when used, not a noop waker. And even then, you're locked out of using channels etc.

Personally, I think that LocalWaker isn't going to be useful (except as an optimization) until such time as there’s a way to create a Context that tells its users it has only a LocalWaker, or that the LocalWaker should be preferred (which, I suppose, could be done using a common helper library and the Context::ext() feature). Without one of those, there’s no way to tell all of the above types of futures that they need to switch to LocalWaker. I don't think it makes sense to stabilize LocalWaker until a solution to this problem is chosen, because the solution might affect what the Context/ContextBuilder methods related to LocalWaker look like.

@raskyld
Copy link

raskyld commented Oct 20, 2024

That's what I thought.. thanks for clarifying!

I share your opinion, we shouldn't stabilise the feature as it stands today. That also means, we would require current users of Context (and Future, which is kind of pervasive) to change their control flow to account for the possibility of a LocalWaker.

I think putting Send and Sync constraints on Waker was premature optimisation. Now, have an abstraction that costs us more overhead than it should. But I guess we are in a kind of dead-end..

@raftario
Copy link

I'm personally of the opposite opinion; I don't think this feature will ever have a path to stabilization as anything more than an opt-in optimisation.

Let's start with the proposition to make Context::waker return an Option<Waker> and assume it was realistic to update every single implementation of Future in the ecosystem to take it into account. Even then this approach would have very little advantage over just providing a panicking Waker.

Any future which can, at runtime, use either a Waker or a LocalWaker is going to be !Send + !Sync. So any such future has no use matching on the Option<Waker> and should go straight for the LocalWaker.

On the other hand, a future that does need the Send + Sync bounds - which in practice is the vast majority of futures in the ecosystem, since most people use Tokio and by virtue of having a work-stealing executor Tokio requires Send + Sync - also has little use for an Option<Waker>, because it will always fail on None. The only advantage over a panicking waker here is that the future implementation can propagate the error itself. I'd argue this is not worth it at all, because it introduces a branch in every future implementation and in practice most implementations would probably also just panic.

The other option is to add a generic parameter to Context. Adding a generic parameter to the Future trait has already been discussed in this RFC. Such a change would actually fully remove the need for this feature in the first place.

So unless I'm missing a secret third option this leaves us with the current API. Existing executors can either continue to provide only a Waker or add an opt-in LocalWaker as an optimisation. Existing futures which are Send + Sync continue using a Waker. Existing futures which are !Send + !Sync can migrate to LocalWaker.

New executors can choose to only provide a working LocalWaker and have their Waker panic. As @kpreid mentioned, this would make the executor incompatible with pretty much every Send + Sync future. I would personally argue that this is perfectly fine. There's a lot of precedent for this in the async ecosystem, with each runtime providing its own I/O futures which are incompatible with every other runtime. I'd argue this incompatibility would actually be a lot less annoying than the I/O situation since LocalWaker-only executors would most likely be targetted at WASM and embedded, and both ecosystems are already used to dealing with incompatibilities and have their own crates for a lot of stuff. Having specialised !Send + !Sync futures also makes a whole lot of sense when considering the implementation of things like channels become a whole lot simpler when they don't have to be thread-safe.

@tvallotton
Copy link
Contributor Author

I'm with @raftario here. The feature as it's stands can be used as a mere optimization, or as a replacement of waker. It is up to the crate author to decide how to use the feature, and both uses are valid.

LocalWaker only executors will always be incompatible with the rest of the ecosystem, regardless of what kind of API we offer. So I don't think we should worry too much about that use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. WG-async Working group: Async & await
Projects
None yet
Development

No branches or pull requests

6 participants