From 0c26b5998d269f58c513fc55ebfdf873be02cc99 Mon Sep 17 00:00:00 2001 From: Stepan Koltsov Date: Mon, 12 Jun 2017 17:19:40 +0300 Subject: [PATCH] Fix condvar.wait(distant future) return immediately on OSX Fixes issue #37440: `pthread_cond_timedwait` on macOS Sierra seems to overflow `ts_sec` parameter and returns immediately. To work around this problem patch rounds timeout down to approximately 1000 years. Patch also fixes overflow when converting `u64` to `time_t`. --- src/libstd/sync/condvar.rs | 66 ++++++++++++++++++++++++++-------- src/libstd/sys/unix/condvar.rs | 34 +++++++++++++++--- 2 files changed, 81 insertions(+), 19 deletions(-) diff --git a/src/libstd/sync/condvar.rs b/src/libstd/sync/condvar.rs index c120a3045e4bb..564021758176b 100644 --- a/src/libstd/sync/condvar.rs +++ b/src/libstd/sync/condvar.rs @@ -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() { @@ -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] diff --git a/src/libstd/sys/unix/condvar.rs b/src/libstd/sys/unix/condvar.rs index 27b9f131d120b..b9ea573b323d2 100644 --- a/src/libstd/sys/unix/condvar.rs +++ b/src/libstd/sys/unix/condvar.rs @@ -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 > ::max_value() as u64 { + ::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 @@ -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; @@ -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. @@ -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)