8000 Add Wake trait for safe construction of Wakers. by withoutboats · Pull Request #68700 · rust-lang/rust · GitHub
[go: up one dir, main page]
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

Show file tree
Hide file tree
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
8000 Prev Previous commit
Next Next commit
More explicit; CFG on atomic pointer
  • Loading branch information
@withoutboats
withoutboats committed Mar 23, 2020
commit 3ae74cafe483b16c6810a4ff34de03da4974b1ce
1 change: 1 addition & 0 deletions src/liballoc/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,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;
Expand Down
6 changes: 3 additions & 3 deletions src/liballoc/task.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,20 +59,20 @@ fn raw_waker<W: Wake + Send + Sync + 'static>(waker: Arc<W>) -> RawWaker {
// Increment the reference count of the arc to clone it.
unsafe fn clone_waker<W: Wake + Send + Sync + 'static>(waker: *const ()) -> RawWaker {
let waker: Arc<W> = Arc::from_raw(waker as *const W);
mem::forget(waker.clone());
mem::forget(Arc::clone(&waker));
raw_waker(waker)
Copy link
Member

Choose a reason for hiding this comment

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

If you use ManuallyDrop like in wake_by_ref, can't the entire thing just become

let waker: ManuallyDrop<Arc<W>> = ManuallyDrop::new(Arc::from_raw(waker as *const W));
raw_waker(waker.clone())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I find this less clear than mem::forgetting the clone.

Copy link
Member
@RalfJung RalfJung Mar 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Hm. I find it really confusing to create a clone only to forget it immediately. Conceptually speaking, you want to return the clone (that's what the function name says), but without dropping the original.

If you stick to the current code, I think this needs a comment explaining why it makes any sense to create a clone that is immediately forgotten.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me clone + mem forget an Arc is a pretty clear "increment the ref count" signal (TBH just having unsafe methods to fiddle with Arc's ref count would be nicer than any of this). What I would find non-obvious is why you need to put self into a manually drop.

There is a comment on this function indicating that it's purpose is to increment the refcount.

}

// Wake by value, moving the Arc into the Wake::wake function
unsafe fn wake<W: Wake + Send + Sync + 'static>(waker: *const ()) {
let waker: Arc<W> = Arc::from_raw(waker as *const W);
Wake::wake(waker);
<W as Wake>::wake(waker);
}

// Wake by reference, wrap the waker in ManuallyDrop to avoid dropping it
unsafe fn wake_by_ref<W: Wake + Send + Sync + 'static>(waker: *const ()) {
let waker: ManuallyDrop<Arc<W>> = ManuallyDrop::new(Arc::from_raw(waker as *const W));
Wake::wake_by_ref(&waker);
<W as Wake>::wake_by_ref(&waker);
}

// Decrement the reference count of the Arc on drop
Expand Down
0