document expectations for Waker::wake #93966
Conversation
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
library/core/src/task/wake.rs
Outdated
| /// the runtime keeps running and the task is not finished it is expected that | ||
| /// each wake-up is followed by an invocation of `poll`, even in the absence of | ||
| /// other events. This makes it possible to yield to other tasks when running | ||
| /// potentially unbounded processing loops in order to maintain fairness. |
There was a problem hiding this comment.
I think this wording to me sounds like what you are getting at is that a call to wake() guarantees at least one call to poll (in the absence of Poll::Ready / runtime termination / panics / etc).
Maybe that's simpler than discussing coalescing etc?
There was a problem hiding this comment.
My understanding is that if I call cx.waker().wake_by_ref() twice in rapid succession, there will usually only be a single wake-up, hence “at least one call” is not correct. But with my proposed “is followed by” wording the coalescing can be left implicit if you find that more appropriate.
There was a problem hiding this comment.
Yeah, that makes sense. I think I had in mind that the interleaving is such that a wake() ensures at least one future call -- not necessarily each wake ensuring a distinct future call though.
There was a problem hiding this comment.
"Guarantees there will be at least one call in the future" sounds correct to me.
There was a problem hiding this comment.
Thanks for the review, I pushed an improvement that focuses more on the essence.
|
r? @tmandry |
JohnCSimon
added
S-waiting-on-review
| /// it possible to temporarily yield to other tasks while running potentially | ||
| /// unbounded processing loops. |
There was a problem hiding this comment.
This isn't necessarily true; the executor could choose to poll this task again immediately instead of polling other tasks in a fair manner. See related discussion on #74335.
The above guarantee does say that you are allowed to yield without breaking your code (just not that it will do anything useful). I'm quite happy to add that to the documentation.
You may want to add the caveat I mentioned to the one you wrote below.
rustbot
added
S-waiting-on-author
rustbot
added
the
T-libs
|
@rustbot ready |
rustbot
added
S-waiting-on-review
|
@bors r+ rollup I'm unclear on whether I should be approving this, but I don't want to let it sit any longer. @rust-lang/libs-api let me know if you would like to be r?'d on reviews like this in the future. |
|
📌 Commit 3d808d5 has been approved by |
bors
added
S-waiting-on-bors
Rollup of 6 pull requests Successful merges: - rust-lang#93966 (document expectations for Waker::wake) - rust-lang#97266 (Make weird name lints trigger behind cfg_attr) - rust-lang#97355 (Remove unused brush image) - rust-lang#97358 (Update minifier-rs version to 0.1.0) - rust-lang#97363 (Fix a small mistake in `SliceIndex`'s documentation) - rust-lang#97364 (Fix weird indentation in continue_keyword docs) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
fixes #93961
Opened PR for a discussion on the precise wording.