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

Stabilize the Wake trait #74304

Merged
merged 1 commit into from
Feb 5, 2021
Merged

Stabilize the Wake trait #74304

merged 1 commit into from
Feb 5, 2021

Conversation

yoshuawuyts
Copy link
Member

This PR proposes stabilizing the wake_trait feature, tracking issue #69912.

Motivation

The surface area this trait introduces is small, and it has been on nightly for 4 months without any reported issues. Given the surface area of this trait is small and only serves to provide a safe interface around the already stable std::task::RawWakerVTable it seems unlikely this trait will require any further changes. So I'm proposing we stabilize this.

Personally I would love to have this available on stable, since it would enable cleaning up some runtime internals by removing the tedious pointer required to construct a RawWakerVTable. I believe the intent was always to introduce a Wake counterpart to RawWaker in order to safely construct Waker instances. And the Wake trait feels like it does that job as intended.

Implementation notes

This PR itself fixes a link in the docs, and introduces an example of how to use the trait: a minimal block_on example that runs a future to completion on the current thread. It doesn't include fancier features such as support for nesting, but is intended to serve as a teaching device for both task::Wake and futures alike.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(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 Jul 13, 2020
src/liballoc/task.rs Outdated Show resolved Hide resolved
@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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jul 13, 2020
@jonas-schievink jonas-schievink added this to the 1.46 milestone Jul 13, 2020
src/liballoc/task.rs Outdated Show resolved Hide resolved
@yoshuawuyts
Copy link
Member Author

Updated with @Aaron1011's feedback!

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 14, 2020
…ark-Simulacrum

Remove unnecessary type hints from Wake internals

While working on rust-lang#74304 I noticed we were writing out the type signature twice in some internal `Wake` impl methods; this streamlines that. Thanks!
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 14, 2020
…ark-Simulacrum

Remove unnecessary type hints from Wake internals

While working on rust-lang#74304 I noticed we were writing out the type signature twice in some internal `Wake` impl methods; this streamlines that. Thanks!
@tmandry
Copy link
Member

tmandry commented Jul 16, 2020

Looks good to me!

r? @dtolnay for libs team FCP

@rust-highfive rust-highfive assigned dtolnay and unassigned kennytm Jul 16, 2020
src/liballoc/task.rs Outdated Show resolved Hide resolved
@Dylan-DPC-zz Dylan-DPC-zz modified the milestones: 1.46, 1.47 Jul 18, 2020
@dtolnay
Copy link
Member

dtolnay commented Jul 18, 2020

Reassigning for an opinion on FCP since the tracking issue is quite bare and I haven't been up to speed on the use cases/whether this would be ready to consider stabilizing: r? @withoutboats

src/liballoc/task.rs Outdated Show resolved Hide resolved
@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Jul 19, 2020

@dtolnay I've posted an overview of use cases on the tracking issue: #69912 (comment).

@yoshuawuyts
Copy link
Member Author

Updated with feedback from @jyn514, @tmiasko, and @Aaron1011. Also bumped the version to point to 1.47 since 1.46 is already in beta.

@bors
Copy link
Contributor

bors commented Jul 28, 2020

☔ The latest upstream changes (presumably #73265) made this pull request unmergeable. Please resolve the merge conflicts.

@Dylan-DPC-zz Dylan-DPC-zz removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Jan 22, 2021

cc @rust-lang/libs this hasn't made it up to the top of our nominated backlog to discuss yet. Are we happy to proceed with the stabilization of Waker possibly without a blanket implementation for Fn? My personal preference is yes (mostly because I'd rather not add the Fn blanket at all), but it also sounds like we were reasonably confident it wouldn't be a compatibility hazard to introduce a blanket later.

@yoshuawuyts
Copy link
Member Author

Rebased on the main branch and tidied up the commits; think that should bring this PR back to a state where it can be merged if the libs team decides it's right to do so.

@bors
Copy link
Contributor

bors commented Feb 1, 2021

☔ The latest upstream changes (presumably #81625) made this pull request unmergeable. Please resolve the merge conflicts.

Co-Authored-By: Ashley Mannix <[email protected]>
@yoshuawuyts
Copy link
Member Author

Are we happy to proceed with the stabilization of Waker possibly without a blanket implementation for Fn?

I'd be in favor of this, even if that meant we couldn't later implement the blanket impl for Fn. Using wakers directly is a bit of a niche already, and creating them even more so. Not having a blanket impl of Waker for Fn wouldn't affect end-users of async Rust in any meaningful way, which to me feels that it's okay to move ahead with this even without 100% assurance the blanket impl can be added later.

My worry is that because we currently don't have anyone dedicated to figuring out the type level details, if we defer making any decisions, Wake will sit around for another year without stabilizing. And that doesn't seem worth it, as Wake can remove unsafe from a fair bit of the ecosystem.

@KodrAus
Copy link
Contributor

KodrAus commented Feb 3, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Feb 3, 2021

📌 Commit 2c8bf1d has been approved by KodrAus

@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 Feb 3, 2021
@bors
Copy link
Contributor

bors commented Feb 4, 2021

⌛ Testing commit 2c8bf1d with merge efaa305a16a30dc82272f1b5d7d63c7102370167...

@bors
Copy link
Contributor

bors commented Feb 4, 2021

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 4, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Feb 4, 2021

@bors retry

@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 Feb 4, 2021
@rust-log-analyzer

This comment has been minimized.

m-ou-se added a commit to m-ou-se/rust that referenced this pull request Feb 4, 2021
Stabilize the Wake trait

This PR proposes stabilizing the `wake_trait` feature, tracking issue rust-lang#69912.

## Motivation

The surface area this trait introduces is small, and it has been on nightly for 4 months without any reported issues. Given the surface area of this trait is small and only serves to provide a safe interface around the already stable [`std::task::RawWakerVTable`](https://doc.rust-lang.org/std/task/struct.RawWaker.html) it seems unlikely this trait will require any further changes. So I'm proposing we stabilize this.

Personally I would love to have this available on stable, since it would enable cleaning up some runtime internals by removing the tedious pointer required to construct a [`RawWakerVTable`](https://doc.rust-lang.org/std/task/struct.RawWakerVTable.html). I believe the intent was always to introduce a `Wake` counterpart to `RawWaker` in order to safely construct `Waker` instances. And the `Wake` trait feels like it does that job as intended.

## Implementation notes

This PR itself fixes a link in the docs, and introduces an example of how to use the trait: a minimal `block_on` example that runs a future to completion on the current thread. It doesn't include fancier features such as support for nesting, but is intended to serve as a teaching device for both `task::Wake` and futures alike.
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 4, 2021
Rollup of 9 pull requests

Successful merges:

 - rust-lang#74304 (Stabilize the Wake trait)
 - rust-lang#79805 (Rename Iterator::fold_first to reduce and stabilize it)
 - rust-lang#81556 (introduce future-compatibility warning for forbidden lint groups)
 - rust-lang#81645 (Add lint for `panic!(123)` which is not accepted in Rust 2021.)
 - rust-lang#81710 (OsStr eq_ignore_ascii_case takes arg by value)
 - rust-lang#81711 (add #[inline] to all the public IpAddr functions)
 - rust-lang#81725 (Move test to be with the others)
 - rust-lang#81727 (Revert stabilizing integer::BITS.)
 - rust-lang#81745 (Stabilize poison API of Once, rename poisoned())

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d20d097 into rust-lang:master Feb 5, 2021
@rustbot rustbot modified the milestones: 1.48.0, 1.51.0 Feb 5, 2021
@yoshuawuyts yoshuawuyts deleted the stabilize-wake branch February 5, 2021 09:40
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-merge This issue / PR is in PFCP or FCP with a disposition to merge 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.