Skip to content

Add Wake trait for safe construction of Wakers. #68700

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Mar 23, 2020
Merged

Add Wake trait for safe construction of Wakers. #68700

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
06ede35
Add Wake trait for safe construction of Wakers.
withoutboats Jan 31, 2020
d8a835f
Add `wake_trait` feature directive to std
withoutboats Jan 31, 2020
c9acdb0
Improve safety implementation, fix typos
withoutboats Jan 31, 2020
ede03a4
typo
withoutboats Jan 31, 2020
3ae74ca
More explicit; CFG on atomic pointer
withoutboats Feb 2, 2020
a4875a7
Update src/libstd/lib.rs
withoutboats Mar 23, 2020
caff9f9
Update src/liballoc/task.rs
withoutboats Mar 23, 2020
32f5724
Apply suggestions from code review
withoutboats Mar 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Apply suggestions from code review 
Co-Authored-By: Ashley Mannix <ashleymannix@live.com.au>
  • Loading branch information
@withoutboats @KodrAus
withoutboats and KodrAus committed Mar 23, 2020
commit 32f5724e8ac35e5a314313c6053ff46702223b27
10 changes: 5 additions & 5 deletions src/liballoc/task.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,24 +16,24 @@ 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<Self>);

/// 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")]
#[unstable(feature = "wake_trait", issue = "69912")]
fn wake_by_ref(self: &Arc<Self>) {
self.clone().wake();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

futures::task::ArcWake has the opposite defaulted method. Is there a reason to assume that implementations would commonly need to own the Arc instead of being able to wake through a shared reference?

Copy link
Contributor Author

@withoutboats withoutboats Jan 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my experience working on romio/juliex and reading the tokio source, the standard executor model is to re-enqueue the task on some TLS or global queue (which means it must have ownership), and the standard reactor model is to store an Option<Waker> and then call .take().unwrap().wake(). IMO the default in futures is backwards, and would result in naive implementations making an additional unnecessary ref count increment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion about this either way-- I can definitely imagine arguments on either side. The reason I did it the other way in futures-rs was that I imagined implementations which only needed by-reference waking would implement just wake not realizing that that would result in a wake_by_ref method which cloned unnecessarily. In practice, I don't think either case is too big of an issue.

Copy link
Contributor Author

@withoutboats withoutboats Feb 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, either way I think its a chance of an incorrectly implemented executor leading to one extra incr/decr on wakeup - a trivial and easily fixable problem.

But I do think the more likely scenario is that they do need ownership of the arc, rather than that they don't (the only point of it being by arc is to cheaply enqueue on wake up after all), and I thought our choice to name the fns wake and wake_by_ref was tied up in this assumption (that wake would be the default one).

}
}

#[unstable(feature = "wake_trait", issue = "0")]
#[unstable(feature = "wake_trait", issue = "69912")]
impl<W: Wake + Send + Sync + 'static> From<Arc<W>> for Waker {
fn from(waker: Arc<W>) -> Waker {
// SAFETY: This is safe because raw_waker safely constructs
Expand All @@ -42,7 +42,7 @@ impl<W: Wake + Send + Sync + 'static> From<Arc<W>> for Waker {
}
}

#[unstable(feature = "wake_trait", issue = "0")]
#[unstable(feature = "wake_trait", issue = "69912")]
impl<W: Wake + Send + Sync + 'static> From<Arc<W>> for RawWaker {
fn from(waker: Arc<W>) -> RawWaker {
raw_waker(waker)
Expand Down