Move the descriptions of LocalWaker and Waker and the primary focus.

[Matthias247]

I started with adding more information around the LocalWaker and Waker structs. I kept the old names since most implementations of Futures at the end will store the thread safe wakers, and Waker sounded more natural than SendWaker for this.

There are two changes on API surface level compared to the current implementation:

• There is no more implicit impl From<LocalWaker> for Waker anymore. The reasoning was that some singlethreaded LocalWakers can not be converted into Wakers and trying to convert them leads to a panic. That seems ok an explicit method call, but not on an implicit conversion.
• I removed the will_wake_nonlocal/local functions, which would need to compare two RawWakers that are potentially different. I think just comparing the data pointer won't work here. Comparing data pointer and vtable exactly would work, but might not hit lots of trues on executors which change the vtable. We could add another method the vtable. Or leave things out. Is there currently a use-case for these methods?

I want to revisit the Wake section and rename things to ArcWake. This can then be a part of the RFC which can get either stabilized or not (the conversion can also live next to executor implementations). However I haven't gotten to this yet, and wanted to ask for feedback for the first parts in the meantime.

[Matthias247]

ArcWake is now also mentioned. It simply has one associated method for creating LocalWakers. The old local_waker_from_nonlocal would be gone. I think that's better, because ArcWake is only one possible way of creating a nonlocal waker.

[aturon]

This is fantastic -- way more clear and detailed. And the new APIs look solid!

I left a bunch of minor suggestions, mostly typographic.

[Matthias247]

Thanks for the review. I incorporated all suggestions.

[Nemo157]

This was an issue with the previous text here, but anyway:

Not all executors will provide an ability to "spawn", the common minimal functionality of an executor is just to "run" futures.

[Matthias247]

I thought spawning one future (like block_on does) is still spawning. But I can see how it is misleading. Updated

[Nemo157]

A task is a ()-producing Future

This was a point of contention back when 0.3 was being implemented, the final verdict was that the future is not the task, the task is a concept on top of the future that is executor specific and not directly exposed in any APIs (indirectly the APIs under core::task are used by the executor to communicate with the future that is at the root of the task).

[Matthias247]

Yes, it's true. The idea was to describe that a task is some kind of running version of a Future that is owned by the executor and accompanied by some extra state. Will try to make it clearer.

[Matthias247]

I updated the wording, and think it's better now. But I would also be fine with the one that MajorBreakfast added in
rust-lang/rust#52610

[Nemo157]

Maybe this should be something like

This mechanism allows implementers of an executor to customize the behavior of Waker and LocalWaker objects by providing an executor specific RawWaker.

Specifically noting that executors aren't directly customizing the [Local]Waker.

[Matthias247]

sounds good. I will change it.

[Nemo157]

The API update looks good, my waker implementation definitely appears more understandable with it.

[Matthias247]

My current opinion on this change after implementation is:

The RawWaker changes work out fine. It's nice to see the less clear trait object transmutes go away.

ArcWake itself is also fine, but I'm not yet sure about the best way for converting it into a LocalWaker after implementation. ArcWake::into_local_waker is one way, but it's not the one that is mostly in futures.rs. Instead the way through LocalWakerRef is utilized, which creates an internal LocalWaker/RawWaker which is slightly cheaper (2 atomic ops per poll).

If we see most implementations using this way, I think it should be in the same place as ArcWake itself. Currently the vtable implementations are doubled in liballoc and the LocalWakerRef implementation in futures.

The next open question is that LocalWakerRef can be constructed through local_waker_ref and local_waker_ref_from_nonlocal, which is a bit confusing (and potentially error-prone?). I understand the second one as a LocalWaker which still calls ArcWake::wake (instead of wake_local) immediately when constructed. And not only after it had been converted into a waker.
This one seems to be a mix of being universal (always safe), but also hard to understand where it actually should be used. I e.g. don't understand why it's used in futures-unordered or shared, where I thought the task is always polled from the same thread. Is it because futures-unordered could be called from within a thread pool?

Even if yes, wouldn't it then make more sense to simply implement the wake() and wake_local methods of ArcWake in the same thread-safe fashion, so that creating a normal LocalWaker would be good enough?

Having 2 different APIs for creating LocalWakers , and 2 additional ones for LocalWakerRefs from ArcWake sound a bit overwhelming for most users. The 2 could maybe merged into one by adding a boolean parameter and doing local_waker_ref(&Arc<ArcWake>, useWakeLocal: bool). But that still sounds a bit like doubling of concerns, since that the behavior can already be configured by implementing ArcWake in the required fashion. The only exception I can see is if for a single executor sometimes useWakeLocal: true and sometimes false is used, but that doesn't seem to be the case for any current executor.

[Matthias247]

We could only provide the way to create a LocalWakerRef from ArcWake, and from that one the LocalWaker could be cloned. That would create a single path instead of 2 variants to get the LocalWaker. But it's debatable whether the API is too discoverable. I thought about renaming LocalWakerRef to something like ArcWakeLocalWakerProvider or ArcLocalWakerProvider, but that hasn't totally convinced me yet either.

[withoutboats]

I think this looks like an improvement on the main RFC's waker APIs. After implementing romio I've been thinking about this API a bit more carefully and I think some naming tweaks would be an improvement.

First, I think it would be good for LocalWaker to be called Waker, and the waker that is Send and Sync to be called SyncWaker. This way, the "main" waker is the one that you see in the interface of Future, and we're also more consistent with the std default, which is to name the concurrent one alternatively and the noncurrent one the default name (e.g. Rc and Arc).

Second, I'm inclined to replace into_waker and try_into_waker with implementations of Into and TryInto. I'd like to hear counterarguments, but we have conversion traits and not using them seems a bit odd to me.

That makes the futures/reactor oriented API of wakers look like this:

struct Waker { }

impl Waker {
     pub fn wake(&self) { }
}

struct SyncWaker { }

impl SyncWaker {
    pub fn wake(&self) { }
}

impl Into<SyncWaker> for Waker { }
impl TryInto<SyncWaker> for Waker { }

impl Send for SyncWaker { }
impl Sync for SyncWaker { }

The API of future would be:

trait Future {
    type Output;
    fn poll(self: Pin<&mut Self>, waker: &Waker) -> Poll<Self::Output>;
}

The other side of this API consideration is how to handle constructing wakers, this is where things like the UnsafeWake trait come in. I actually really like the approach of creating a manual vtable proposed by this PR, because it leaves us a lot of leeway to deprecate it and replace it with something better in the future (such as when raw pointer methods become object safe). I'm 👍 on that part of the API with the understanding we'll deprecate it someday.

[Matthias247]

@withoutboats Thanks for your feedback!

I agree that SyncWaker/Waker actually describe their purposes better, and @aturon also liked that direction. I had two reasons for not changing it:

• Most futures will want to store and interact with the thread-safe version of the waker, and Waker is more concise here.
• There is less churn for changing all existing code.
  However I don't really feel strongly about it, either way if fine.

Regarding From/TryFrom: I opted not to add From, since creating a thread-safe waker from a non thread-safe one can panic in case the eventloop doesn't support remote wakeup. I felt like a panic should be ok in an explicit conversion, but feels weird to come up in an implicit one (that some users might not even see).
It's probably not really a big deal, since that panic will be very direct for users and tell them their overall application configuration is wrong, due to trying to user futures which require multithreading support with an event loop that doesn't support it. I'm open for both.

I agree on the API being extensible regarding other waker implementations. We could later on store an enum { RawWaker(w), WhateverOtherWaker(w), etc. } inside the [Local/Sync]Waker and dispatch on that additionally. Or offer functions which convert newer mechanisms into RawWakers, which has probably less overhead.

The thing I'm currently mostly concerned about is ArcWake, as described above. I think my recommendation would be to leave it out of std and stabilization for now, if there's not a lot of feedback around it. It can be added again later, and isn't required for interoperability.

[withoutboats]

Most futures will want to store and interact with the thread-safe version of the waker, and Waker is more concise here.

This was actually not my experience porting tokio's reactor, which uses an AtomicWaker to wrap a LocalWaker and never directly constructs a Waker. Doing the port was confusing for me at first - I had to figure out that tokio's AtomicTask was analogous to AtomicWaker before I figured out that I actually didn't want to convert to a Waker somewhere close to the boundary. I think most functions the waker passes through will actually not want to perform the conversion, and so they'll be talking about the localwaker. In other words, I think that the opposite of this is true, even though maybe most reactors will want to interact with the syncwaker at the ultimate leaves.

I think my recommendation would be to leave it out of std and stabilization for now, if there's not a lot of feedback around it. It can be added again later, and isn't required for interoperability.

Agreed, I think we should just stick with the RawWaker type for now and leave better abstractions for the future or the external ecosystem to experiment with.

[Matthias247]

This was actually not my experience porting tokio's reactor, which uses an AtomicWaker to wrap a LocalWaker and never directly constructs a Waker.

Ok, some multithreaded future implementations totally uses AtomicWrapper which wraps the thread-safe ([Sync]Waker) waker variant. User Futures do store those directly, because e.g. they might use mutexes instead of atomics for coordination.
In general I think we have seen the usage from a different view-point:

• I looked at the implementation of handwritten Futures, where the thread-safe variant might be more common (if AtomicWaker isn't used).
• You also looked around the user code, which mostly needs forward the [Local]Waker reference through all levels.

I guess there might be more of this user-code around in the end than low level library and framework implementations. So using the more concise variant there makes sense.

[Nemo157]

This was actually not my experience porting tokio's reactor, which uses an AtomicWaker to wrap a LocalWaker and never directly constructs a Waker

AtomicWaker wraps a Waker, it just takes a LocalWaker in on it’s public API so that it can avoid converting it into a waker on the very rare case where it is being woken at the same time the task is changed.

I agree with the sentiment of your post though, only leaf futures and some special combinators like FuturesUnordered will actually interact with SyncWaker. But also, most intermediate futures should be written with async in the future which completely hides this argument from the user.

[Nemo157]

Another question I've had with this API is whether anyone is using/has plans to use LocalWaker's capabilities. futures-rs appears to use ArcWake everywhere, I didn't even consider it when implementing my executor because places that will actually call it seem rare enough—and the optimization small enough—to not justify the extra maintenance burden.

I am tempted to try benchmarking the difference now though, on thumbv6m making SyncWaker safe requires a critical section because of the lack of atomics, so it might be a noticeable optimization. That said, I still feel like the actual use of it will be rare enough that the optimization isn't worth it.

[withoutboats]

@Nemo157 I know AtomicWaker converts the localwaker to a waker internally, but the implication is that romio never uses the Waker type at all, only AtomicWaker and LocalWaker.

That said, I still feel like the actual use of it will be rare enough that the optimization isn't worth it.

I would be thrilled if empirically this use case was not worth supporting because it would allow us to massively simplify the API, but @cramertj argued before that the optimization is actually really critical for single threaded executors.

[Nemo157]

the optimization is actually really critical for single threaded executors.

I guess this is not just any single threaded executor, but a single threaded executor coupled with a reactor in the same thread that knows it can store LocalWakers instead of SyncWakers for its own futures (which is why futures::executor::LocalPool can't use this optimization, it still needs to be coupled with an off-thread reactor, or other IO notification -> waker system, to be able to actually perform IO).

[Centril]

More nits.

[Centril]

This is an unsafe fn pointer; but what are the invariants the function requires from its callers? It should be clearly specified in terms of "Behavior is undefined if X.". The same applies to the subsequent 3 unsafe function pointers.

[gereeter]

Specifically, it seems to me that the requirement should be that callers must pass the data pointer of the RawWaker that this table was created as a part of. Behavior is undefined if any other pointer is passed.

I don't see any other source of undefined behavior - aside from taking an over-broad type (*const ()), these functions should be perfectly safe.

[Matthias247]

I appreciate the changes around ArcWake, this makes things much clearer. If I understand correctly, an ArcWake-based RawWaker's data points to the contents of an Arc<dyn ArcWake> (since that's basically what it is in disguise), but its vtable.wake function reconstructs an actual Arc<T> on the stack and then passes a reference to that into <T as ArcWake>::wake. Is that accurate? (It makes me wish we had something like C++'s enable_shared_from_this so ArcWake impls could use clone without that dance...)

It's actually more of an Arc<impl ArcWake>, and data points to whatever Arc::from_raw and the other way around return (it's more like the address of the Arc itself and not of the T inside it). But in general that's an implementation detail, and it had been done in this way to make the implementation easy and avoid having 2 levels of dynamic dispatch. You can see how it can be built in this CR: https://github.com/rust-lang-nursery/futures-rs/pull/1340/files#diff-1ededb833a5fe62f0ee3a38329a54d6f
The implementation there is a little bit more tricky than the minimum required thing because it also powers LocalWakerRef and thereby an additional level of "upgrades" between Wakers.

I'll also throw this out again (from rust-lang#2592 (comment)): I think there might be a better and more noun-y name than Waker- maybe Task/task::Handle/etc, similar to 0.1. It feels like this is more direct and reduces the number of terms flying around. Or do we want to reserve the name Task for internal types on the executor side?

The equivalent in C++ is coroutine_handle. Therefore yes, things along TaskHandle, SyncTaskHandle or LocalTaskHandle would also make sense. I'm not too sure about task::Handle, would need to see some code with it. Sounds like it could lead more likely to same naming collisions, but that also depends on what module level we expect users to import in the end. One thing to consider about Task is also that it's some kind of phantom construct. It is there as a concept, but not really as a type (apart from in concrete executor implementations).

Although there is another open issue from the original stabilization CR for finding a good top level module name for all of this infrastructure, and task or tasks might be one of the better ones since async is not available.

[rpjohnst]

Or do we want to reserve the name Task for internal types on the executor side?

One thing to consider about Task is also that it's some kind of phantom construct. It is there as a concept, but not really as a type (apart from in concrete executor implementations).

Right- and another thing that comes to mind now is that Task might be a confusing name for users. Future is the trait they should be working with to represent async values/computations, but C# uses the name Task for the analogous user-facing type. So something with "handle" might also make sense for that reason.

[Matthias247]

Right- and another thing that comes to mind now is that Task might be a confusing name for users. Future is the trait they should be working with to represent async values/computations, but C# uses the name Task for the analogous user-facing type. So something with "handle" might also make sense for that reason.

I thought a bit more about Handles. An hour ago I really liked it - better than the Wakers!
In between I have found one reason why I think it might not be preferable:
Handle implies some kind of identity for me. If I have a Handle for a task, I might expect that it uniquely identifies the task, and I can compare Handles, etc.

However that's not really possible with the current Wakers. It's perfectly legal to build a Waker which just wakes up an executor, and that executor polls all of it's tasks again. There would be no reference to the associated task stored at all inside the Waker. And in fact things like the executor on which block_on is based are doing this.

We could argue that Handles in WinApi & Co also only have a weak identity since they are reused, but I think at least for their normal usage scope (until they are closed) their identity is stronger than what some Wakers provide.

[Matthias247]

Here is a summary of open discussion points that I gathered from this thread and the original stabilization PR:

• Naming of Wakers:
  We have the following options:
  • Stay with Waker and LocalWaker
  • Move to SyncWaker and Waker
  • Something else (e.g. Handle, TaskHandle, etc)
    I think all are fine and fairly descriptive. We should just choose for one. If nobody is concerned about churn in renaming things (@cramertj, @aturon, @Nemo157 ?) then SyncWaker and Waker might be closest to other naming conventions. If we go with these names the methods on ArcWake might also be required to be updated.
• Should LocalWakers be convertible into Wakers via conversion traits (e.g. From/TryFrom)?
  It's the idiomatic way to do conversions in Rust. However the conversion can panic for some waker types, and panics on implicit conversions might not be desired. Only implementing TryFrom does't make sense, 99.9% of all applications wouldn't face any conversion errors and adding .unwrap() everywhere just adds noise and is not better ergonomically than .into_waker().
• Module organization:
  We currently have parts in core/std::task, and others in core/std::future, and for the pure std parts also things alloc.
  There was a strong request for a common top level module (at least for the core parts), where documentation can be moved. async is the most obvious, but not allowed.
  My recommendation would be task (or tasks), since it contains the building blocks for creating and running lightweight tasks.
  The building blocks are Wakers, Futures (as intermediate results of running those tasks), and the types that are required by them (e.g. Poll and RawWaker). I think those should be all directly within task. If at a later point more async compatible types are added, those could live in submodules like task::channel.
  The reasoning for having Future outside of task was that it might only be one async primitive, and others like Stream are equivalent. I however rather think that those should be redefined on top of Future (Redefine Stream/Sink/AsyncRead/AsyncWrite/etc on top of Future rust-lang/futures-rs#1365), since it's possible to do more kinds of things with Futures than Streams.
• Discussions around ArcWake:
  • Include it into std at all or not? If we include it, with the associated conversion functions to Wakers or not?
  • Should the methods to create LocalWaker and Waker be on the trait itself, freestanding methods or on an extension trait. The latter two might have more discoverability issues, but also let the trait be more compact, and might leave more room for other conversion functions in the future if the find the currently defined ones not ideal.
  • The RFC currently proposes one into_local_waker() function, which directly creates a LocalWaker, where calling .wake() will call into it's wake_local() method.
    However the equivalent conversion methods in futures-rs has 2 methods:
    • One that creates a LocalWaker, where calling .wake() will call wake() on the ArcWake. This method is safe.
    • Another one that creates a LocalWaker, where calling .wake() will call wake_local() on the ArcWake. This method is defined as unsafe. I guess for the reasoning that this kind of Waker could be constructed in any code-path which has access to the ArcWake, which could be in any kind of thread, including ones that are not "local" to the associated executor. Strictly speaking this is more correct, even though it's unlikely that executor creators would use the functionality from non-executor threads.
      Having 2 methods raises naming questions (e.g. unsafe fn into_local_waker() -> LocalWaker and fn into_local_waker_with_nonlocal_wake() -> LocalWaker, or with the other naming fn into_waker_with_sync_wake() -> Waker and unsafe fn into_waker() -> Waker), and the associated discoverability and learnability questions.
      Things would definitely get easier if ArcWake would just contain a sync wake() method, but that leaves some performance on the table (although executors could still implement custom ArcWake -> LocalWaker conversion functions if they need that performance).
      I have some suspicion that splitting ArcWake into two traits (one with wake() and another one that inherits from the first and adds wake_local()) might clean things more up by at least only providing safe functions for ArcWake, but I haven't looked at it in detail.
  • futures-rs mostly doesn't create directly LocalWakers from ArcWake, but uses an intermediate structure called LocalWakerRef, that dereferences into a LocalWaker, but is a bit more efficient due to avoiding 2 atomic operations for running each task. Since many executors that make use of ArcWake might want to use this behavior, it raises the question whether it also should be included. However on the negative side, this adds more hard to understand APIs (LocalWakerRef and up to two functions to create it, similar to the ones for creating a general LocalWaker from an ArcWake as discussed above).

[Thomasdezeeuw]

Should this function own the data (*const ()) and return Option<Waker> instead? This way if there are two different waking mechanisms for local and sync wakers it would be supported. Furthmore returning a Waker, rather then RawWaker, makes it clear that all the requirements of the Waker type must be met.

Edit: first part was incorrect, as @Nemo157 pointed out.

[Nemo157]

@Thomasdezeeuw if there are different implementations then the returned RawWaker can contain different function pointers.

[Thomasdezeeuw]

@Nemo157 Right scratch the first part, I had in my head that data was part of Waker, not RawWaker. What are you thoughts about the second part?

[Nemo157]

I could go either way. I like the fact that when writing the RawWaker type currently you don’t need to import any of the concrete types it will be wrapped in, and think that the documentation is clear enough (and the entire area unsafe enough) that implementors will be careful about what they’re doing. But having to call Waker::new_unchecked doesn’t seem like a massive burden (although, my current implementation of RawWaker is 100% safe code, this would require adding an unsafe block to it EDIT: Scratch that, I just realised I have a cast that needs to be a pointer re-reference instead which needs unsafe).

[Matthias247]

I see it the same as @Nemo157: I like the separation of Api concerns.
There should also be no extra unsafe call involved, since the new_unchecked from the result would happen in LocalWaker::into_waker()

[withoutboats]

I'd like to make a proposal building off of this: we leave the RawWaker stuff unstable for now, and provide these additional components.

First, a core "Wake" trait that we expect to work for all use cases:

trait Wake {
    fn wake(&self);
}

Second, these three construction APIs for Waker and these two for SyncWaker:

• for<T: Wake + 'static> &'static T -> Waker
• for<T: Wake + 'static> Rc<T> -> Waker
• for<T: Wake + 'static> Arc<T> -> Waker
• for<T: Wake + Send + Sync + 'static> &'static T -> SyncWaker
• for<T: Wake + Send + Sync + 'static> Arc<T> -> SyncWaker

My understanding (which could be badly mistaken!) is that this should cover the concrete use cases today, which I believe use Arcs, Rcs, or global statics as their ownership model for wakers.

We can leave the RawWaker stuff, but unstable, for people experimenting with custom shared ownership implementations. Someday we could stabilize RawWaker, or possibly a different, better factored abstraction: I've started iterating on this concept generically in an external crate called smart.

Assuming this properly covers the different use cases, I think this API surface would be dramatically simpler to understand immediately than the RawWaker stuff is.

[cramertj]

@withoutboats AFAICT the Wake trait you suggested is useless for any real-world use of these types. There's a reason you need the &Arc<Self> parameter, and for the lower-level uses like LocalWakerRef (which are required for reasonable performance) we need RawWaker (or add LocalWakerRef to std, which seems like an odd fight to pick).

Unless there are any uses of this API that won't work with RawWaker or cannot performantly implement the RawWaker API, I think it's critical that we stabilize it as part of the first pass.

[withoutboats]

@cramertj can you link to concrete implementations of waker so that I can understand them better? Perhaps the one in fuchsia? Neither of the implementations in futures (which I know are not optimal) rely on the Arc, and @Nemo157 described a waker implemented using an AtomicBool that this would be satisfactory for.

EDIT: Its also hard to translate the code exactly, but tokio-executor's Unpark trait uses a method with an &self signature, which tokio-threadpool uses in a way that afaict never involves cloning an arc.

[cramertj]

The AtomicBool thing is just a toy and is useful for writing tests-- it doesn't actually queue any work. The fuchsia implementation is here, and I'd recommend taking a look at the async book example which uses the same strategy.

[Matthias247]

I agree with @cramertj here. First of all we need Arc<Self>, and apart from that we would lose all "local" optimizations that are already used through various places. If we don't stabilize those things, then everyone who wants to benefit from those optimizations has to use nightly again (which brings us further away from stabilization).

Besides that I think RawWaker doesn't seem to be a discussion point anymore - at least I don't have it on my list above. It seems to cover all known use-cases. Even if we find something that isn't covered we could add new alternative mechanisms to create the "real" [Local]Waker types.

I'm also not concerned about the low-levelness of RawWaker, since the amount of people who need to implement it is minimal, and things like ArcWake can provide tools for easier creating RawWakers. Those tools can be inside or outside of std.

[Matthias247]

Here are some ideas around ArcWake. The following namings here are based on the proposals to rename LocalWaker/Waker to Waker/SendWaker:

If we split the trait into a version which is safe (wake() is implemented in a fashion where it can be safely called from any thread), and one which is less safe but more optimized (something along wake_local(), which will be called from the thread which creates the initial waker), things seem to get a bit more clear.
We would get an API along the following:

pub trait ArcWake: Send + Sync {
    fn wake(arc_self: &Arc<Self>);
    fn into_waker(wake: Arc<Self>) -> Waker where Self: Sized;
    fn into_waker_ref(wake: &Arc<Self>) -> WakerRef<'_> where Self: Sized;
}

pub trait ArcWakeLocal: ArcWake {
    unsafe fn wake_from_local(arc_self: &Arc<Self>);
    unsafe fn into_waker_with_local_opt(wake: Arc<Self>) -> Waker where Self: Sized;
    unsafe fn into_waker_ref_with_local_opt(wake: &Arc<Self>) -> WakerRef<'_> where Self: Sized;
}

Users that want to hack together an executor in the simplest possible fashion can use ArcWake and it's associated methods, which is completely safe. Someone who wants to make use of an optimized wakeup implementation in the case where wake() is called before the Waker is converted into a SyncWaker can implement ArcWakeLocal. The functions here are all are unsafe because for the same reasons the current equivalents in futures-rs are unsafe: If someone would create a waker that way from the non-executor thread, and pass it to a future, and that future would call wake() (which leads to wake_from_local), the requirement that wake_from_local might only be called from the executor thread is invalidated. I'm not 100% sure if all of those methods need to be unsafe, but that's maybe open for discussion (Creating a waker that way actually isn't unsafe - passing it to futures in certain codepath might be).

Names for ArcWake feel clumsy. If we follow the Waker/SendWaker scheme the traits should actually be

pub trait ArcWake: ArcSendWake { unsafe fn wake() };
pub trait ArcSendWake { fn wake_send() }

in order to avoid asymetry. But implementing a wake_send() function sounds weird too. Maybe it could be wake_from_anywhere(), but wake() without parameters already implies that.

That said I like the renaming from LocalWakerRef to WakerRef, even though it's still not clear without extensive documentation what it actually is (a type that can be passed when a local &Waker is required, e.g. into poll, but which is cheaper to create from the Arc).
I thought about alternatives here and came up with WakerBuilder, WakerProxy, WakerFactory, VirtualWaker, LazyWaker and WakerPromise.

Out of those I found LazyWaker or LazyArcWaker or the existing [Arc]WakerRef best.

[Centril]

As for discussion items open, in #15 (comment) I noted that the unsafe fn function pointers haven't specified their invariants in documentation; If we keep them around, I think that should be done clearly and as formally as possible in terms of "Behavior is undefined if X, Y, Z".

[Matthias247]

@Centril I've seen the comment, but don't know exactly what you want to be added. It is described how the API should behave. And everything else is undefined behavior (which is how C-like APIs usually work). Do you have any special suggestions? I'm open for any improvements there. Feel free to directly add them to the review.

[gereeter]

I may be missing something, but is there a good reason why Wake couldn't take &self if it was implemented for Arc<Task> instead of Task? Naturally, this would preclude aggressive blanket impls, but that doesn't seem too bad. That is, the Fuchsia implementation would be

impl task::Wake for Arc<Task> {
    fn wake(arc_self: &Self) {
        arc_self.executor.ready_tasks.push(arc_self.clone());
        arc_self.executor.notify_task_ready();
    }
}

Then, the implementation of LocalWakerRef would use Referenced<W> and NonlocalReferenced<W> instead of ReferencedArc<W> and NonlocalReferencedArc<W> and would either require W: Clone or (preferably, in my opinion) a method on wake such as clone_waker(&self) -> Waker:

Implementing `LocalWakerRef` with `Wake` taking `&self`.

// This is a port of the code before the changes in this RFC, since I wanted it to be most easily
// comparable to what already exists and I'm not confident about exactly how `LocalWakerRef`
// fits into the new system.

#[derive(Debug)]
pub struct LocalWakerRef<'a> {
    local_waker: LocalWaker,
    _marker: PhantomData<&'a ()>,
}

impl<'a> LocalWakerRef<'a> {
    pub fn new(local_waker: LocalWaker) -> Self {
        LocalWakerRef {
            local_waker,
            _marker: PhantomData,
        }
    }
}

pub unsafe fn local_waker_ref<W>(wake: &W) -> LocalWakerRef<'_>
where
     W: Wake + Clone /* or clone_waker on Wake */ + 'static,
{
    let ptr = wake
        as *const W
        as *const Referenced<W>
        as *const dyn UnsafeWake
        as *mut dyn UnsafeWake;
    let local_waker = LocalWaker::new(NonNull::new_unchecked(ptr));
    LocalWakerRef::new(local_waker)
}

// Pointers to this type below are really pointers to `W`
struct Referenced<W> {
    _marker: PhantomData<W>
}

unsafe impl<W: Wake + Clone /* or clone_waker on Wake */ + 'static> UnsafeWake for Referenced<W> {
    #[inline]
    unsafe fn clone_raw(&self) -> Waker {
        let me = self as *const Referenced<W> as *const W;
        W::clone(&*me).into() // or W::clone_waker(&*me)
    }

    #[inline]
    unsafe fn drop_raw(&self) {}

    #[inline]
    unsafe fn wake(&self) {
        let me = self as *const Referenced<W> as *const W;
        W::wake(&*me)
    }

    #[inline]
    unsafe fn wake_local(&self) {
        let me = self as *const ReferencedArc<W> as *const W;
        W::wake_local(&*me)
    }
}

// And analogously for NonlocalReferenced<W>:

// Pointers to this type below are really pointers to `W`
struct Referenced<W> {
    _marker: PhantomData<W>
}

unsafe impl<W: Wake + Clone /* or clone_waker on Wake */ + 'static> UnsafeWake for NonlocalReferenced<W> {
    #[inline]
    unsafe fn clone_raw(&self) -> Waker {
        let me = self as *const NonlocalReferenced<W> as *const W;
        W::clone(&*me).into() // or W::clone_waker(&*me)
    }

    #[inline]
    unsafe fn drop_raw(&self) {}

    #[inline]
    unsafe fn wake(&self) {
        let me = self as *const NonlocalReferenced<W> as *const W;
        W::wake(&*me)
    }

    #[inline]
    unsafe fn wake_local(&self) {
        let me = self as *const NonlocalReferencedArc<W> as *const W;
        W::wake(&*me)
    }
}

#[inline]
pub fn local_waker_ref_from_nonlocal<W>(wake: &W) -> LocalWakerRef<'_>
where
    W: Wake + Clone /* or clone_waker on Wake */ + 'static,
{
    let ptr = wake
        as *const W
        as *const NonlocalReferenced<W>
        as *const dyn UnsafeWake
        as *mut dyn UnsafeWake;
    let local_waker = unsafe { LocalWaker::new(NonNull::new_unchecked(ptr)) };
    LocalWakerRef::new(local_waker)
}

I not sure this would be that much better from a documentation standpoint (as no blanket impls means that you still have to think about Arc), but it is significantly more flexible. In particular, I could see

• Using Rc to avoid overhead in a completely single-threaded program (this would likely need some other shenanigans, but should be possible)
• Using a version of Arc that is slimmed down to have, e.g., no weak count and a strong count stored in a smaller integer type.
• Using some ArcInner type that is the dereferenced part of an Arc (i.e. &ArcInner<T> stores the same pointer as Arc<T> and can be upgraded to Arc<T>, but does not actually require incrementing the reference count, like &Arc<T>) to avoid the extra layer of indirection. This is the reason I prefer a clone_waker method to just requiring Clone.

Admittedly, these could all be implemented unsafely, but it seems useful to have the safe interface be simpler, not rely on a particular fixed structure in the standard library, and be more flexible (including supporting no_std usecases).

[gereeter]

Specifically, it seems to me that the requirement should be that callers must pass the data pointer of the RawWaker that this table was created as a part of. Behavior is undefined if any other pointer is passed.

I don't see any other source of undefined behavior - aside from taking an over-broad type (*const ()), these functions should be perfectly safe.

[gereeter]

Although it is clearly stated in the documentation, it would still probably be good to include the core difference between Waker and LocalWaker (the Send and Sync implementations) in the code snippet.

Suggested change

	+unsafe impl Send for Waker { }
	+unsafe impl Sync for Waker { }

[gereeter]

While I agree that in practice basically every Waker should clean up after itself, I do think calling it a requirement gives the wrong impression. This implies (to me) that a lack of resource leaks is a safety property that code can rely on for memory safety. Along the same lines that std::mem::forget is safe but not recommended, I think that cleaning up can be mentioned, but should not be a requirement.

[gnzlbg]

So how do we wrap with Futures asynchronous APIs that do not have a way to inform the execution about this without adding overhead?

[cramertj]

I may be missing something, but is there a good reason why Wake couldn't take &self if it was implemented for Arc instead of Task?

This doesn't work unless you either double-allocate or force Wake to be an unsafe trait that can only be implemented by pointer-sized objects so that they may fit in a Waker/LocalWaker.

[withoutboats]

I may have missed this in the discussion, but its not in the RFC. I believed the intent was that an API would exist that would convert any type that implements ArcWake to a LocalWaker - which would be the purpose of that higher level function. However, I do not personally know how to make that bridge and construct a &'static RawWakerVTable value without a feature that does not currently exist, like generic statics. Can anyone clarify the intended bridge here?

[Matthias247]

@withoutboats It just works :) See this CR in the local_waker_ref file: https://github.com/rust-lang-nursery/futures-rs/pull/1340/files#diff-1ededb833a5fe62f0ee3a38329a54d6fR47

To be fair: When I implemented it I also wasn't sure whether it works, and wanted to build a generic static vtable first - which the compiler didn't accept. However it seems like static constant locals that are defined by a macro are properly accepted as static globals, which worked around this.

[withoutboats]

@Matthias247 thanks, that makes sense.

(To be specific so others dont have to look at the diff, you can use static promotion to make this work even though you can't use real statics.)

[withoutboats]

I'm going to merge this into the main PR since it reflects the current status of the proposal. Any additional changes can be made in new PRs against the main RFC just like this was done.

[Matthias247]

Thanks! Let's continue the discussion in the original place