From 25e3945cb995d09063eb3a17a2ad333d9248e87e Mon Sep 17 00:00:00 2001 From: Without Boats Date: Fri, 31 Jan 2020 14:26:24 +0100 Subject: [PATCH 1/8] Add Wake trait for safe construction of Wakers. Currently, constructing a waker requires calling the unsafe `Waker::from_raw` API. This API requires the user to manually construct a vtable for the waker themself - which is both cumbersome and very error prone. This API would provide an ergonomic, straightforward and guaranteed memory-safe way of constructing a waker. It has been our longstanding intention that the `Waker` type essentially function as an `Arc`, with a `Wake` trait as defined here. Two considerations prevented the original API from being shipped as simply an `Arc`: - We want to support futures on embedded systems, which may not have an allocator, and in optimized executors for which this API may not be best-suited. Therefore, we have always explicitly supported the maximally-flexible (but also memory-unsafe) `RawWaker` API, and `Waker` has always lived in libcore. - Because `Waker` lives in libcore and `Arc` lives in liballoc, it has not been feasible to provide a constructor for `Waker` from `Arc`. Therefore, the Wake trait was left out of the initial version of the task waker API. However, as Rust 1.41, it is possible under the more flexible orphan rules to implement `From> for Waker where W: Wake` in liballoc. Therefore, we can now define this constructor even though `Waker` lives in libcore. This PR adds these APIs: - A `Wake` trait, which contains two methods - A required method `wake`, which is called by `Waker::wake` - A provided method `wake_by_ref`, which is called by `Waker::wake_by_ref` and which implementors can override if they can optimize this use case. - An implementation of `From> for Waker where W: Wake + Send + Sync + 'static` - A similar implementation of `From> for RawWaker`. --- src/liballoc/lib.rs | 1 + src/liballoc/task.rs | 91 ++++++++++++++++++++++++++++++++++++++++++++ src/libstd/lib.rs | 5 +++ 3 files changed, 97 insertions(+) create mode 100644 src/liballoc/task.rs diff --git a/src/liballoc/lib.rs b/src/liballoc/lib.rs index 38c6fa91cc55c..6c88280567aee 100644 --- a/src/liballoc/lib.rs +++ b/src/liballoc/lib.rs @@ -159,6 +159,7 @@ pub mod str; pub mod string; #[cfg(target_has_atomic = "ptr")] pub mod sync; +pub mod task; #[cfg(test)] mod tests; pub mod vec; diff --git a/src/liballoc/task.rs b/src/liballoc/task.rs new file mode 100644 index 0000000000000..8e0466d9a2ff5 --- /dev/null +++ b/src/liballoc/task.rs @@ -0,0 +1,91 @@ +#![unstable(feature = "wake_trait", issue = "0")] +//! Types and Traits for working with asynchronous tasks. +use core::mem; +use core::task::{Waker, RawWaker, RawWakerVTable}; + +use crate::sync::Arc; + +/// The implementation of waking a task on an executor. +/// +/// This trait can be used to create a [`Waker`]. An executor can define an +/// implementation of this trait, and use that to construct a Waker to pass +/// to the tasks that are executed on that executor. +/// +/// This trait is a memory-safe and ergonomic alternative to constructing a +/// [`RawWaker`]. It supports the common executor design in which the data +/// used to wake up a task is stored in an [`Arc`]. Some executors (especially +/// those for embedded systems) cannot use this API, which is way [`RawWaker`] +/// exists as an alternative for those systems. +#[unstable(feature = "wake_trait", issue = "0")] +pub trait Wake { + /// Wake this task. + #[unstable(feature = "wake_trait", issue = "0")] + fn wake(self: Arc); + + /// Wake this task without consuming the waker. + /// + /// If an executor supports a cheaper way to wake without consuming the + /// waker, it should override this method. By default, it clones the + /// [`Arc`] and calls `wake` on the clone. + #[unstable(feature = "wake_trait", issue = "0")] + fn wake_by_ref(self: &Arc) { + self.clone().wake(); + } +} + +#[unstable(feature = "wake_trait", issue = "0")] +impl From> for Waker { + fn from(waker: Arc) -> Waker { + unsafe { + Waker::from_raw(raw_waker(waker)) + } + } +} + +#[unstable(feature = "wake_trait", issue = "0")] +impl From> for RawWaker { + fn from(waker: Arc) -> RawWaker { + raw_waker(waker) + } +} + +// NB: This private function for constructing a RawWaker is used, rather than +// inlining this into the `From> for RawWaker` impl, to ensure that +// the safety of `From> for Waker` does not depend on the correct +// trait dispatch - instead both impls call this function directly and +// explicitly. +#[inline(always)] +fn raw_waker(waker: Arc) -> RawWaker { + + // Increment the reference count of the arc to clone it. + unsafe fn clone_waker(waker: *const ()) -> RawWaker { + let waker: Arc = Arc::from_raw(waker as *const W); + mem::forget(waker.clone()); + raw_waker(waker) + } + + // Wake by value, moving the Arc into the Wake::wake function + unsafe fn wake(waker: *const ()) { + let waker: Arc = Arc::from_raw(waker as *const W); + Wake::wake(waker); + } + + // Wake by reference, forgetting the Arc to avoid decrementing the reference count + unsafe fn wake_by_ref(waker: *const ()) { + let waker: Arc = Arc::from_raw(waker as *const W); + Wake::wake_by_ref(&waker); + mem::forget(waker); + } + + // Decrement the reference count of the Arc on drop + unsafe fn drop_waker(waker: *const ()) { + mem::drop(Arc::from_raw(waker as *const W)); + } + + RawWaker::new(Arc::into_raw(waker) as *const (), &RawWakerVTable::new( + clone_waker::, + wake::, + wake_by_ref::, + drop_waker::, + )) +} diff --git a/src/libstd/lib.rs b/src/libstd/lib.rs index bc07c6b487b17..e6f3536d46b54 100644 --- a/src/libstd/lib.rs +++ b/src/libstd/lib.rs @@ -469,9 +469,14 @@ pub mod time; #[stable(feature = "futures_api", since = "1.36.0")] pub mod task { //! Types and Traits for working with asynchronous tasks. + #[doc(inline)] #[stable(feature = "futures_api", since = "1.36.0")] pub use core::task::*; + + #[doc(inline)] + #[unstable(feature = "wake_trait", issue = "0")] + pub use alloc::task::*; } #[stable(feature = "futures_api", since = "1.36.0")] From e4fadd9b1d9af959ecfc8332086aaf65dd845f22 Mon Sep 17 00:00:00 2001 From: Without Boats Date: Fri, 31 Jan 2020 14:43:41 +0100 Subject: [PATCH 2/8] Add `wake_trait` feature directive to std --- src/libstd/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libstd/lib.rs b/src/libstd/lib.rs index e6f3536d46b54..25b0de31cdbd3 100644 --- a/src/libstd/lib.rs +++ b/src/libstd/lib.rs @@ -310,6 +310,7 @@ #![feature(unboxed_closures)] #![feature(untagged_unions)] #![feature(unwind_attributes)] +#![feature(wake_trait)] // NB: the above list is sorted to minimize merge conflicts. #![default_lib_allocator] From 911f32398ff441a0f9fea3329d2c570243a55cd9 Mon Sep 17 00:00:00 2001 From: Without Boats Date: Fri, 31 Jan 2020 17:01:41 +0100 Subject: [PATCH 3/8] Improve safety implementation, fix typos --- src/liballoc/task.rs | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/src/liballoc/task.rs b/src/liballoc/task.rs index 8e0466d9a2ff5..3fb148e8e34cb 100644 --- a/src/liballoc/task.rs +++ b/src/liballoc/task.rs @@ -1,12 +1,12 @@ #![unstable(feature = "wake_trait", issue = "0")] //! Types and Traits for working with asynchronous tasks. -use core::mem; -use core::task::{Waker, RawWaker, RawWakerVTable}; +use core::mem::{self, ManuallyDrop}; +use core::task::{RawWaker, RawWakerVTable, Waker}; use crate::sync::Arc; /// The implementation of waking a task on an executor. -/// +/// /// This trait can be used to create a [`Waker`]. An executor can define an /// implementation of this trait, and use that to construct a Waker to pass /// to the tasks that are executed on that executor. @@ -14,7 +14,7 @@ use crate::sync::Arc; /// This trait is a memory-safe and ergonomic alternative to constructing a /// [`RawWaker`]. It supports the common executor design in which the data /// used to wake up a task is stored in an [`Arc`]. Some executors (especially -/// those for embedded systems) cannot use this API, which is way [`RawWaker`] +/// those for embedded systems) cannot use this API, which is why [`RawWaker`] /// exists as an alternative for those systems. #[unstable(feature = "wake_trait", issue = "0")] pub trait Wake { @@ -36,9 +36,9 @@ pub trait Wake { #[unstable(feature = "wake_trait", issue = "0")] impl From> for Waker { fn from(waker: Arc) -> Waker { - unsafe { - Waker::from_raw(raw_waker(waker)) - } + // SAFETY: This is safe because raw_waker safely constructs + // a RawWaker from Arc. + unsafe { Waker::from_raw(raw_waker(waker)) } } } @@ -56,7 +56,6 @@ impl From> for RawWaker { // explicitly. #[inline(always)] fn raw_waker(waker: Arc) -> RawWaker { - // Increment the reference count of the arc to clone it. unsafe fn clone_waker(waker: *const ()) -> RawWaker { let waker: Arc = Arc::from_raw(waker as *const W); @@ -70,11 +69,10 @@ fn raw_waker(waker: Arc) -> RawWaker { Wake::wake(waker); } - // Wake by reference, forgetting the Arc to avoid decrementing the reference count + // Wake by reference, wrap the waker in ManuallyDrop to avoid dropping it unsafe fn wake_by_ref(waker: *const ()) { - let waker: Arc = Arc::from_raw(waker as *const W); + let waker: ManuallyDrop> = ManuallyDrop::new(Arc::from_raw(waker as *const W)); Wake::wake_by_ref(&waker); - mem::forget(waker); } // Decrement the reference count of the Arc on drop @@ -82,10 +80,8 @@ fn raw_waker(waker: Arc) -> RawWaker { mem::drop(Arc::from_raw(waker as *const W)); } - RawWaker::new(Arc::into_raw(waker) as *const (), &RawWakerVTable::new( - clone_waker::, - wake::, - wake_by_ref::, - drop_waker::, + RawWaker::new( + Arc::into_raw(waker) as *const (), + &RawWakerVTable::new(clone_waker::, wake::, wake_by_ref::, drop_waker::), )) } From 9beccfbcc9f6c0fa94e63fe40386ee59ad2b1d47 Mon Sep 17 00:00:00 2001 From: Without Boats Date: Fri, 31 Jan 2020 17:14:20 +0100 Subject: [PATCH 4/8] typo --- src/liballoc/task.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/liballoc/task.rs b/src/liballoc/task.rs index 3fb148e8e34cb..8da2989784d95 100644 --- a/src/liballoc/task.rs +++ b/src/liballoc/task.rs @@ -83,5 +83,5 @@ fn raw_waker(waker: Arc) -> RawWaker { RawWaker::new( Arc::into_raw(waker) as *const (), &RawWakerVTable::new(clone_waker::, wake::, wake_by_ref::, drop_waker::), - )) + ) } From dbe9d5df017dec812529bc3edcb3547e135148bd Mon Sep 17 00:00:00 2001 From: Without Boats Date: Sun, 2 Feb 2020 16:51:54 +0100 Subject: [PATCH 5/8] More explicit; CFG on atomic pointer --- src/liballoc/lib.rs | 1 + src/liballoc/task.rs | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/liballoc/lib.rs b/src/liballoc/lib.rs index 6c88280567aee..13b0dd977fd8b 100644 --- a/src/liballoc/lib.rs +++ b/src/liballoc/lib.rs @@ -159,6 +159,7 @@ pub mod str; pub mod string; #[cfg(target_has_atomic = "ptr")] pub mod sync; +#[cfg(target_has_atomic = "ptr")] pub mod task; #[cfg(test)] mod tests; diff --git a/src/liballoc/task.rs b/src/liballoc/task.rs index 8da2989784d95..3705ec9dcb6c9 100644 --- a/src/liballoc/task.rs +++ b/src/liballoc/task.rs @@ -59,20 +59,20 @@ fn raw_waker(waker: Arc) -> RawWaker { // Increment the reference count of the arc to clone it. unsafe fn clone_waker(waker: *const ()) -> RawWaker { let waker: Arc = Arc::from_raw(waker as *const W); - mem::forget(waker.clone()); + mem::forget(Arc::clone(&waker)); raw_waker(waker) } // Wake by value, moving the Arc into the Wake::wake function unsafe fn wake(waker: *const ()) { let waker: Arc = Arc::from_raw(waker as *const W); - Wake::wake(waker); + ::wake(waker); } // Wake by reference, wrap the waker in ManuallyDrop to avoid dropping it unsafe fn wake_by_ref(waker: *const ()) { let waker: ManuallyDrop> = ManuallyDrop::new(Arc::from_raw(waker as *const W)); - Wake::wake_by_ref(&waker); + ::wake_by_ref(&waker); } // Decrement the reference count of the Arc on drop From ac5ecdca3f927b9c61c7cad5fbe1e2d2168aa743 Mon Sep 17 00:00:00 2001 From: Saoirse Shipwreckt Date: Mon, 23 Mar 2020 01:35:01 +0100 Subject: [PATCH 6/8] Update src/libstd/lib.rs Co-Authored-By: Ashley Mannix --- src/libstd/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstd/lib.rs b/src/libstd/lib.rs index 25b0de31cdbd3..2416683c329db 100644 --- a/src/libstd/lib.rs +++ b/src/libstd/lib.rs @@ -476,7 +476,7 @@ pub mod task { pub use core::task::*; #[doc(inline)] - #[unstable(feature = "wake_trait", issue = "0")] + #[unstable(feature = "wake_trait", issue = "69912")] pub use alloc::task::*; } From a25a5c898e24d8bfd94654dc2ca2ba6b671ebe93 Mon Sep 17 00:00:00 2001 From: Saoirse Shipwreckt Date: Mon, 23 Mar 2020 01:35:15 +0100 Subject: [PATCH 7/8] Update src/liballoc/task.rs Co-Authored-By: Ashley Mannix --- src/liballoc/task.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/liballoc/task.rs b/src/liballoc/task.rs index 3705ec9dcb6c9..8cd21c108055c 100644 --- a/src/liballoc/task.rs +++ b/src/liballoc/task.rs @@ -1,4 +1,4 @@ -#![unstable(feature = "wake_trait", issue = "0")] +#![unstable(feature = "wake_trait", issue = "69912")] //! Types and Traits for working with asynchronous tasks. use core::mem::{self, ManuallyDrop}; use core::task::{RawWaker, RawWakerVTable, Waker}; From 732d71a976054806858a3c3669735e6301150014 Mon Sep 17 00:00:00 2001 From: Saoirse Shipwreckt Date: Mon, 23 Mar 2020 01:36:08 +0100 Subject: [PATCH 8/8] Apply suggestions from code review Co-Authored-By: Ashley Mannix --- src/liballoc/task.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/liballoc/task.rs b/src/liballoc/task.rs index 8cd21c108055c..981095302c730 100644 --- a/src/liballoc/task.rs +++ b/src/liballoc/task.rs @@ -16,10 +16,10 @@ use crate::sync::Arc; /// used to wake up a task is stored in an [`Arc`]. Some executors (especially /// those for embedded systems) cannot use this API, which is why [`RawWaker`] /// exists as an alternative for those systems. -#[unstable(feature = "wake_trait", issue = "0")] +#[unstable(feature = "wake_trait", issue = "69912")] pub trait Wake { /// Wake this task. - #[unstable(feature = "wake_trait", issue = "0")] + #[unstable(feature = "wake_trait", issue = "69912")] fn wake(self: Arc); /// Wake this task without consuming the waker. @@ -27,13 +27,13 @@ pub trait Wake { /// If an executor supports a cheaper way to wake without consuming the /// waker, it should override this method. By default, it clones the /// [`Arc`] and calls `wake` on the clone. - #[unstable(feature = "wake_trait", issue = "0")] + #[unstable(feature = "wake_trait", issue = "69912")] fn wake_by_ref(self: &Arc) { self.clone().wake(); } } -#[unstable(feature = "wake_trait", issue = "0")] +#[unstable(feature = "wake_trait", issue = "69912")] impl From> for Waker { fn from(waker: Arc) -> Waker { // SAFETY: This is safe because raw_waker safely constructs @@ -42,7 +42,7 @@ impl From> for Waker { } } -#[unstable(feature = "wake_trait", issue = "0")] +#[unstable(feature = "wake_trait", issue = "69912")] impl From> for RawWaker { fn from(waker: Arc) -> RawWaker { raw_waker(waker)