Overhaul and make Condvar::wait_timeout public #21132

sfackler · 2015-01-14T07:58:09Z

The implementation is a direct adaptation of libcxx's condition_variable implementation.

I also added a wait_timeout_with method, which matches the second overload in C++'s condition_variable. The implementation right now is kind of dumb but it works. There is an outstanding issue with it: as is it doesn't support the use case where a user doesn't care about poisoning and wants to continue through poison.

r? @alexcrichton @aturon

sfackler · 2015-01-14T08:01:57Z

I think there's a pretty easy tweak that can be made to wait_timeout_with to fix the poison issue, but I'm too tired to properly think it through right now. I'll fix it up tomorrow.

sfackler · 2015-01-14T18:56:45Z

Another thing to consider here is that we may want to rename wait_timeout to wait_for. This matches with C++'s API and allows room for the addition of a wait_until method that takes an absolute time in some future release when std gains a time library.

alexcrichton · 2015-01-14T19:40:21Z

src/libstd/sys/unix/condvar.rs

+        let time_t_max: libc::time_t = Int::max_value();
+        let timeout = if time_t_max - dur.num_seconds() > sys_now.tv_sec {
+            libc::timespec {
+                tv_sec: sys_now.tv_sec + dur.num_seconds(),


Could this use .checked_add to check for overflow instead?

alexcrichton · 2015-01-14T19:52:03Z

There is an outstanding issue with it: as is it doesn't support the use case where a user doesn't care about poisoning and wants to continue through poison.

One possibly very weird thing to do would be to give the closure LockResult<&mut MutexGuard<T>> and then expect it to return Result<bool, PoisonError<&mut MutexGuard<T>>>. That's a pretty horrendous signature though, so I don't think we'd want to do that.

sfackler · 2015-01-14T19:56:05Z

I don't think it'd need to return anything other than the bool. It's not passed ownership of the guard, so it can't unlock and relock, right?

alexcrichton · 2015-01-14T19:58:03Z

I was thinking it could commonly be called as:

foo.wait_timeout_with(lock, duration, |state| Ok(try!(state).condition()));

The try! macro would somewhat necessitate returning something, although we could use FromError to do some more tricky. Altogether though it doesn't feel right no matter how I look at it, so I'm not a super huge fan of the idea.

sfackler · 2015-01-14T20:00:59Z

I think we need to work out conventions on how the poisoning stuff should actually be used. My thought was you have the common cases where you don't expect to be poisoned, so you just unwrap and propagate the failure, or you don't care about poisoning so you'd call some method on LockResult to just give you the MutexGuard no matter what.

alexcrichton · 2015-01-14T20:07:38Z

Yeah currently those looks like:

foo.unwrap() // assertion that "a poison is bad"
foo.unwrap_or_else(|e| e.into_guard()) // assertion that "I'm ok with poisons"

sfackler · 2015-01-15T04:38:28Z

Just pushed fixes up in a new commit.

alexcrichton · 2015-01-15T05:31:39Z

src/libstd/time/mod.rs

-    // Apparently android provides this in some other library?
-    #[cfg(not(target_os = "android"))]
-    #[link(name = "rt")]
-    extern {}


I think this was lost in movement to a different location

sfackler · 2015-01-15T06:36:34Z

Updated

alexcrichton · 2015-01-15T06:38:12Z

src/libstd/sync/condvar.rs

+                                       dur: Duration,
+                                       mut f: F)
+                                       -> LockResult<(MutexGuard<'a, T>, bool)>
+            where F: FnMut(&mut LockResult<MutexGuard<'a, T>>) -> bool {


Oh sorry when I said &mut T I meant:

where F: FnMut(LockResult<&mut T>) -> bool

Oh, sure that seems reasonable. We'll definitely want to rename into_guard then though.

alexcrichton · 2015-01-15T06:38:40Z

Ok this is all looking pretty good to me, would like @aturon's opinion as well, however.

sfackler · 2015-01-15T07:08:38Z

Re-updated. Ideally, the function would take a &'a mut T, but I couldn't get the lifetimes to work :(

EDIT: Oh wait, that lifetime wouldn't make any sense at all.

aturon · 2015-01-16T02:02:39Z

So, I'm basically fine with this as an initial, experimental API.

In the long run, I'd like to nix the _with variance in favor of something more compositional -- like a generic adapter that can work with any timeout-based method + a closure for cancellation. But we can tackle that later. (Basically I would be opposed to making the _with variant #[stable] until we've explored more general solutions.)

alexcrichton · 2015-01-16T05:05:15Z

r=me with a squash

**The implementation is a direct adaptation of libcxx's condition_variable implementation.** pthread_cond_timedwait uses the non-monotonic system clock. It's possible to change the clock to a monotonic via pthread_cond_attr, but this is incompatible with static initialization. To deal with this, we calculate the timeout using the system clock, and maintain a separate record of the start and end times with a monotonic clock to be used for calculation of the return value.

@alexcrichton

**The implementation is a direct adaptation of libcxx's condition_variable implementation.** I also added a wait_timeout_with method, which matches the second overload in C++'s condition_variable. The implementation right now is kind of dumb but it works. There is an outstanding issue with it: as is it doesn't support the use case where a user doesn't care about poisoning and wants to continue through poison. r? @alexcrichton @aturon

@alexcrichton

**The implementation is a direct adaptation of libcxx's condition_variable implementation.** I also added a wait_timeout_with method, which matches the second overload in C++'s condition_variable. The implementation right now is kind of dumb but it works. There is an outstanding issue with it: as is it doesn't support the use case where a user doesn't care about poisoning and wants to continue through poison. r? @alexcrichton @aturon

rust-highfive assigned alexcrichton Jan 14, 2015

sfackler force-pushed the wait_timeout branch from c015489 to 34dd78a Compare January 14, 2015 07:59

alexcrichton reviewed Jan 14, 2015
View reviewed changes

sfackler force-pushed the wait_timeout branch from b69c0e5 to a4d9a7e Compare January 15, 2015 04:42

alexcrichton reviewed Jan 15, 2015
View reviewed changes

sfackler force-pushed the wait_timeout branch from fe909aa to d2d46c1 Compare January 15, 2015 07:08

sfackler force-pushed the wait_timeout branch from 9ae4954 to 36ea376 Compare January 16, 2015 05:15

sfackler force-pushed the wait_timeout branch 3 times, most recently from 9881c4a to 5639f78 Compare January 16, 2015 07:46

sfackler force-pushed the wait_timeout branch from 5639f78 to 08f6380 Compare January 16, 2015 17:17

bors merged commit 08f6380 into rust-lang:master Jan 17, 2015

sfackler mentioned this pull request Jul 26, 2016

Use monotonic time in condition variables. #35048

Merged

sfackler deleted the wait_timeout branch November 26, 2016 05:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul and make Condvar::wait_timeout public #21132

Overhaul and make Condvar::wait_timeout public #21132

sfackler commented Jan 14, 2015

sfackler commented Jan 14, 2015

sfackler commented Jan 14, 2015

alexcrichton Jan 14, 2015

alexcrichton commented Jan 14, 2015

sfackler commented Jan 14, 2015

alexcrichton commented Jan 14, 2015

sfackler commented Jan 14, 2015

alexcrichton commented Jan 14, 2015

sfackler commented Jan 15, 2015

alexcrichton Jan 15, 2015

sfackler commented Jan 15, 2015

alexcrichton Jan 15, 2015

sfackler Jan 15, 2015

alexcrichton commented Jan 15, 2015

sfackler commented Jan 15, 2015

aturon commented Jan 16, 2015

alexcrichton commented Jan 16, 2015

Overhaul and make Condvar::wait_timeout public #21132

Overhaul and make Condvar::wait_timeout public #21132

Conversation

sfackler commented Jan 14, 2015

sfackler commented Jan 14, 2015

sfackler commented Jan 14, 2015

alexcrichton Jan 14, 2015

Choose a reason for hiding this comment

alexcrichton commented Jan 14, 2015

sfackler commented Jan 14, 2015

alexcrichton commented Jan 14, 2015

sfackler commented Jan 14, 2015

alexcrichton commented Jan 14, 2015

sfackler commented Jan 15, 2015

alexcrichton Jan 15, 2015

Choose a reason for hiding this comment

sfackler commented Jan 15, 2015

alexcrichton Jan 15, 2015

Choose a reason for hiding this comment

sfackler Jan 15, 2015

Choose a reason for hiding this comment

alexcrichton commented Jan 15, 2015

sfackler commented Jan 15, 2015

aturon commented Jan 16, 2015

alexcrichton commented Jan 16, 2015