Use monotonic time in condition variables. #35048
Conversation
|
Any backcompat hazards? Is there anybody who could be relying on the current behaviour? |
|
The only observable change should occur when system time is moved backwards. I don't think anyone could rely on previous behaviour, especially considering |
|
The implementation was pulled pretty directly from C++ standard libraries - do you know why they don't do something similar? |
|
Specifically libcxx: #21132 |
|
@sfackler r? self if you’re interested in reviewing this. hfive didn’t assign anybody for some reason. |
sfackler
added
I-nominated
T-libs-api
|
Yep, I'll grab it. |
| @@ -18,14 +20,14 @@ use sys::condvar as imp; | |||
| /// condition variables. It is consequently entirely unsafe to use. It is | |||
| /// recommended to use the safer types at the top level of this crate instead of | |||
| /// this type. | |||
| pub struct Condvar(imp::Condvar); | |||
| pub struct Condvar(Box<imp::Condvar>); | |||
There was a problem hiding this comment.
The initial intention of these sys_common types was to be a thin a layer over the system primitives as possible, unsafety an all. Could this box be left upwards and the initialization be delayed like with mutexes?
There was a problem hiding this comment.
@alexcrichton There are two main reasons for moving the box. First, it seemed
preferable to me, to ensure that Condvar::new() returns fully initialized
object (especially now given that StaticCondvar is gone). Second, it didn't
require chasing all uses of Condvar::new() and inserting init nearby.
If you prefer to have boxes outside sys_common I can of course move them back.
There was a problem hiding this comment.
I'd rather the Box not be here because it is punishing Windows for something that isn't its fault.
There was a problem hiding this comment.
@retep998 This PR does not add any additional boxes. It just has been moved from sync::Condvar::inner.
There was a problem hiding this comment.
Ah, well it certainly confused me.
There was a problem hiding this comment.
@tmiasko yeah let's keep the boxes instd::sync to stay consistent with the mutex module next to this one.
|
@sfackler C++ story is much more complicated because std::condition_variable |
| fn drop(&mut self) { | ||
| unsafe { libc::pthread_condattr_destroy(self.0) }; | ||
| } | ||
| } |
There was a problem hiding this comment.
I think it's fine to not worry about assertions below cleaning this up, this can just be inserted after the pthread_cond_init (like mutex initialization)
|
I reverted box changes and added init function to sys::Condvar instead. |
|
☔ The latest upstream changes (presumably #35060) made this pull request unmergeable. Please resolve the merge conflicts. |
Configure condition variables to use monotonic time using pthread_condattr_setclock on systems where this is possible. This fixes the issue when thread waiting on condition variable is woken up too late when system time is moved backwards.
|
I rebased changes against master. The commit updating liblibc is gone as it is no longer necessary. |
IIRC, the implementations in Boost and GCC's standard library are basically the same. |
|
Boost implementation was changed to use monotonic clock in version 1.60: It seems that similar solution is considered in GNU libstdc++ |
|
The libs team discussed this at the last triage meeting, and decided that we're on board with this change, but would possibly like the documentation for the Could you add that bit on? We should be good to merge afterwards! |
|
I have updated the docs for
|
|
📌 Commit 8dae1b6 has been approved by |
Use monotonic time in condition variables. Configure condition variables to use monotonic time using pthread_condattr_setclock on systems where this is possible. This fixes the issue when thread waiting on condition variable is woken up too late when system time is moved backwards.
|
💔 Test failed - auto-linux-64-x-android-t |
The pthread_condattr_setclock is available only since Android 5.0 and API level 21.
|
@tmiasko good for another bors run? |
|
The android build failed to link with pthread_condattr_setclock. The I am not sure if there is some minimum Android version supported by Rust, and @sfackler If that is ok with you, then yes, it is ready for another bors run. |
|
@bors r=alexcrichton |
|
📌 Commit 59e5e0b has been approved by |
|
Cool! I think the minimum Android API level we support is somewhere in the teens unfortunately. |
|
⌛ Testing commit 59e5e0b with merge b953acf... |
|
💔 Test failed - auto-mac-64-opt-rustbuild |
|
@bors: retry |
|
⌛ Testing commit 59e5e0b with merge e797b8f... |
|
⛄ The build was interrupted to prioritize another pull request. |
|
⌛ Testing commit 59e5e0b with merge e08f211... |
|
@bors: retry force clean
|
|
💣 Buildbot returned an error: |
|
💔 Test failed - auto-mac-32-opt |
|
@bors: retry On Tue, Aug 30, 2016 at 11:27 AM, bors [email protected] wrote:
|
|
⌛ Testing commit 59e5e0b with merge 86c6c1c... |
|
@bors: retry force clean
|
Use monotonic time in condition variables. Configure condition variables to use monotonic time using pthread_condattr_setclock on systems where this is possible. This fixes the issue when thread waiting on condition variable is woken up too late when system time is moved backwards.
|
On Android, you might want to optionally use pthread_cond_timedwait_monotonic_np() as pthread_condattr_setclock() was only added with Android API 21 (5.0 / Lollipop), and pthread_cond_timedwait_monotonic() was using the wallclock. If that's something you would consider (instead of just not supporting Android pre 21 properly), I could probably make a patch for this. |
ghost
deleted the
Configure condition variables to use monotonic time using
pthread_condattr_setclock on systems where this is possible.
This fixes the issue when thread waiting on condition variable is
woken up too late when system time is moved backwards.