Overhaul and make Condvar::wait_timeout public

[sfackler]

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]

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]

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]

Could this use .checked_add to check for overflow instead?

[alexcrichton]

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]

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]

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]

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]

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]

Just pushed fixes up in a new commit.

[alexcrichton]

I think this was lost in movement to a different location

[sfackler]

Updated

[alexcrichton]

Oh sorry when I said &mut T I meant:

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

[sfackler]

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

[alexcrichton]

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

[sfackler]

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]

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]

r=me with a squash