Skip to content

Fix condvar.wait(distant future) return immediately on OSX #42604

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

Merged
merged 1 commit into from
Jun 17, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 51 additions & 15 deletions src/libstd/sync/condvar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -480,9 +480,10 @@ impl Drop for Condvar {
mod tests {
use sync::mpsc::channel;
use sync::{Condvar, Mutex, Arc};
use sync::atomic::{AtomicBool, Ordering};
use thread;
use time::Duration;
use u32;
use u64;

#[test]
fn smoke() {
Expand Down Expand Up @@ -547,23 +548,58 @@ mod tests {

#[test]
#[cfg_attr(target_os = "emscripten", ignore)]
fn wait_timeout_ms() {
fn wait_timeout_wait() {
let m = Arc::new(Mutex::new(()));
let m2 = m.clone();
let c = Arc::new(Condvar::new());
let c2 = c.clone();

let g = m.lock().unwrap();
let (g, _no_timeout) = c.wait_timeout(g, Duration::from_millis(1)).unwrap();
// spurious wakeups mean this isn't necessarily true
// assert!(!no_timeout);
let _t = thread::spawn(move || {
let _g = m2.lock().unwrap();
c2.notify_one();
});
let (g, timeout_res) = c.wait_timeout(g, Duration::from_millis(u32::MAX as u64)).unwrap();
assert!(!timeout_res.timed_out());
drop(g);
loop {
let g = m.lock().unwrap();
let (_g, no_timeout) = c.wait_timeout(g, Duration::from_millis(1)).unwrap();
// spurious wakeups mean this isn't necessarily true
// so execute test again, if not timeout
if !no_timeout.timed_out() {
continue;
}

break;
}
}

#[test]
#[cfg_attr(target_os = "emscripten", ignore)]
fn wait_timeout_wake() {
let m = Arc::new(Mutex::new(()));
let c = Arc::new(Condvar::new());

loop {
let g = m.lock().unwrap();

let c2 = c.clone();
let m2 = m.clone();

let notified = Arc::new(AtomicBool::new(false));
let notified_copy = notified.clone();

let t = thread::spawn(move || {
let _g = m2.lock().unwrap();
thread::sleep(Duration::from_millis(1));
notified_copy.store(true, Ordering::SeqCst);
c2.notify_one();
});
let (g, timeout_res) = c.wait_timeout(g, Duration::from_millis(u64::MAX)).unwrap();
assert!(!timeout_res.timed_out());
// spurious wakeups mean this isn't necessarily true
// so execute test again, if not notified
if !notified.load(Ordering::SeqCst) {
t.join().unwrap();
continue;
}
drop(g);

t.join().unwrap();

break;
}
}

#[test]
Expand Down
34 changes: 30 additions & 4 deletions src/libstd/sys/unix/condvar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,14 @@ const TIMESPEC_MAX: libc::timespec = libc::timespec {
tv_nsec: 1_000_000_000 - 1,
};

fn saturating_cast_to_time_t(value: u64) -> libc::time_t {
if value > <libc::time_t>::max_value() as u64 {
<libc::time_t>::max_value()
} else {
value as libc::time_t
}
}

impl Condvar {
pub const fn new() -> Condvar {
// Might be moved and address is changing it is better to avoid
Expand Down Expand Up @@ -79,8 +87,7 @@ impl Condvar {

// Nanosecond calculations can't overflow because both values are below 1e9.
let nsec = dur.subsec_nanos() as libc::c_long + now.tv_nsec as libc::c_long;
// FIXME: Casting u64 into time_t could truncate the value.
let sec = (dur.as_secs() as libc::time_t)
let sec = saturating_cast_to_time_t(dur.as_secs())
.checked_add((nsec / 1_000_000_000) as libc::time_t)
.and_then(|s| s.checked_add(now.tv_sec));
let nsec = nsec % 1_000_000_000;
Expand All @@ -100,10 +107,29 @@ impl Condvar {
// https://github.com/llvm-mirror/libcxx/blob/release_35/src/condition_variable.cpp#L46
// https://github.com/llvm-mirror/libcxx/blob/release_35/include/__mutex_base#L367
#[cfg(any(target_os = "macos", target_os = "ios", target_os = "android"))]
pub unsafe fn wait_timeout(&self, mutex: &Mutex, dur: Duration) -> bool {
pub unsafe fn wait_timeout(&self, mutex: &Mutex, mut dur: Duration) -> bool {
use ptr;
use time::Instant;

// 1000 years
let max_dur = Duration::from_secs(1000 * 365 * 86400);

if dur > max_dur {
// OSX implementation of `pthread_cond_timedwait` is buggy
// with super long durations. When duration is greater than
// 0x100_0000_0000_0000 seconds, `pthread_cond_timedwait`
// in macOS Sierra return error 316.
//
// This program demonstrates the issue:
// https://gist.github.com/stepancheg/198db4623a20aad2ad7cddb8fda4a63c
//
// To work around this issue, and possible bugs of other OSes, timeout
// is clamped to 1000 years, which is allowable per the API of `wait_timeout`
// because of spurious wakeups.

dur = max_dur;
}

// First, figure out what time it currently is, in both system and
// stable time. pthread_cond_timedwait uses system time, but we want to
// report timeout based on stable time.
Expand All @@ -116,7 +142,7 @@ impl Condvar {
(sys_now.tv_usec * 1000) as libc::c_long;
let extra = (nsec / 1_000_000_000) as libc::time_t;
let nsec = nsec % 1_000_000_000;
let seconds = dur.as_secs() as libc::time_t;
let seconds = saturating_cast_to_time_t(dur.as_secs());

let timeout = sys_now.tv_sec.checked_add(extra).and_then(|s| {
s.checked_add(seconds)
Expand Down