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

stepancheg · 2017-06-12T16:01:37Z

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 year 3000.

Patch also fixes overflow when converting u64 to time_t.

nagisa · 2017-06-12T19:25:00Z

Is year 3000 special in that sense that this is a maximum OS X will support?

stepancheg · 2017-06-12T20:01:01Z

@nagisa OSX pthread_cond_timedwait breaks near year 3170843 on my notebook. But I'm not sure about behavior on other versions of OSX or iPhone, so I decided it is safer to round down to year 3000. I hope in a couple of centuries Apple will fix this issue, and this rounding down could be removed from rust std source.

stepancheg · 2017-06-12T20:51:17Z

r? @alexcrichton

alexcrichton

Do you have an evidence of where the overflow is? The apparent source code for pthread_cond_timedwait shells out to _pthread_cond_wait which doesn't look like it has any overflows?

Using dtruss indicates that psynch_cvwait is failing with error 316. Headers don't seem to indicate what that is and related StackOverflow questions seem to also be similarly confused.

It seems that we don't know why this is failing and we're just blindly applying a workaround?

alexcrichton · 2017-06-13T03:57:14Z

src/libstd/sync/condvar.rs

-        let (g, timeout_res) = c.wait_timeout(g, Duration::from_millis(u32::MAX as u64)).unwrap();
+        let (g, timeout_res) = c.wait_timeout(g, Duration::from_millis(u64::MAX)).unwrap();
+        // spurious wakeups mean this isn't necessarily true
+        // assert!(notified.load(Ordering::SeqCst));


If we can't have this assert, why have the Arc?

So next person going to patch this part of the library could execute test with this line uncommented. I believe that prevous comment "spurious wakeups mean this isn't necessarily true" several lines above kept in the source for the same reason.

Can this be removed in that case? It's surely easier to test this outside the standard library than inside, so this seems like it's just an extra diff for not a lot of value.

stepancheg · 2017-06-13T19:40:50Z

Do you have an evidence of where the overflow is? The apparent source code for pthread_cond_timedwait shells out to _pthread_cond_wait which doesn't look like it has any overflows?

I believe, overflow is somewhere inside kernel. I couldn't found sources of latest kernel, but there's a source of 10.7.5.

It has code like this:

nanoseconds_to_absolutetime((uint64_t)ts.tv_sec * NSEC_PER_SEC + ts.tv_nsec,  &abstime );

which does not explain this particular issue, but shows that authors of xnu don't care much about integer overflows.

Using dtruss indicates that psynch_cvwait is failing with error 316.

Or maybe it is not overflow, but an indication of parameter error. Whatever.

It seems that we don't know why this is failing and we're just blindly applying a workaround?

That's fine with me.

BTW, now I think that it would probably be slightly better to wait with explicitly infinite timeout if duration is more than 1000 years, instead of rounding down to year 3000, but this version is OK too.

stepancheg · 2017-06-13T19:58:17Z

BTW, now I think that it would probably be slightly better to wait with explicitly infinite timeout if duration is more than 1000 years, instead of rounding down to year 3000, but this version is OK too.

No, rounding down is legal, because cond.wait is allowed to wake up spuriously.

stepancheg · 2017-06-13T20:03:11Z

Going to update patch now to round duration down to 1000 years instead of rounding down deadline to year 3000.

stepancheg · 2017-06-13T20:17:11Z

Done.

alexcrichton · 2017-06-13T21:37:30Z

src/libstd/sys/unix/condvar.rs

@@ -116,7 +173,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 = wrapping_cast_to_time_t(dur.as_secs());


This should no longer be necessary due to the clamp above, right?

Yes, if time_t is more than 32 bit (as in all modern OSes), this cast is not needed.

However, I'd keep this cast, because

it is not performance critical

makes code easier to read (reader does not need to check that there's no overflow)

makes OSX/Linux versions more like to each other

alexcrichton · 2017-06-13T21:37:58Z

src/libstd/sys/unix/condvar.rs

+            //         println!("$");
+            //     }
+            // }
+            // ```


This is a pretty long comment to work with, could the program be extracted to something like a gist? Could the example program be C and not Rust?

Rewritten in C and moved to gist.

alexcrichton · 2017-06-13T21:39:23Z

src/libstd/sys/unix/condvar.rs

+        let max_dur = Duration::from_secs(1000 * 365 * 86400);
+
+        if dur > max_dur {
+            // Apparently `tv_sec` overflows somewhere around


Are we 100% sure that this is an overflow somewhere? The fact that the kernel returns an error seems to indicate that this comment is not correct. I think we can basically just say that OSX is buggy with super long durations, so we clamp which is also allowable per the API of wait_timeout because of spurious wakeups.

Updated comment to use diffent words.

alexcrichton · 2017-06-13T21:39:56Z

src/libstd/sync/condvar.rs

-        let (g, timeout_res) = c.wait_timeout(g, Duration::from_millis(u32::MAX as u64)).unwrap();
+        let (g, timeout_res) = c.wait_timeout(g, Duration::from_millis(u64::MAX)).unwrap();
+        // spurious wakeups mean this isn't necessarily true
+        // assert!(notified.load(Ordering::SeqCst));


Can this be removed in that case? It's surely easier to test this outside the standard library than inside, so this seems like it's just an extra diff for not a lot of value.

Fixes issue rust-lang#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`.

stepancheg · 2017-06-15T21:53:52Z

Can this be removed in that case? It's surely easier to test this outside the standard library than inside, so this seems like it's just an extra diff for not a lot of value.

I've rewritten tests with loop until wake up is no spurious.

alexcrichton · 2017-06-16T14:42:17Z

@bors: r+

bors · 2017-06-16T14:42:18Z

📌 Commit 0c26b59 has been approved by alexcrichton

bors · 2017-06-17T00:05:56Z

⌛ Testing commit 0c26b59 with merge 1169a1f...

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 year 3000. Patch also fixes overflow when converting `u64` to `time_t`.

bors · 2017-06-17T02:21:33Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 1169a1f to master...

stepancheg force-pushed the timedwait branch from dbf0708 to 709cfe4 Compare June 12, 2017 16:06

rust-highfive assigned alexcrichton Jun 12, 2017

alexcrichton reviewed Jun 13, 2017

View reviewed changes

stepancheg force-pushed the timedwait branch from 709cfe4 to 24f915b Compare June 13, 2017 20:17

arielb1 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 13, 2017

alexcrichton reviewed Jun 13, 2017

View reviewed changes

stepancheg force-pushed the timedwait branch from 24f915b to 0c26b59 Compare June 15, 2017 21:53

bors merged commit 0c26b59 into rust-lang:master Jun 17, 2017

Mark-Simulacrum mentioned this pull request Jun 17, 2017

Unexpected behavior using std::thread::park_timeout on OSX #37440

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

stepancheg commented Jun 12, 2017

nagisa commented Jun 12, 2017

stepancheg commented Jun 12, 2017

stepancheg commented Jun 12, 2017

alexcrichton left a comment

alexcrichton Jun 13, 2017

stepancheg Jun 13, 2017

alexcrichton Jun 13, 2017

stepancheg commented Jun 13, 2017

stepancheg commented Jun 13, 2017

stepancheg commented Jun 13, 2017

stepancheg commented Jun 13, 2017

alexcrichton Jun 13, 2017

stepancheg Jun 15, 2017

alexcrichton Jun 13, 2017

stepancheg Jun 15, 2017

alexcrichton Jun 13, 2017

stepancheg Jun 15, 2017

alexcrichton Jun 13, 2017

stepancheg commented Jun 15, 2017

alexcrichton commented Jun 16, 2017

bors commented Jun 16, 2017

bors commented Jun 17, 2017

bors commented Jun 17, 2017

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

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

Conversation

stepancheg commented Jun 12, 2017

nagisa commented Jun 12, 2017

stepancheg commented Jun 12, 2017

stepancheg commented Jun 12, 2017

alexcrichton left a 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

stepancheg commented Jun 13, 2017

stepancheg commented Jun 13, 2017

stepancheg commented Jun 13, 2017

stepancheg commented Jun 13, 2017

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

stepancheg commented Jun 15, 2017

alexcrichton commented Jun 16, 2017

bors commented Jun 16, 2017

bors commented Jun 17, 2017

bors commented Jun 17, 2017