Wait condition_variable_any for steady_clock (#4755)

Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>
microsoft · Aug 8, 2024 · 2f5b967 · 2f5b967
1 parent eaf7b31
commit 2f5b967
Show file tree

Hide file tree

Showing 5 changed files with 54 additions and 46 deletions.
diff --git a/stl/inc/condition_variable b/stl/inc/condition_variable
@@ -123,12 +123,26 @@ public:
         }
 
         const auto _Rel_time_ms = _Clamped_rel_time_ms_count(_Rel_time);
-        const cv_status _Result = _Wait_for_ms_count(_Lck, _Rel_time_ms._Count);
-        if (_Rel_time_ms._Clamped) {
-            return cv_status::no_timeout;
-        }
 
-        return _Result;
+        // wait for signal with timeout
+        const shared_ptr<mutex> _Ptr = _Myptr; // for immunity to *this destruction
+        const auto _Start            = _CHRONO steady_clock::now();
+        const auto _Target           = _Start + _Rel_time;
+
+        unique_lock<mutex> _Guard{*_Ptr};
+        _Unlock_guard<_Lock> _Unlock_outer{_Lck};
+        const _Thrd_result _Res = _Cnd_timedwait_for_unchecked(_Mycnd(), _Ptr->_Mymtx(), _Rel_time_ms._Count);
+        _Guard.unlock();
+
+        if (_Res == _Thrd_result::_Success) {
+            return cv_status::no_timeout; // Real or WinAPI-internal spurious wake
+        } else if (_Rel_time_ms._Clamped) {
+            return cv_status::no_timeout; // Spuriously wake due to clamped timeout
+        } else if (_CHRONO steady_clock::now() < _Target) {
+            return cv_status::no_timeout; // Spuriously wake due to premature timeout
+        } else {
+            return cv_status::timeout;
+        }
     }
 
     template <class _Lock, class _Rep, class _Period, class _Predicate>
@@ -202,7 +216,7 @@ public:
             }
 
             const unsigned long _Rel_ms_count = _Clamped_rel_time_ms_count(_Abs_time - _Now)._Count;
-            (void) _Cnd_timedwait_for(_Mycnd(), _Myptr->_Mymtx(), _Rel_ms_count);
+            (void) _Cnd_timedwait_for_unchecked(_Mycnd(), _Myptr->_Mymtx(), _Rel_ms_count);
             _Guard_unlocks_before_locking_outer.unlock();
         } // relock
 
@@ -223,22 +237,6 @@ private:
     _NODISCARD _Cnd_t _Mycnd() noexcept {
         return &_Cnd_storage;
     }
-
-    template <class _Lock>
-    cv_status _Wait_for_ms_count(_Lock& _Lck, const unsigned int _Rel_ms_count) {
-        // wait for signal with timeout
-        const shared_ptr<mutex> _Ptr = _Myptr; // for immunity to *this destruction
-        unique_lock<mutex> _Guard{*_Ptr};
-        _Unlock_guard<_Lock> _Unlock_outer{_Lck};
-        const _Thrd_result _Res = _Cnd_timedwait_for(_Mycnd(), _Ptr->_Mymtx(), _Rel_ms_count);
-        _Guard.unlock();
-
-        if (_Res == _Thrd_result::_Success) {
-            return cv_status::no_timeout;
-        } else {
-            return cv_status::timeout;
-        }
-    }
 };
 
 _EXPORT_STD inline void notify_all_at_thread_exit(condition_variable& _Cnd, unique_lock<mutex> _Lck) noexcept

diff --git a/stl/inc/mutex b/stl/inc/mutex
@@ -599,7 +599,7 @@ public:
 
             const unsigned long _Rel_ms_count = _Clamped_rel_time_ms_count(_Abs_time - _Now)._Count;
 
-            const _Thrd_result _Res = _Cnd_timedwait_for(_Mycnd(), _Lck.mutex()->_Mymtx(), _Rel_ms_count);
+            const _Thrd_result _Res = _Cnd_timedwait_for_unchecked(_Mycnd(), _Lck.mutex()->_Mymtx(), _Rel_ms_count);
             if (_Res == _Thrd_result::_Success) {
                 return cv_status::no_timeout;
             }

diff --git a/stl/inc/xthreads.h b/stl/inc/xthreads.h
@@ -62,7 +62,8 @@ _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_signal(_Cnd_t) noexcept; // TRANSITION,
 _CRTIMP2_PURE void __cdecl _Cnd_register_at_thread_exit(_Cnd_t, _Mtx_t, int*) noexcept;
 _CRTIMP2_PURE void __cdecl _Cnd_unregister_at_thread_exit(_Mtx_t) noexcept;
 _CRTIMP2_PURE void __cdecl _Cnd_do_broadcast_at_thread_exit() noexcept;
-_Thrd_result __stdcall _Cnd_timedwait_for(_Cnd_t, _Mtx_t, unsigned int) noexcept;
+// '_unchecked' means it is not checked against the 'steady_clock', so may report timeout prematurely
+_Thrd_result __stdcall _Cnd_timedwait_for_unchecked(_Cnd_t, _Mtx_t, unsigned int) noexcept;
 } // extern "C"
 
 _STD_BEGIN

diff --git a/stl/src/sharedmutex.cpp b/stl/src/sharedmutex.cpp
@@ -41,25 +41,42 @@ void __stdcall _Thrd_sleep_for(const unsigned long ms) noexcept { // suspend cur
     Sleep(ms);
 }
 
-_Thrd_result __stdcall _Cnd_timedwait_for(const _Cnd_t cond, const _Mtx_t mtx, const unsigned int target_ms) noexcept {
-    _Thrd_result res    = _Thrd_result::_Success;
-    const auto start_ms = GetTickCount64();
+namespace {
+    _Thrd_result __stdcall _Cnd_timedwait_for_impl(
+        const _Cnd_t cond, const _Mtx_t mtx, const unsigned int target_ms, const bool checked) noexcept {
+        _Thrd_result res            = _Thrd_result::_Success;
+        unsigned long long start_ms = 0;
+
+        if (checked) {
+            start_ms = GetTickCount64();
+        }
 
-    // TRANSITION: replace with _Mtx_clear_owner(mtx);
-    mtx->_Thread_id = -1;
-    --mtx->_Count;
+        // TRANSITION: replace with _Mtx_clear_owner(mtx);
+        mtx->_Thread_id = -1;
+        --mtx->_Count;
 
-    if (!_Primitive_wait_for(cond, mtx, target_ms)) { // report timeout
-        const auto end_ms = GetTickCount64();
-        if (end_ms - start_ms >= target_ms) {
-            res = _Thrd_result::_Timedout;
+        if (!_Primitive_wait_for(cond, mtx, target_ms)) { // report timeout
+            if (!checked || GetTickCount64() - start_ms >= target_ms) {
+                res = _Thrd_result::_Timedout;
+            }
         }
+        // TRANSITION: replace with _Mtx_reset_owner(mtx);
+        mtx->_Thread_id = static_cast<long>(GetCurrentThreadId());
+        ++mtx->_Count;
+
+        return res;
     }
-    // TRANSITION: replace with _Mtx_reset_owner(mtx);
-    mtx->_Thread_id = static_cast<long>(GetCurrentThreadId());
-    ++mtx->_Count;
+} // namespace
 
-    return res;
+// TRANSITION, ABI: preserved for compatibility
+_Thrd_result __stdcall _Cnd_timedwait_for(const _Cnd_t cond, const _Mtx_t mtx, const unsigned int target_ms) noexcept {
+    return _Cnd_timedwait_for_impl(cond, mtx, target_ms, true);
 }
 
+_Thrd_result __stdcall _Cnd_timedwait_for_unchecked(
+    const _Cnd_t cond, const _Mtx_t mtx, const unsigned int target_ms) noexcept {
+    return _Cnd_timedwait_for_impl(cond, mtx, target_ms, false);
+}
+
+
 } // extern "C"
diff --git a/tests/libcxx/expected_results.txt b/tests/libcxx/expected_results.txt
@@ -467,14 +467,6 @@ std/input.output/string.streams/stringbuf/stringbuf.members/view.pass.cpp FAIL
 std/input.output/syncstream/syncbuf/syncstream.syncbuf.cons/dtor.pass.cpp FAIL
 std/input.output/syncstream/syncbuf/syncstream.syncbuf.members/emit.pass.cpp FAIL
 
-# GH-4723: <condition_variable>: condition_variable_any::wait_for returns cv_status::timeout when the elapsed time is shorter than requested
-std/thread/thread.condition/thread.condition.condvarany/notify_all.pass.cpp SKIPPED
-std/thread/thread.condition/thread.condition.condvarany/notify_one.pass.cpp SKIPPED
-std/thread/thread.condition/thread.condition.condvarany/wait_for_pred.pass.cpp SKIPPED
-std/thread/thread.condition/thread.condition.condvarany/wait_for.pass.cpp SKIPPED
-std/thread/thread.condition/thread.condition.condvarany/wait_until_pred.pass.cpp SKIPPED
-std/thread/thread.condition/thread.condition.condvarany/wait_until.pass.cpp SKIPPED
-
 
 # *** VCRUNTIME BUGS ***
 # DevCom-10373274 VSO-1824997 "vcruntime nothrow array operator new falls back on the wrong function"