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

task: remove wrong comments about non-existent LocalWake trait #52316

Merged
merged 1 commit into from
Jul 13, 2018

Conversation

seanmonstar
Copy link
Contributor

@seanmonstar seanmonstar commented Jul 12, 2018

A LocalWaker is specifically !Send , and the unsafety comment around
LocalWaker::new only specifies that it be safe to call wake_local.
One could then accidentally promote a LocalWaker into a Waker, which
is universally Send, simply via Waker::from(local_waker). A
LocalWaker the was built expecting to not be Send, such as using
Rc, could be sent to other threads safely.

Separately, though somewhat related, Context holds a &LocalWaker
internally, and exposes a waker() -> &Waker method. This simply
transmutes the &LocalWaker to &Waker, which would be unsound, except
that you can't "send" a &Waker, you'd need to clone it first. Since
UnsafeWake::clone_raw requires that it return a Waker, the transmute
is not unsound. The transmuted LocalWaker will be promoted to a
Waker correctly.

That would mean that if UnsafeWake::clone_raw were to be changed, such
as returning Self instead of Waker, this would no longer be sound.
Thus, this also adds a comment to clone_raw to remember this.

r? @cramertj

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 12, 2018
/// If you're working with the standard library then it's recommended to
/// use the `LocalWaker::from` function instead which works with the safe
/// `Rc` type and the safe `LocalWake` trait.
///
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also removed these comments, since there is no LocalWake trait.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch! Sorry, this got left in after I rewrote this code to work differently. I agree that this comment should go away, and it should instead suggest using the local_waker_from_nonlocal or local_waker functions in std::task.

@cramertj
Copy link
Member

cramertj commented Jul 12, 2018

@seanmonstar Thanks for pointing out this potential source of confusion! I think this is a documentation issue-- perhaps you can help me figure out the best way to document the required usage. LocalWaker::new takes a NonNull<UnsafeWake>, and the UnsafeWake trait itself requires Send and Sync. Implementations of UnsafeWake methods other than wake_local are expected to be thread-safe.

@seanmonstar
Copy link
Contributor Author

With UnsafeWake: Send + Sync, it seems that a user couldn't actually create a local waker that clones via Rc instead of Arc. In that case, I'm having a hard time seeing how LocalWaker could be useful then, unless users are expected to unsafe impl Send for MyWaker and promise themselves to never actually send it, which sounds like a very scary thing to encourage people to do.

@cramertj
Copy link
Member

@seanmonstar

With UnsafeWake: Send + Sync, it seems that a user couldn't actually create a local waker that clones via Rc instead of Arc.

That's correct!

I'm having a hard time seeing how LocalWaker could be useful then, unless users are expected to unsafe impl Send for MyWaker and promise themselves to never actually send it, which sounds like a very scary thing to encourage people to do.

LocalWaker allows for unsynchronized access to memory inside of the waker that would otherwise have to be synchronized. For example:

struct MyOptimizedTaskQueue {
    local_queue: RefCell<VecDeque<Arc<MyTask>>>,
    sync_queue: crossbeam::sync::SegQueue<Arc<MyTask>>,
}

unsafe impl Send for MyOptimizedTaskQueue {}
unsafe impl Sync for MyOptimizedTaskQueue {}

struct MyTask {
    task: RefCell<TaskObj>,
    queue: Arc<MyOptimizedTaskQueue>,
}

unsafe impl Send for MyTask {}
unsafe impl Sync for MyTask {}

impl Wake for MyOptimizedTaskQueue {
    fn wake(arc_self: &Arc<Self>) {
        arc_self.queue.sync_queue.push(arc_self.clone());
    }

    // HERE is the key bit that `LocalWaker` allows-- because we know we're on the same thread
    // from which the `LocalWaker` was created, we know that we can use the `local_queue` `RefCell`
    // without any synchronization.
    unsafe fn wake_local(arc_self: &Arc<Self>) {
        arc_self.queue.local_queue.push(arc_self.clone());
    }
}

This gives you a single-threaded task system that only requires synchronization for Arc cloning-- not for wakeups nor for polling futures.

@seanmonstar
Copy link
Contributor Author

O_O Being able to be certain that the waker is actually local seems very tricky; I'd be suspicious of code that thinks it's been done safely.

Anyways, seems the only thing then here is that the documentation mentions non-existent LocalWake, so I'll update.

@seanmonstar seanmonstar changed the title task: remove unsound Waker::from(LocalWaker), and document more unsafety task: remove wrong comments about non-existent LocalWake trait Jul 12, 2018
@cramertj
Copy link
Member

O_O Being able to be certain that the waker is actually local seems very tricky; I'd be suspicious of code that thinks it's been done safely.

Right-- you have to create it in one place, on one thread, and then it can be passed down into task::Context after calling the (unsafe) local_waker function. LocalWaker isn't Send or Sync, so this makes it impossible to misuse.

@cramertj
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 13, 2018

📌 Commit 4f4e91a has been approved by cramertj

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 13, 2018
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 13, 2018

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
task: remove wrong comments about non-existent LocalWake trait

~~A `LocalWaker` is specifically `!Send `, and the unsafety comment around
`LocalWaker::new` only specifies that it be safe to call `wake_local`.
One could then accidentally promote a `LocalWaker` into a `Waker`, which
is universally `Send`, simply via `Waker::from(local_waker)`. A
`LocalWaker` the was built expecting to not be `Send`, such as using
`Rc`, could be sent to other threads safely.~~

~~Separately, though somewhat related, `Context` holds a `&LocalWaker`
internally, and exposes a `waker() -> &Waker` method. This simply
transmutes the `&LocalWaker` to `&Waker`, which would be unsound, except
that you can't "send" a `&Waker`, you'd need to clone it first. Since
`UnsafeWake::clone_raw` requires that it return a `Waker`, the transmute
is not unsound. The transmuted `LocalWaker` will be promoted to a
`Waker` correctly.~~

~~That would mean that if `UnsafeWake::clone_raw` were to be changed, such
as returning `Self` instead of `Waker`, this would no longer be sound.
Thus, this also adds a comment to `clone_raw` to remember this.~~

r? @cramertj
bors added a commit that referenced this pull request Jul 13, 2018
Rollup of 16 pull requests

Successful merges:

 - #51962 (Provide llvm-strip in llvm-tools component)
 - #52003 (Implement `Option::replace` in the core library)
 - #52156 (Update std::ascii::ASCIIExt deprecation notes)
 - #52242 (NLL: Suggest `ref mut` and `&mut self`)
 - #52244 (Don't display default generic parameters in diagnostics that compare types)
 - #52290 (Deny bare trait objects in src/librustc_save_analysis)
 - #52293 (Deny bare trait objects in librustc_typeck)
 - #52299 (Deny bare trait objects in src/libserialize)
 - #52300 (Deny bare trait objects in librustc_target and libtest)
 - #52302 (Deny bare trait objects in the rest of rust)
 - #52310 (Backport 1.27.1 release notes to master)
 - #52314 (Fix ICE when using a pointer cast as array size)
 - #52315 (Resolve FIXME(#27942))
 - #52316 (task: remove wrong comments about non-existent LocalWake trait)
 - #52322 (Update llvm-rebuild-trigger in light of LLVM 7 upgrade)
 - #52332 (dead-code lint: say "constructed", "called" for structs, functions)

Failed merges:

r? @ghost
kennytm added a commit to kennytm/rust that referenced this pull request Jul 13, 2018

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
task: remove wrong comments about non-existent LocalWake trait

~~A `LocalWaker` is specifically `!Send `, and the unsafety comment around
`LocalWaker::new` only specifies that it be safe to call `wake_local`.
One could then accidentally promote a `LocalWaker` into a `Waker`, which
is universally `Send`, simply via `Waker::from(local_waker)`. A
`LocalWaker` the was built expecting to not be `Send`, such as using
`Rc`, could be sent to other threads safely.~~

~~Separately, though somewhat related, `Context` holds a `&LocalWaker`
internally, and exposes a `waker() -> &Waker` method. This simply
transmutes the `&LocalWaker` to `&Waker`, which would be unsound, except
that you can't "send" a `&Waker`, you'd need to clone it first. Since
`UnsafeWake::clone_raw` requires that it return a `Waker`, the transmute
is not unsound. The transmuted `LocalWaker` will be promoted to a
`Waker` correctly.~~

~~That would mean that if `UnsafeWake::clone_raw` were to be changed, such
as returning `Self` instead of `Waker`, this would no longer be sound.
Thus, this also adds a comment to `clone_raw` to remember this.~~

r? @cramertj
bors added a commit that referenced this pull request Jul 13, 2018
Rollup of 17 pull requests

Successful merges:

 - #51962 (Provide llvm-strip in llvm-tools component)
 - #52003 (Implement `Option::replace` in the core library)
 - #52156 (Update std::ascii::ASCIIExt deprecation notes)
 - #52280 (llvm-tools-preview: fix build-manifest)
 - #52290 (Deny bare trait objects in src/librustc_save_analysis)
 - #52293 (Deny bare trait objects in librustc_typeck)
 - #52299 (Deny bare trait objects in src/libserialize)
 - #52300 (Deny bare trait objects in librustc_target and libtest)
 - #52302 (Deny bare trait objects in the rest of rust)
 - #52310 (Backport 1.27.1 release notes to master)
 - #52315 (Resolve FIXME(#27942))
 - #52316 (task: remove wrong comments about non-existent LocalWake trait)
 - #52322 (Update llvm-rebuild-trigger in light of LLVM 7 upgrade)
 - #52330 (Don't silently ignore invalid data in target spec)
 - #52333 (CI: Enable core dump on Linux, and print their stack trace on segfault. )
 - #52346 (Fix typo in improper_ctypes suggestion)
 - #52350 (Bump bootstrap compiler to 1.28.0-beta.10)

Failed merges:

r? @ghost
@bors bors merged commit 4f4e91a into rust-lang:master Jul 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants