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 Condvar::wait_while and wait_timeout_while (previously wait_until, wait_timeout_until) #67076

Merged
merged 2 commits into from Jan 14, 2020

Conversation

@mbrubeck
Copy link
Contributor

mbrubeck commented Dec 6, 2019

Closes #47960.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 6, 2019

r? @Mark-Simulacrum

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

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Dec 6, 2019

r? @dtolnay for commencing FCP here, this is proposing to stabilize the following APIs, implemented in #47970.

impl Condvar {
    fn wait_until<'a, T, F>(&self, mut guard: MutexGuard<'a, T>,
                            mut condition: F)
                            -> LockResult<MutexGuard<'a, T>>
                            where F: FnMut(&mut T) -> bool;
    fn wait_timeout_until<'a, T, F>(&self, mut guard: MutexGuard<'a, T>,
                                    dur: Duration, mut condition: F)
                                    -> LockResult<(MutexGuard<'a, T>, WaitTimeoutResult)>
                                    where F: FnMut(&mut T) -> bool;
}

For some background, these APIs mirror the ones present in C++ (though there they are through overloads on std::condition_variable::wait and wait_for). There appear to be no open questions on the tracking issue. The API being fairly verbose due to the absorbance of poisoning along with timeouts into the return type. It seems that unless we choose to diverge from the main API Condvar::wait, this is the only API we can go with. I suspect that such a divergence would not be worth it in the overall design of Condvar.

parking_lot does not implement these two functions, though it does offer a wait_until API -- but that focuses on Instant rather than Duration plus a closure. I think the divergence here isn't ideal but std is staying consistent with the APIs currently provided.

Before stabilization, we would likely want to update the documentation to avoid the language around ignoring spurious wakeups (which is not true; the closure is still run).

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Dec 11, 2019

I am on board with these APIs.

These are practically always what you want -- there are few use cases for directly using wait or wait_timeout that isn't an exact reimplementation of wait_until or wait_timeout_until. The main exception would when you need a condition that is fallible. Based on @mbrubeck's comment in #47960 (comment) asking for these to be stabilized, I expect their codebase consists of conditions that are all or almost all infallible where these APIs are a good fit.

impl Condvar {
    fn wait_until<'a, T, F>(
        &self,
        guard: MutexGuard<'a, T>,
        condition: F,
    ) -> LockResult<MutexGuard<'a, T>>
    where
        F: FnMut(&mut T) -> bool;

    fn wait_timeout_until<'a, T, F>(
        &self,
        guard: MutexGuard<'a, T>,
        dur: Duration,
        condition: F,
    ) -> LockResult<(MutexGuard<'a, T>, WaitTimeoutResult)>
    where
        F: FnMut(&mut T) -> bool;
}

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Dec 11, 2019

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

Concerns:

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.

@Amanieu

This comment has been minimized.

Copy link
Contributor

Amanieu commented Dec 11, 2019

@rfcbot concern _until naming

Using the name _until might cause confusion if in the future we add a different way of specifying an timeout using an Instant instead of a Duration.

See parking_lot's Condvar::wait_until.

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Dec 11, 2019

Good call. Perhaps wait_while, with the meaning of the bool inverted? Std has other _while methods with predicates, and it is unambiguous with referring to a timeout. @mbrubeck do you have a sense of whether inverting the bool would cause confusing negations in your codebase?

@mbrubeck

This comment has been minimized.

Copy link
Contributor Author

mbrubeck commented Dec 11, 2019

Our uses look like approximately like:

wait_until(guard, |x| x.count == 0)
// or
wait_until(guard, |x| x.started)

I think these are about equally readable as:

wait_while(guard, |x| x.count > 0)
// or
wait_while(guard, |x| !x.started)

(Or for the last one we could flip the meaning of the boolean and rename it to pending.)

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Dec 16, 2019

That looks all right to me. I would be prepared to accept this stabilization after a rename to wait_while / wait_timeout_while.

@mbrubeck

This comment has been minimized.

Copy link
Contributor Author

mbrubeck commented Dec 16, 2019

Added a commit to rename both methods (flipping the meaning of the condition) and update all docs and tests.

@mbrubeck mbrubeck changed the title Stabilize Condvar::wait_until and wait_timeout_until Stabilize and rename Condvar::wait_until and wait_timeout_until Dec 16, 2019
@mbrubeck mbrubeck force-pushed the mbrubeck:condvar branch from a7fecd2 to 4a64be2 Dec 17, 2019
@Centril Centril modified the milestones: 1.41, 1.42 Dec 20, 2019
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 23, 2019

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

@mbrubeck mbrubeck force-pushed the mbrubeck:condvar branch from 4a64be2 to a3227b7 Dec 26, 2019
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 26, 2019

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-12-26T17:34:14.4026487Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-26T17:34:14.4257325Z ##[command]git config gc.auto 0
2019-12-26T17:34:14.4314985Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-26T17:34:14.4350257Z ##[command]git config --get-all http.proxy
2019-12-26T17:34:14.4525462Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67076/merge:refs/remotes/pull/67076/merge
---
2019-12-26T18:34:15.5023167Z .................................................................................................... 1600/9454
2019-12-26T18:34:19.9658911Z .................................................................................................... 1700/9454
2019-12-26T18:34:28.9240416Z .............................................................................................i...... 1800/9454
2019-12-26T18:34:36.2393548Z .................................................................................................... 1900/9454
2019-12-26T18:34:42.6884196Z ...............................................................................iiiii................ 2000/9454
2019-12-26T18:35:02.7253577Z .................................................................................................... 2200/9454
2019-12-26T18:35:04.8962084Z .................................................................................................... 2300/9454
2019-12-26T18:35:07.1924098Z .................................................................................................... 2400/9454
2019-12-26T18:35:13.2281373Z .................................................................................................... 2500/9454
---
2019-12-26T18:38:16.2042573Z ..........i...............i......................................................................... 4900/9454
2019-12-26T18:38:26.6496194Z .................................................................................................... 5000/9454
2019-12-26T18:38:32.1271365Z ......................................................i............................................. 5100/9454
2019-12-26T18:38:42.3008634Z .................................................................................................... 5200/9454
2019-12-26T18:38:48.9051572Z .....................ii.ii...........i.............................................................. 5300/9454
2019-12-26T18:38:58.1835493Z .................................................................................................... 5500/9454
2019-12-26T18:39:09.5085672Z .................................................................................................... 5600/9454
2019-12-26T18:39:17.0926709Z ...i................................................................................................ 5700/9454
2019-12-26T18:39:22.8013460Z .................................................................................................... 5800/9454
2019-12-26T18:39:22.8013460Z .................................................................................................... 5800/9454
2019-12-26T18:39:33.4208940Z ...........................................................................................ii...i..i 5900/9454
2019-12-26T18:39:46.0009404Z i...........i....................................................................................... 6000/9454
2019-12-26T18:40:03.2681551Z .................................................................................................... 6200/9454
2019-12-26T18:40:11.0197173Z .................................................................................................... 6300/9454
2019-12-26T18:40:11.0197173Z .................................................................................................... 6300/9454
2019-12-26T18:40:25.2867753Z ..................i..ii............................................................................. 6400/9454
2019-12-26T18:40:45.4157208Z ..............................................................................................i..... 6600/9454
2019-12-26T18:40:47.5572977Z .................................................................................................... 6700/9454
2019-12-26T18:40:49.8803014Z ..............................................................................................i..... 6800/9454
2019-12-26T18:40:52.4908131Z .................................................................................................... 6900/9454
---
2019-12-26T18:42:30.8983184Z .................................................................................................... 7500/9454
2019-12-26T18:42:35.3718659Z .................................................................................................... 7600/9454
2019-12-26T18:42:41.9355720Z .................................................................................................... 7700/9454
2019-12-26T18:42:51.8442828Z .................................................................................................... 7800/9454
2019-12-26T18:42:58.0201979Z .........................iiii....................................................................... 7900/9454
2019-12-26T18:43:13.3299895Z .................................................................................................... 8100/9454
2019-12-26T18:43:22.8207526Z .................................................................................................... 8200/9454
2019-12-26T18:43:35.7475298Z .................................................................................................... 8300/9454
2019-12-26T18:43:42.6155679Z .................................................................................................... 8400/9454
---
2019-12-26T18:45:57.0469271Z  finished in 6.567
2019-12-26T18:45:57.0695852Z Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-12-26T18:45:57.2560172Z 
2019-12-26T18:45:57.2561994Z running 166 tests
2019-12-26T18:46:00.4948751Z iiii......i........ii..iiii...i.............................i..i..................i....i............ 100/166
2019-12-26T18:46:02.7562567Z i.i.i...iii..iiiiiii.......................iii............ii......
2019-12-26T18:46:02.7563177Z 
2019-12-26T18:46:02.7563233Z  finished in 5.686
2019-12-26T18:46:02.7790453Z Check compiletest suite=codegen-units mode=codegen-units (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-12-26T18:46:02.9609602Z 
---
2019-12-26T18:46:05.0007505Z  finished in 2.220
2019-12-26T18:46:05.0209220Z Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-12-26T18:46:05.1908698Z 
2019-12-26T18:46:05.1910439Z running 9 tests
2019-12-26T18:46:05.1911269Z iiiiiiiii
2019-12-26T18:46:05.1911677Z 
2019-12-26T18:46:05.1911723Z  finished in 0.170
2019-12-26T18:46:05.2147698Z Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-12-26T18:46:05.3998063Z 
---
2019-12-26T18:46:24.0907591Z  finished in 18.876
2019-12-26T18:46:24.1121249Z Check compiletest suite=debuginfo mode=debuginfo-gdb+lldb (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-12-26T18:46:24.2645716Z 
2019-12-26T18:46:24.2646569Z running 124 tests
2019-12-26T18:46:47.7194493Z .iiiii..ii.....i..i...i..i.i.i..i..i..iii....ii.ii....ii..........iiii..........i.....i..ii.......ii 100/124
2019-12-26T18:46:51.8050225Z .i.iii.....iiiiii.....ii
2019-12-26T18:46:51.8051464Z 
2019-12-26T18:46:51.8052602Z  finished in 27.692
2019-12-26T18:46:51.8061260Z Uplifting stage1 rustc (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-12-26T18:46:51.8061903Z Copying stage2 rustc from stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
---
2019-12-26T18:59:59.3767014Z 
2019-12-26T18:59:59.3768016Z    Doc-tests core
2019-12-26T19:00:04.2090351Z 
2019-12-26T19:00:04.2091260Z running 2439 tests
2019-12-26T19:00:14.0325750Z ......iiiii......................................................................................... 100/2439
2019-12-26T19:00:23.2179659Z ..................................................................................ii................ 200/2439
2019-12-26T19:00:43.4261144Z ................i................................................................................... 400/2439
2019-12-26T19:00:43.4261144Z ................i................................................................................... 400/2439
2019-12-26T19:00:52.5698219Z ................................................................i..i..................iiii.......... 500/2439
2019-12-26T19:01:09.9779377Z .................................................................................................... 700/2439
2019-12-26T19:01:18.3139099Z .................................................................................................... 800/2439
2019-12-26T19:01:26.5074361Z .................................................................................................... 900/2439
2019-12-26T19:01:34.8017701Z .................................................................................................... 1000/2439
---
2019-12-26T19:03:43.4611162Z    Compiling std v0.0.0 (/checkout/src/libstd)
2019-12-26T19:03:45.5358385Z error[E0433]: failed to resolve: use of undeclared type or module `AtomicBool`
2019-12-26T19:03:45.5358721Z    --> src/libstd/sync/condvar.rs:777:37
2019-12-26T19:03:45.5358913Z     |
2019-12-26T19:03:45.5359913Z 777 |             let notified = Arc::new(AtomicBool::new(false));
2019-12-26T19:03:45.5360343Z 
2019-12-26T19:03:45.5360673Z error[E0433]: failed to resolve: use of undeclared type or module `Ordering`
2019-12-26T19:03:45.5360938Z    --> src/libstd/sync/condvar.rs:783:43
2019-12-26T19:03:45.5361152Z     |
2019-12-26T19:03:45.5361152Z     |
2019-12-26T19:03:45.5361471Z 783 |                 notified_copy.store(true, Ordering::SeqCst);
2019-12-26T19:03:45.5361846Z     |                                           ^^^^^^^^ use of undeclared type or module `Ordering`
2019-12-26T19:03:45.5367416Z error[E0433]: failed to resolve: use of undeclared type or module `Ordering`
2019-12-26T19:03:45.5367642Z    --> src/libstd/sync/condvar.rs:790:31
2019-12-26T19:03:45.5367808Z     |
2019-12-26T19:03:45.5367808Z     |
2019-12-26T19:03:45.5368068Z 790 |             if !notified.load(Ordering::SeqCst) {
2019-12-26T19:03:45.5368349Z     |                               ^^^^^^^^ use of undeclared type or module `Ordering`
2019-12-26T19:03:45.5368385Z 
2019-12-26T19:03:55.8760829Z error[E0599]: no method named `clone` found for type `realstd::sync::Arc<_>` in the current scope
2019-12-26T19:03:55.8763072Z     |
2019-12-26T19:03:55.8763072Z     |
2019-12-26T19:03:55.8764028Z 778 |             let notified_copy = notified.clone();
2019-12-26T19:03:55.8764623Z     |                                          ^^^^^ method not found in `realstd::sync::Arc<_>`
2019-12-26T19:03:55.8765672Z     = note: notified is a function, perhaps you wish to call it
2019-12-26T19:03:55.8765923Z 
2019-12-26T19:03:55.8765923Z 
2019-12-26T19:03:55.8782325Z error[E0599]: no method named `load` found for type `realstd::sync::Arc<_>` in the current scope
2019-12-26T19:03:55.8783372Z     |
2019-12-26T19:03:55.8783372Z     |
2019-12-26T19:03:55.8783894Z 790 |             if !notified.load(Ordering::SeqCst) {
2019-12-26T19:03:55.8784405Z     |                          ^^^^ method not found in `realstd::sync::Arc<_>`
2019-12-26T19:03:55.8785545Z     = note: notified is a function, perhaps you wish to call it
2019-12-26T19:03:55.8785850Z 
2019-12-26T19:03:56.9402486Z error: aborting due to 5 previous errors
2019-12-26T19:03:56.9402573Z 
---
2019-12-26T19:03:56.9729916Z   local time: Thu Dec 26 19:03:56 UTC 2019
2019-12-26T19:03:57.2573929Z   network time: Thu, 26 Dec 2019 19:03:57 GMT
2019-12-26T19:03:57.2575106Z == end clock drift check ==
2019-12-26T19:03:57.8087657Z 
2019-12-26T19:03:57.8185955Z ##[error]Bash exited with code '1'.
2019-12-26T19:03:57.8282271Z ##[section]Starting: Checkout
2019-12-26T19:03:57.8283839Z ==============================================================================
2019-12-26T19:03:57.8283903Z Task         : Get sources
2019-12-26T19:03:57.8283941Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@mbrubeck mbrubeck force-pushed the mbrubeck:condvar branch from a3227b7 to 98d054a Dec 26, 2019
@mbrubeck mbrubeck changed the title Stabilize and rename Condvar::wait_until and wait_timeout_until Stabilize Condvar::wait_while and wait_timeout_while (previously wait_until, wait_timeout_until) Dec 30, 2019
@mbrubeck

This comment has been minimized.

Copy link
Contributor Author

mbrubeck commented Jan 3, 2020

Why hasn't this entered final comment period yet? Does the concern need to be marked resolved?

@Amanieu

This comment has been minimized.

Copy link
Contributor

Amanieu commented Jan 3, 2020

@rfcbot resolved _until naming

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 3, 2020

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

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Jan 9, 2020

Shouldn't renaming usually mean we wait a bit for further feedback before stabilizing?

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Jan 9, 2020

Not always - sometimes the thing we learn from the FCP is that we want to make a small change (like wait_until -> wait_while), but otherwise feel confident enough that we don't need to wait another N months to stabilize.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 13, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

Copy link
Member

dtolnay left a comment

Thanks @mbrubeck!

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Jan 14, 2020

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 14, 2020

📌 Commit 98d054a has been approved by dtolnay

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 14, 2020

⌛️ Testing commit 98d054a with merge cb6122d...

bors added a commit that referenced this pull request Jan 14, 2020
Stabilize Condvar::wait_while and wait_timeout_while (previously wait_until, wait_timeout_until)

Closes #47960.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 14, 2020

☀️ Test successful - checks-azure
Approved by: dtolnay
Pushing cb6122d to master...

@bors bors added the merged-by-bors label Jan 14, 2020
@bors bors merged commit 98d054a into rust-lang:master Jan 14, 2020
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20191226.56 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.