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

Impl task::Wake for Fn #75793

Closed
wants to merge 1 commit into from
Closed

Impl task::Wake for Fn #75793

wants to merge 1 commit into from

Conversation

a1phyr
Copy link
Contributor

@a1phyr a1phyr commented Aug 21, 2020

Add a blanket alloc::task::Wake impl for F: Fn() + Send + Sync + 'static.

Motivation

The goal of the Wake trait is to ease the creation of Wakers (especially by removing the need of unsafe). This proposal goes further by offering to create them from closures :

use std::sync::Arc;
use std::task::Waker;

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

Prior art

Alternatives

  • Do nothing
  • Create a Waker::from_fn function to provide equivalent functionnality without using trait Wake.

@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 Aug 21, 2020
@taiki-e
Copy link
Member

taiki-e commented Aug 22, 2020

previous discussion: #70764

@jyn514 jyn514 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 3, 2020
@jonas-schievink jonas-schievink added A-async-await Area: Async & Await relnotes Marks issues that should be documented in the release notes of the next release. labels Sep 13, 2020
@Dylan-DPC-zz
Copy link

r? @KodrAus

@rust-highfive rust-highfive assigned KodrAus and unassigned sfackler Oct 4, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Oct 5, 2020

Thanks for the PR @a1phyr!

I'm a bit uncomfortable adding such a broad blanket implementation here, especially since those prior art crates you've linked to seem to work just fine.

cc @withoutboats do you have any thoughts on this?

@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 23, 2020
@Dylan-DPC-zz Dylan-DPC-zz added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 23, 2020
@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Nov 13, 2020
@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Dec 4, 2020
@m-ou-se
Copy link
Member

m-ou-se commented Dec 8, 2020

r? @withoutboats

@rust-highfive rust-highfive assigned withoutboats and unassigned KodrAus Dec 8, 2020
@m-ou-se m-ou-se added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Dec 8, 2020
@withoutboats
Copy link
Contributor

This is interesting as a solution to waker_fn. cc @stjepang

It should work because Fn is fundamental, so we can add these sort of blanket impls without conflicting with other impls. If we want to support a conversion from a closure to a waker in std, this seems like the way to do it.

However, I'd also note that waker_fn also implements some optimizations to avoid the Arc when its not necessary. Not sure if those are winning much performance though.

@sfackler
Copy link
Member

sfackler commented Dec 9, 2020

I think I'd personally be a bit more comfortable with something like waker_fn than this blanket. We don't do this for any other traits in the standard library, and it seems IMO clearly a bit strange to e.g. implement Iterator for all FnMut() -> Option<T>.

@sfackler
Copy link
Member

sfackler commented Dec 9, 2020

If we do decide to do this, though, it needs to land in the same release as #74304 since Fn is fundamental.

@withoutboats
Copy link
Contributor

@sfackler Another example where we do this is the Pattern trait: https://doc.rust-lang.org/std/str/pattern/trait.Pattern.html#impl-Pattern%3C%27a%3E-5

I believe it isn't true that this would not be backward incompatible to add later.. fundamental means something different on traits than on data types; but it also hurts my brains to think about fundamental so I might be wrong.

@sfackler
Copy link
Member

sfackler commented Dec 9, 2020

Ah yeah that is a good point, though I feel the Wake case is a bit more similar to Iterator than Pattern. Patterns are (almost?) always just passed directly to methods as immediate arguments, where Wake/Iterator more commonly sit around as "persistent" values.

@a1phyr
Copy link
Contributor Author

a1phyr commented Dec 10, 2020

However, I'd also note that waker_fn also implements some optimizations to avoid the Arc when its not necessary. Not sure if those are winning much performance though.

Which implementation are you talking about ? Both the async_task and waker_fn crates use a Arc in all cases.

Anyway, I now think that a waker_fn-like function would be more flexible and elegant, but the biggest downside is that it cannot be attached to the Waker type, as Waker is defined in core but waker_fn require alloc for its implementation.

@a1phyr
Copy link
Contributor Author

a1phyr commented Dec 10, 2020

Regarding the #[fundamental] discussion, I believe that adding the blanket impl afterward would not be breaking, because users cannot implement Fn for their types. It seems to be a weak guarantee for something as important as compatibility, though.

However, it has nothing to do with Fn being fundamental IMHO. The discussion would be the same for any trait.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 11, 2021
@KodrAus
Copy link
Contributor

KodrAus commented Jan 29, 2021

Thanks for the PR @a1phyr!

@rfcbot fcp close

I think we should not add a blanket impl of Fn for Wake (at least not now), and instead consider an inherent method that wraps up an implementation for you. It seems convenient to add a blanket impl, but its utility doesn't seem to outweigh the limitations of betting on the blanket.

So instead of writing this:

use std::{sync::Arc, task::Waker};

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

You would instead write this:

use std::task::Waker;

let waker = Waker::from_fn(|| {
    println!("Woken !");
});

Since we already have a completed FCP for the Wake trait itself in #74304 if we did want to reconsider a blanket Fn impl in the future then we'd do it with backwards compatibility in mind there.

@rfcbot
Copy link

rfcbot commented Jan 29, 2021

Team member @KodrAus has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jan 29, 2021
@rfcbot
Copy link

rfcbot commented Feb 4, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Feb 4, 2021
@dtolnay dtolnay closed this Feb 8, 2021
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Feb 14, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet