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

Add conversion from Arc<Fn> to Waker #70764

Closed
wants to merge 1 commit into from
Closed

Add conversion from Arc<Fn> to Waker #70764

wants to merge 1 commit into from

Conversation

yoshuawuyts
Copy link
Member

This is an addition to work done in #68700. This patch adds a conversion from Arc<Fn> to Waker making it significantly easier to construct wakers for instrumentation purposes.

The conversion of a closure into a waker has seen prior adoption in the ecosystem in the form of the wakeful crate by @sagebind. And was later adopted by @stjepang in async-task as well.

Usage

let waker = Waker::from(Arc::new(|| {
    println!("wake called");
}));
waker.wake();

Alternatives

Originally there was talk about adding a task::waker_fn instead. But @withoutboats pointed out there had always been an intention to add a safe Wake trait, and conversions from closures could instead utilize that. Now that Wake is available on nightly, it seemed like a good idea to implement this conversion as well.

While drafting this patch I also tried to make a non-arc conversion work, but unfortunately failed because of orphan rules (Waker and Fn are defined in core, Arc is defined in alloc):

// This would use From<Fn() + Send + Sync + 'static> for Waker,
// which is currently not allowed.
let waker = Waker::from(|| {
    println!("wake called");
});
waker.wake();

I'm not sure if there is a way around that given that tests wouldn't compile if this was a problem. But if that's not desirable for any reason, we should probably review this conversion is forward-compatible with a future conversion from From<Fn() + Sync + Send + 'static> for Waker.

Stability

I think this conversion should be separate from the core trait introduced in #68700. Any issues that might arise from this should not hold up the stabilization of the core trait; so I propose we track this (and any future conversions) separately.

References

@rust-highfive
Copy link
Collaborator

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 4, 2020
@withoutboats
Copy link
Contributor

But @withoutboats pointed out there had always been an intention to add a safe Wake trait, and conversions from closures could instead utilize that.

The point I meant to make was actually a bit different: that the safe Wake trait could be a better alternative to converting from closures.

I wonder if you could elaborate on what you see as the advantages of a closure-conversion API in contrast to using the Wake trait? I'll write about how I understand the trade off.

First, a real-world example of a waker - block_on implemented on top of crossbeam Unparker:

// With the Wake trait
use std::task::{Waker, Wake};
use crossbeam::sync::{Unparker};

struct BlockOnWaker {
    unparker: Unparker,
}

impl Wake for BlockOnWaker {
    fn wake(self: Arc<Self>) {
        self.unparker.unpark();
    }
}

fn block_on<F: Future>(future: F) -> F::Output {
    let unparker = ...;
    let waker = Waker::from(Arc::new(BlockOnWaker { unparker }));
    ...
}
// With this API
use std::task::Waker;

fn block_on<F: Future>(future: F) -> F::Output {
    let unparker = ...;
    let waker = Waker::from(Arc::new(move || unparker.unpark()));
    ...
}

Certainly this API is briefer! Indeed, that's the advantage of the closure-based API that's obvious to me: it allows you to write a waker in only a few lines of code, without the "ceremony" of defining a new type and implementing Wake for it.

But I think that the ceremony is actually quite valuable, and I'm not convinced we should endorse an alternative which eliminates it:

  1. By defining a structure, the state needed by the waker is explicitly enumerated somewhere, allowing a lot more clarity into how the waker actually works. In the closure form, the state used by a waker has to be derived from whatever the closure captures in its environment.
  2. By using the Wake trait, users are exposed to whatever optional methods we add to wakers, and encouraged to implement them. Right now, this just means wake_by_ref, but we may add other methods in the future. With closures, users have no opportunity to implement these optional methods.
  3. I also think its easier to teach the idea of a "MyWaker" object that implements a method called "wake" just like the std "Waker" type does, rather than trying to explain to a user new to the task system how a "Waker" is built from a closure.

So I would say this bit of ceremony is advantageous for everyone, and only really costs points in code golf scenarios where users are showing off executor systems built in 12 lines of code and so on. From my perspective, it doesn't seem worthwhile to promote this ellison to std.

@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Apr 4, 2020

The point I meant to make was actually a bit different: that the safe Wake trait could be a better alternative to converting from closures.

@withoutboats thanks for taking your time to reply in full. Looks like I indeed misunderstood what you said. I don't disagree with any of the points you've laid out and trust your ability to assess the value this conversion has.

I guess the only thing I neither of us has touched on so far is this method as a means of teaching folks about futures: this shows a Waker can be thought of as an Arc<Fn()> (or "thread-safe callback"). But that by itself is not an overly compelling reason to add this conversion, so I think it's reasonable to close this PR.

@yoshuawuyts yoshuawuyts closed this Apr 4, 2020
@withoutboats
Copy link
Contributor

I guess the only thing I neither of us has touched on so far is this method as a means of teaching folks about futures: this shows a Waker can be thought of as an Arc<Fn()> (or "thread-safe callback"). But that by itself is not an overly compelling reason to add this conversion, so I think it's reasonable to close this PR.

This maybe has to do with our different backgrounds: having worked in pretty OOP-oriented languages before Rust, and not any language which is callback-heavy (ie not pre-ES2015 JS) I find "an object which has a wake method" a much more grokable explanation of what a waker is than "a thread-safe callback."

@yoshuawuyts yoshuawuyts deleted the waker_from_fn branch April 7, 2020 11:32
@taiki-e taiki-e mentioned this pull request Aug 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants