Add Condvar APIs not susceptible to spurious wake #47970

vlovich · 2018-02-02T19:22:46Z

Provide wait_until and wait_timeout_until helper wrappers that aren't susceptible to spurious wake.
Additionally wait_timeout_until makes it possible to more easily write code that waits for a fixed amount of time in face of spurious wakes since otherwise each user would have to do math on adjusting the duration.

Implements #47960.

rust-highfive · 2018-02-02T19:22:58Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Provide wait_until and wait_timeout_until helper wrappers that aren't susceptible to spurious wake.

sfackler · 2018-02-02T19:59:55Z

src/libstd/sync/condvar.rs

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


This should take &mut T.

sfackler · 2018-02-02T20:00:12Z

src/libstd/sync/condvar.rs

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


This should take &mut T.

sfackler · 2018-02-02T20:02:14Z

src/libstd/sync/condvar.rs

+            }
+            let wait_timer = Instant::now();
+            let wait_result = self.wait_timeout(guard, dur)?;
+            dur = dur.checked_sub(wait_timer.elapsed()).unwrap_or(timed_out);


wait_result already contains the information about if we time out or not. It seems like we should use that rather than doubling the time manipulation work.

Unfortunately that doesn't work because the semantics of the function are such that we can't return timeout if the condition is satisfied. This particular wait_timeout (since we are doing it in a loop to protect against spurious wakes) may have timed-out with the condition satisfied so we need to check that first.

How is the difference between a checking after a timeout and not checking semantically observable?

The semantic difference is whether or not the contract for the function is observed which is that condition == true xor WaitTimeoutResult(true). Since the condition is protected by a mutex, failure to check the condition before exiting would result in a potential race where WaitTimeoutResult(true) is returned even though the condition has been met.

FWIW C++ has this same pattern of checking the result before returning. The main difference is that they don't adjust the wait duration time so you could potentially never wake up if you keep getting spurious wakeups before the timeout.

I think the C++ standard (at least as described on cppreference & implemented by libstdc++ & libc++) is broken here because they don't account for time slept in their loop, but there might be a simpler way to rewrite this more like the C++ implementation.

EDIT: Nvm. I totally misread the cppreference. They use absolute timestamps for sleeping so the code is simpler.

Do you think

let timed_out = Duration::new(0, 0); while !condition(&mut *guard) { let wait_timer = Instant::now(); let wait_result = self.wait_timeout(guard, dur)?; dur = dur.checked_sub(wait_timer.elapsed()).unwrap_or(timed_out); guard = wait_result.0; if wait_result.1.timed_out() { Ok((guard, WaitTimeoutResult(condition(&mut *guard)))) } } Ok((guard, WaitTimeoutResult(true)))

is more readable?

How about something like

let start = Instant::now(); loop { if condition(guard) { return Ok((guard, WaitTimeoutResult(true))); } let timeout = match dur.checked_sub(start.elapsed()) { Some(timeout) => timeout, None => return Ok((guard, WaitTimeoutResult(false))), }; guard = self.wait_timeout(guard, dur)?.0; }

I think that works.

Although I think you have the true/false reversed for the WaitTimeoutResult.

sfackler · 2018-02-02T20:03:26Z

src/libstd/sync/condvar.rs

@@ -219,6 +219,61 @@ impl Condvar {
        }
    }

+    /// Blocks the current thread until this condition variable receives a
+    /// notification and the required condition is met. There are no spurious
+    /// wakeups when calling this.


Saying that there aren't any spurious wakeups doesn't seem totally accurate - the thread is awake when the closure is executing.

You're right. I meant to say this function won't return due to spurious wakes. I'll clarify.

sfackler · 2018-02-02T20:03:57Z

src/libstd/sync/condvar.rs

+    /// // Wait for the thread to start up.
+    /// let &(ref lock, ref cvar) = &*pair;
+    /// // As long as the value inside the `Mutex` is false, we wait.
+    /// cvar.wait_until(lock.lock().unwrap(), |ref started| { started });


The ref here is unneccessary.

This is ignoring the Result returned by wait_until.

Make condition closure accept mut T&. Clarify spurious wakeup documentation. Cleanup doc example code.

sfackler · 2018-02-02T20:48:39Z

src/libstd/sync/condvar.rs

+                                        where F: FnMut(&mut T) -> bool {
+        let timed_out = Duration::new(0, 0);
+        loop {
+            if !condition(&mut *guard) {


This condition is backwards, right?

Whoops. It is. I can fix it or rewrite it as above (not sure which is simpler to read). Which would you prefer?

sfackler · 2018-02-07T02:16:01Z

src/libstd/sync/condvar.rs

+                Some(timeout) => timeout,
+                None => return Ok((guard, WaitTimeoutResult(true))),
+            }
+            guard = self.wait_timeout(guard, dur)?.0;


This should wait for timeout rather than dur.

sfackler · 2018-02-07T02:18:01Z

@rust-lang/libs Methods like these originally existed pre-1.0 but were removed to limit the API surface. I think they're valuable, though, particularly the version with the timeout.

@rfcbot fcp merge

rfcbot · 2018-02-07T02:18:02Z

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

No concerns currently listed.

Once these reviewers reach consensus, 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.

alexcrichton · 2018-02-07T03:07:09Z

These are currently added as stable, but is it intended that they're added as unstable?

sfackler · 2018-02-07T06:09:23Z

Ah yeah they should definitely land unstable.

alexcrichton · 2018-02-07T15:06:47Z

Ok! I've checked my box assuming they'll land unstable

pietroalbini · 2018-02-12T11:50:42Z

@Kimundi and @withoutboats, there are some checkboxes in #47970 (comment) waiting for you!

vlovich · 2018-02-13T05:08:50Z

I'm unfamiliar with how to tag APIs. How do I make it unstable?

sfackler · 2018-02-13T05:16:24Z

You'd replace the stable annotations with #[unstable(feature = "wait_until", issue = "N")] where N is the number of a tracking issue you open on this repo.

Switch feature guards to unstable Add missing semicolon Remove mut that's no longer necessary

dtolnay · 2018-02-16T17:53:57Z

These are being added as unstable, so I think we can go ahead and accept and then consider more closely as a team before stabilization.

@rfcbot fcp cancel
@bors r+

rfcbot · 2018-02-16T17:53:57Z

@dtolnay proposal cancelled.

bors · 2018-02-16T17:53:58Z

📌 Commit 6fe2d1d has been approved by dtolnay

Also fix some code snippets in documentation.

sfackler · 2018-02-20T22:29:31Z

@bors r=dtolnay

bors · 2018-02-20T22:29:32Z

📌 Commit 14b403c has been approved by dtolnay

Add Condvar APIs not susceptible to spurious wake Provide wait_until and wait_timeout_until helper wrappers that aren't susceptible to spurious wake. Additionally wait_timeout_until makes it possible to more easily write code that waits for a fixed amount of time in face of spurious wakes since otherwise each user would have to do math on adjusting the duration. Implements rust-lang#47960.

Rollup of 17 pull requests - Successful merges: #47964, #47970, #48076, #48115, #48166, #48281, #48297, #48302, #48362, #48369, #48489, #48491, #48494, #48517, #48529, #48235, #48330 - Failed merges:

rust-highfive assigned sfackler Feb 2, 2018

Vitali Lovich added 2 commits February 2, 2018 11:29

Add Condvar APIs not susceptible to spurious wake

7d296c4

Provide wait_until and wait_timeout_until helper wrappers that aren't susceptible to spurious wake.

Fix typo

404e1a6

sfackler reviewed Feb 2, 2018

View reviewed changes

Review response

e72bd6d

Make condition closure accept mut T&. Clarify spurious wakeup documentation. Cleanup doc example code.

sfackler reviewed Feb 2, 2018

View reviewed changes

shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 3, 2018

Simplify wait_timeout_until & fix condition typo

95e4dc2

sfackler reviewed Feb 7, 2018

View reviewed changes

sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Feb 7, 2018

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Feb 7, 2018

kennytm 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 Feb 11, 2018

Fix wait_timeout value

97df227

Misc fixes

6fe2d1d

Switch feature guards to unstable Add missing semicolon Remove mut that's no longer necessary

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 16, 2018

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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Feb 16, 2018

Vitali Lovich added 3 commits February 16, 2018 20:00

Fix unit test compilation

b1f04a3

Also fix some code snippets in documentation.

Fix tidy violation

d549db8

Fix doc compile error

14b403c

Manishearth mentioned this pull request Feb 25, 2018

Rollup of 9 pull requests #48526

Closed

kennytm mentioned this pull request Feb 25, 2018

Rollup of 17 pull requests #48531

Merged

bors merged commit 14b403c into rust-lang:master Feb 25, 2018

Mark-Simulacrum mentioned this pull request Dec 6, 2019

Stabilize Condvar::wait_while and wait_timeout_while (previously wait_until, wait_timeout_until) #67076

Merged

vlovich deleted the condvar_wait_until branch December 2, 2020 01:22

dtolnay self-assigned this Mar 24, 2024

Add Condvar APIs not susceptible to spurious wake #47970

Add Condvar APIs not susceptible to spurious wake #47970

Conversation

vlovich commented Feb 2, 2018

rust-highfive commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlovich Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfackler commented Feb 7, 2018

rfcbot commented Feb 7, 2018 • edited by dtolnay Loading

alexcrichton commented Feb 7, 2018

sfackler commented Feb 7, 2018

alexcrichton commented Feb 7, 2018

pietroalbini commented Feb 12, 2018

vlovich commented Feb 13, 2018

sfackler commented Feb 13, 2018

dtolnay commented Feb 16, 2018

rfcbot commented Feb 16, 2018

bors commented Feb 16, 2018

sfackler commented Feb 20, 2018

bors commented Feb 20, 2018

vlovich Feb 2, 2018 •

edited

Loading

rfcbot commented Feb 7, 2018 •

edited by dtolnay

Loading