Fix not unlocking and relocking in condition_variable_any::wait_for (#685)

stl/inc/condition_variable

@@ -27,8 +27,7 @@ _STL_DISABLE_CLANG_WARNINGS
 _STD_BEGIN
 class condition_variable_any { // class for waiting for conditions with any kind of mutex
 public:
-    condition_variable_any() {
+    condition_variable_any() : _Myptr{_STD make_shared<mutex>()} {
-        _Myptr = _STD make_shared<mutex>();
         _Cnd_init_in_situ(_Mycnd());
     }
 
@@ -40,38 +39,58 @@ public:
     condition_variable_any& operator=(const condition_variable_any&) = delete;
 
     void notify_one() noexcept { // wake up one waiter
-        lock_guard<mutex> _Lck(*_Myptr);
+        lock_guard<mutex> _Guard{*_Myptr};
-        _Check_C_return(_Cnd_signal(_Mycnd()));
+        _Cnd_signal(_Mycnd());
     }
 
     void notify_all() noexcept { // wake up all waiters
-        lock_guard<mutex> _Lck(*_Myptr);
+        lock_guard<mutex> _Guard{*_Myptr};
-        _Check_C_return(_Cnd_broadcast(_Mycnd()));
+        _Cnd_broadcast(_Mycnd());
     }
 
-    template <class _Mutex>
+    template <class _Lock>
-    void wait(_Mutex& _Xtrnl) { // wait for signal
+    void wait(_Lock& _Lck) noexcept /* terminates */ { // wait for signal
         {
-            shared_ptr<mutex> _Ptr = _Myptr; // for immunity to *this destruction
+            const shared_ptr<mutex> _Ptr = _Myptr; // for immunity to *this destruction
-            lock_guard<mutex> _Lck(*_Ptr);
+            lock_guard<mutex> _Guard{*_Ptr};
-            _Xtrnl.unlock(); // could throw
+            _Lck.unlock();
-            _Check_C_return(_Cnd_wait(_Mycnd(), _Ptr->_Mymtx()));
+            _Cnd_wait(_Mycnd(), _Ptr->_Mymtx());
         } // unlock
 
-        _Relock(_Xtrnl);
+        _Lck.lock();
     }
 
-    template <class _Mutex, class _Predicate>
+    template <class _Lock, class _Predicate>
-    void wait(_Mutex& _Xtrnl, _Predicate _Pred) { // wait for signal and check predicate
+    void wait(_Lock& _Lck, _Predicate _Pred) noexcept(noexcept(!_Pred())) /* strengthened */ {
+        // wait for signal and check predicate
         while (!_Pred()) {
-            wait(_Xtrnl);
+            wait(_Lck);
         }
     }
 
+    template <class _Lock, class _Clock, class _Duration>
+    cv_status wait_until(_Lock& _Lck, const chrono::time_point<_Clock, _Duration>& _Abs_time) {
+        // wait until time point
+        return wait_for(_Lck, _Abs_time - _Clock::now());
+    }
+
+    template <class _Lock, class _Clock, class _Duration, class _Predicate>
+    bool wait_until(_Lock& _Lck, const chrono::time_point<_Clock, _Duration>& _Abs_time, _Predicate _Pred) {
+        // wait for signal with timeout and check predicate
+        while (!_Pred()) {
+            if (wait_until(_Lck, _Abs_time) == cv_status::timeout) {
+                return _Pred();
+            }
+        }
+
+        return true;
+    }
+
     template <class _Lock, class _Rep, class _Period>
-    cv_status wait_for(_Lock& _Lck, const chrono::duration<_Rep, _Period>& _Rel_time) {
+    cv_status wait_for(_Lock& _Lck, const chrono::duration<_Rep, _Period>& _Rel_time) { // wait for duration
-        // wait for duration
         if (_Rel_time <= chrono::duration<_Rep, _Period>::zero()) {
+            _Lck.unlock();
+            _Relock(_Lck);
             return cv_status::timeout;
         }
 
@@ -79,7 +98,7 @@ public:
         // but unfortunately our ABI speaks struct xtime, which is relative to the system clock.
         _CSTD xtime _Tgt;
         const bool _Clamped     = _To_xtime_10_day_clamped(_Tgt, _Rel_time);
-        const cv_status _Result = wait_until(_Lck, &_Tgt);
+        const cv_status _Result = _Wait_until(_Lck, &_Tgt);
         if (_Clamped) {
             return cv_status::no_timeout;
         }
@@ -90,46 +109,46 @@ public:
     template <class _Lock, class _Rep, class _Period, class _Predicate>
     bool wait_for(_Lock& _Lck, const chrono::duration<_Rep, _Period>& _Rel_time, _Predicate _Pred) {
         // wait for signal with timeout and check predicate
-        return _Wait_until1(_Lck, chrono::steady_clock::now() + _Rel_time, _Pred);
+        return wait_until(_Lck, chrono::steady_clock::now() + _Rel_time, _STD move(_Pred));
     }
 
-    template <class _Lock, class _Clock, class _Duration>
+    template <class _Lock>
-    cv_status wait_until(_Lock& _Lck, const chrono::time_point<_Clock, _Duration>& _Abs_time) {
+    cv_status wait_until(_Lock& _Lck, const xtime* const _Abs_time) { // wait for signal with timeout
-        // wait until time point
+        return _Wait_until(_Lck, _Abs_time);
-        for (;;) {
-            const auto _Now = _Clock::now();
-            if (_Abs_time <= _Now) {
-                return cv_status::timeout;
     }
 
-            _CSTD xtime _Tgt;
+    template <class _Lock, class _Predicate>
-            (void) _To_xtime_10_day_clamped(_Tgt, _Abs_time - _Now);
+    bool wait_until(_Lock& _Lck, const xtime* const _Abs_time, _Predicate _Pred) {
-            const cv_status _Result = wait_until(_Lck, &_Tgt);
-            if (_Result == cv_status::no_timeout) {
-                return cv_status::no_timeout;
-            }
-        }
-    }
-
-    template <class _Lock, class _Clock, class _Duration, class _Predicate>
-    bool wait_until(_Lock& _Lck, const chrono::time_point<_Clock, _Duration>& _Abs_time, _Predicate _Pred) {
         // wait for signal with timeout and check predicate
-        return _Wait_until1(_Lck, _Abs_time, _Pred);
+        while (!_Pred()) {
+            if (_Wait_until(_Lck, _Abs_time) == cv_status::timeout) {
+                return _Pred();
+            }
+        }
+        return true;
     }
 
-    template <class _Mutex>
+private:
-    cv_status wait_until(_Mutex& _Xtrnl, const xtime* _Abs_time) {
+    shared_ptr<mutex> _Myptr;
-        // wait for signal with timeout
+
+    aligned_storage_t<_Cnd_internal_imp_size, _Cnd_internal_imp_alignment> _Cnd_storage;
+
+    _NODISCARD _Cnd_t _Mycnd() noexcept { // get pointer to _Cnd_internal_imp_t inside _Cnd_storage
+        return reinterpret_cast<_Cnd_t>(&_Cnd_storage);
+    }
+
+    template <class _Lock>
+    cv_status _Wait_until(_Lock& _Lck, const xtime* const _Abs_time) { // wait for signal with timeout
         int _Res;
 
         {
-            shared_ptr<mutex> _Ptr = _Myptr; // for immunity to *this destruction
+            const shared_ptr<mutex> _Ptr = _Myptr; // for immunity to *this destruction
-            lock_guard<mutex> _Lck(*_Ptr);
+            lock_guard<mutex> _Guard{*_Ptr};
-            _Xtrnl.unlock(); // could throw
+            _Lck.unlock();
             _Res = _Cnd_timedwait(_Mycnd(), _Ptr->_Mymtx(), _Abs_time);
         } // unlock
 
-        _Relock(_Xtrnl);
+        _Relock(_Lck);
 
         switch (_Res) {
         case _Thrd_success:
@@ -141,57 +160,11 @@ public:
         }
     }
 
-    template <class _Mutex, class _Predicate>
+    template <class _Lock>
-    bool wait_until(_Mutex& _Xtrnl, const xtime* _Abs_time, _Predicate _Pred) {
+    static void _Relock(_Lock& _Lck) noexcept /* terminates */ { // relock external mutex or terminate()
-        // wait for signal with timeout and check predicate
+        // Wait functions are required to terminate if the mutex cannot be locked;
-        return _Wait_until1(_Xtrnl, _Abs_time, _Pred);
+        // we slam into noexcept here for easier user debugging.
-    }
+        _Lck.lock();
-
-private:
-    shared_ptr<mutex> _Myptr;
-
-    aligned_storage_t<_Cnd_internal_imp_size, _Cnd_internal_imp_alignment> _Cnd_storage;
-
-    _Cnd_t _Mycnd() noexcept { // get pointer to _Cnd_internal_imp_t inside _Cnd_storage
-        return reinterpret_cast<_Cnd_t>(&_Cnd_storage);
-    }
-
-    template <class _Mutex>
-    static void _Relock(_Mutex& _Xtrnl) noexcept /* terminates */ {
-        // relocks external mutex, terminate() on failure
-        // LWG-2135 says terminate rather than leaving the mutex unlocked;
-        // we slam into noexcept here for that for easier user debugging
-        _Xtrnl.lock();
-    }
-
-    template <class _Mutex, class _Predicate>
-    bool _Wait_until1(_Mutex& _Xtrnl, const xtime* _Abs_time, _Predicate& _Pred) {
-        // wait for signal with timeout and check predicate, without copying/moving the predicate
-        while (!_Pred()) {
-            if (wait_until(_Xtrnl, _Abs_time) == cv_status::timeout) {
-                return _Pred();
-            }
-        }
-
-        return true;
-    }
-
-    template <class _Mutex, class _Clock, class _Duration, class _Predicate>
-    bool _Wait_until1(_Mutex& _Xtrnl, const chrono::time_point<_Clock, _Duration>& _Abs_time, _Predicate& _Pred) {
-        while (!_Pred()) {
-            const auto _Now = _Clock::now();
-            if (_Abs_time <= _Now) {
-                return false;
-            }
-
-            _CSTD xtime _Tgt;
-            const bool _Clamped = _To_xtime_10_day_clamped(_Tgt, _Abs_time - _Now);
-            if (wait_until(_Xtrnl, &_Tgt) == cv_status::timeout && !_Clamped) {
-                return _Pred();
-            }
-        }
-
-        return true;
     }
 };
 

stl/inc/mutex

@@ -64,7 +64,7 @@ public:
     }
 
     void unlock() {
-        _Check_C_return(_Mtx_unlock(_Mymtx()));
+        _Mtx_unlock(_Mymtx());
     }
 
     using native_handle_type = void*;
@@ -587,16 +587,16 @@ public:
     condition_variable& operator=(const condition_variable&) = delete;
 
     void notify_one() noexcept { // wake up one waiter
-        _Check_C_return(_Cnd_signal(_Mycnd()));
+        _Cnd_signal(_Mycnd());
     }
 
     void notify_all() noexcept { // wake up all waiters
-        _Check_C_return(_Cnd_broadcast(_Mycnd()));
+        _Cnd_broadcast(_Mycnd());
     }
 
     void wait(unique_lock<mutex>& _Lck) { // wait for signal
         // Nothing to do to comply with LWG-2135 because std::mutex lock/unlock are nothrow
-        _Check_C_return(_Cnd_wait(_Mycnd(), _Lck.mutex()->_Mymtx()));
+        _Cnd_wait(_Mycnd(), _Lck.mutex()->_Mymtx());
     }
 
     template <class _Predicate>

stl/inc/xthreads.h

@@ -82,7 +82,7 @@ _CRTIMP2_PURE int __cdecl _Mtx_current_owns(_Mtx_t);
 _CRTIMP2_PURE int __cdecl _Mtx_lock(_Mtx_t);
 _CRTIMP2_PURE int __cdecl _Mtx_trylock(_Mtx_t);
 _CRTIMP2_PURE int __cdecl _Mtx_timedlock(_Mtx_t, const xtime*);
-_CRTIMP2_PURE int __cdecl _Mtx_unlock(_Mtx_t);
+_CRTIMP2_PURE int __cdecl _Mtx_unlock(_Mtx_t); // TRANSITION, ABI: always returns _Thrd_success
 
 _CRTIMP2_PURE void* __cdecl _Mtx_getconcrtcs(_Mtx_t);
 _CRTIMP2_PURE void __cdecl _Mtx_clear_owner(_Mtx_t);
@@ -103,10 +103,10 @@ _CRTIMP2_PURE int __cdecl _Cnd_init(_Cnd_t*);
 _CRTIMP2_PURE void __cdecl _Cnd_destroy(_Cnd_t);
 _CRTIMP2_PURE void __cdecl _Cnd_init_in_situ(_Cnd_t);
 _CRTIMP2_PURE void __cdecl _Cnd_destroy_in_situ(_Cnd_t);
-_CRTIMP2_PURE int __cdecl _Cnd_wait(_Cnd_t, _Mtx_t);
+_CRTIMP2_PURE int __cdecl _Cnd_wait(_Cnd_t, _Mtx_t); // TRANSITION, ABI: Always returns _Thrd_success
 _CRTIMP2_PURE int __cdecl _Cnd_timedwait(_Cnd_t, _Mtx_t, const xtime*);
-_CRTIMP2_PURE int __cdecl _Cnd_broadcast(_Cnd_t);
+_CRTIMP2_PURE int __cdecl _Cnd_broadcast(_Cnd_t); // TRANSITION, ABI: Always returns _Thrd_success
-_CRTIMP2_PURE int __cdecl _Cnd_signal(_Cnd_t);
+_CRTIMP2_PURE int __cdecl _Cnd_signal(_Cnd_t); // TRANSITION, ABI: Always returns _Thrd_success
 _CRTIMP2_PURE void __cdecl _Cnd_register_at_thread_exit(_Cnd_t, _Mtx_t, int*);
 _CRTIMP2_PURE void __cdecl _Cnd_unregister_at_thread_exit(_Mtx_t);
 _CRTIMP2_PURE void __cdecl _Cnd_do_broadcast_at_thread_exit();

stl/src/cond.cpp

@@ -15,7 +15,9 @@ struct _Cnd_internal_imp_t { // condition variable implementation for ConcRT
     std::aligned_storage_t<Concurrency::details::stl_condition_variable_max_size,
         Concurrency::details::stl_condition_variable_max_alignment>
         cv;
-    Concurrency::details::stl_condition_variable_interface* _get_cv() { // get pointer to implementation
+
+    [[nodiscard]] Concurrency::details::stl_condition_variable_interface* _get_cv() noexcept {
+        // get pointer to implementation
         return reinterpret_cast<Concurrency::details::stl_condition_variable_interface*>(&cv);
     }
 };
@@ -24,38 +26,46 @@ static_assert(sizeof(_Cnd_internal_imp_t) <= _Cnd_internal_imp_size, "incorrect
 static_assert(std::alignment_of<_Cnd_internal_imp_t>::value <= _Cnd_internal_imp_alignment,
     "incorrect _Cnd_internal_imp_alignment");
 
-void _Cnd_init_in_situ(_Cnd_t cond) { // initialize condition variable in situ
+void _Cnd_init_in_situ(const _Cnd_t cond) { // initialize condition variable in situ
     Concurrency::details::create_stl_condition_variable(cond->_get_cv());
 }
 
-void _Cnd_destroy_in_situ(_Cnd_t cond) { // destroy condition variable in situ
+void _Cnd_destroy_in_situ(const _Cnd_t cond) { // destroy condition variable in situ
     cond->_get_cv()->destroy();
 }
 
-int _Cnd_init(_Cnd_t* pcond) { // initialize
+int _Cnd_init(_Cnd_t* const pcond) { // initialize
-    _Cnd_t cond;
+    *pcond = nullptr;
-    *pcond = 0;
 
-    if ((cond = static_cast<_Cnd_t>(_calloc_crt(1, sizeof(struct _Cnd_internal_imp_t)))) == 0) {
+    const auto cond = static_cast<_Cnd_t>(_calloc_crt(1, sizeof(_Cnd_internal_imp_t)));
+    if (cond == nullptr) {
         return _Thrd_nomem; // report alloc failed
-    } else { // report success
+    }
+
     _Cnd_init_in_situ(cond);
     *pcond = cond;
     return _Thrd_success;
 }
-}
 
-void _Cnd_destroy(_Cnd_t cond) { // clean up
+void _Cnd_destroy(const _Cnd_t cond) { // clean up
     if (cond) { // something to do, do it
         _Cnd_destroy_in_situ(cond);
         _free_crt(cond);
     }
 }
 
-static int do_wait(_Cnd_t cond, _Mtx_t mtx, const xtime* target) { // wait for signal or timeout
+int _Cnd_wait(const _Cnd_t cond, const _Mtx_t mtx) { // wait until signaled
+    const auto cs = static_cast<Concurrency::details::stl_critical_section_interface*>(_Mtx_getconcrtcs(mtx));
+    _Mtx_clear_owner(mtx);
+    cond->_get_cv()->wait(cs);
+    _Mtx_reset_owner(mtx);
+    return _Thrd_success; // TRANSITION, ABI: Always returns _Thrd_success
+}
+
+int _Cnd_timedwait(const _Cnd_t cond, const _Mtx_t mtx, const xtime* const target) { // wait until signaled or timeout
     int res       = _Thrd_success;
-    auto cs = static_cast<Concurrency::details::stl_critical_section_interface*>(_Mtx_getconcrtcs(mtx));
+    const auto cs = static_cast<Concurrency::details::stl_critical_section_interface*>(_Mtx_getconcrtcs(mtx));
-    if (target == 0) { // no target time specified, wait on mutex
+    if (target == nullptr) { // no target time specified, wait on mutex
         _Mtx_clear_owner(mtx);
         cond->_get_cv()->wait(cs);
         _Mtx_reset_owner(mtx);
@@ -74,30 +84,14 @@ static int do_wait(_Cnd_t cond, _Mtx_t mtx, const xtime* target) { // wait for s
     return res;
 }
 
-static int do_signal(_Cnd_t cond, int all) { // release threads
+int _Cnd_signal(const _Cnd_t cond) { // release one waiting thread
-    if (all) {
-        cond->_get_cv()->notify_all();
-    } else {
     cond->_get_cv()->notify_one();
+    return _Thrd_success; // TRANSITION, ABI: Always returns _Thrd_success
 }
 
-    return _Thrd_success;
+int _Cnd_broadcast(const _Cnd_t cond) { // release all waiting threads
-}
+    cond->_get_cv()->notify_all();
-
+    return _Thrd_success; // TRANSITION, ABI: Always returns _Thrd_success
-int _Cnd_wait(_Cnd_t cond, _Mtx_t mtx) { // wait until signaled
-    return do_wait(cond, mtx, 0);
-}
-
-int _Cnd_timedwait(_Cnd_t cond, _Mtx_t mtx, const xtime* xt) { // wait until signaled or timeout
-    return do_wait(cond, mtx, xt);
-}
-
-int _Cnd_signal(_Cnd_t cond) { // release one waiting thread
-    return do_signal(cond, 0);
-}
-
-int _Cnd_broadcast(_Cnd_t cond) { // release all waiting threads
-    return do_signal(cond, 1);
 }
 
 /*

stl/src/mutex.cpp

@@ -168,7 +168,7 @@ int _Mtx_unlock(_Mtx_t mtx) { // unlock mutex
         mtx->thread_id = -1;
         mtx->_get_cs()->unlock();
     }
-    return _Thrd_success;
+    return _Thrd_success; // TRANSITION, ABI: always returns _Thrd_success
 }
 
 int _Mtx_lock(_Mtx_t mtx) { // lock mutex

tests/std/test.lst

@@ -157,6 +157,7 @@ tests\Dev11_1158803_regex_thread_safety
 tests\Dev11_1180290_filesystem_error_code
 tests\GH_000457_system_error_message
 tests\GH_000545_include_compare
+tests\GH_000685_condition_variable_any
 tests\GH_000690_overaligned_function
 tests\LWG3018_shared_ptr_function
 tests\P0024R2_parallel_algorithms_adjacent_difference

tests/std/tests/GH_000685_condition_variable_any/env.lst

@@ -0,0 +1,4 @@
+# Copyright (c) Microsoft Corporation.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+RUNALL_INCLUDE ..\native_matrix.lst

tests/std/tests/GH_000685_condition_variable_any/test.cpp

@@ -0,0 +1,60 @@
+// Copyright (c) Microsoft Corporation.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+// Test GH-685 "wait_for in condition_variable_any should unlock and lock"
+
+#include <cassert>
+#include <chrono>
+#include <condition_variable>
+#include <mutex>
+
+using namespace std;
+
+namespace {
+    class my_mutex { // user-defined mutex type
+    public:
+        void lock() {
+            mtx.lock();
+            ++num_lock;
+        }
+
+        void unlock() {
+            mtx.unlock();
+        }
+
+        int num_locks() const {
+            return num_lock;
+        }
+
+    private:
+        mutex mtx;
+        int num_lock = 0;
+    };
+
+    const chrono::nanoseconds interval{20};
+    const chrono::nanoseconds zero_dur{0};
+
+    void test_condition_variable_any() { // test wait functions of condition variables
+        condition_variable_any cnd;
+        {
+            my_mutex mtx;
+            unique_lock<my_mutex> guard{mtx};
+
+            cnd.wait_for(mtx, zero_dur);
+            assert(mtx.num_locks() == 2);
+
+            cnd.wait_for(mtx, interval);
+            assert(mtx.num_locks() == 3);
+
+            cnd.wait_until(mtx, chrono::steady_clock::now());
+            assert(mtx.num_locks() == 4);
+
+            cnd.wait_until(mtx, chrono::steady_clock::now() + interval);
+            assert(mtx.num_locks() == 5);
+        }
+    }
+} // unnamed namespace
+
+int main() {
+    test_condition_variable_any();
+}

tests/tr1/tests/chrono/test.cpp

@@ -297,7 +297,7 @@ namespace {
         }
     }
 
-} // anonymous namespace
+} // unnamed namespace
 
 void test_main() {
     t_common_type();

tests/tr1/tests/condition_variable/test.cpp

@@ -13,7 +13,7 @@
 #include <system_error>
 #include <thread>
 
-namespace { // anonymous namespace
+namespace {
     int count;
     int value;
     STD mutex sync_mutex;
@@ -309,7 +309,7 @@ namespace { // anonymous namespace
         my_mutex mtx4;
         t_condition_variables(cnd, mtx4, "my_mutex");
     }
-} // anonymous namespace
+} // unnamed namespace
 
 void test_main() { // test header <condition_variable>
     t_condition_variable();

tests/tr1/tests/ratio/test.cpp

@@ -359,7 +359,7 @@ namespace {
         CHECK_INT(yotta_test::value, true);
 #endif // 1000000 <= INTMAX_MAX / 1000000000000000000
     }
-} // anonymous namespace
+} // unnamed namespace
 
 void test_main() { // test header <ratio>
     t_ratio();

tests/tr1/tests/thread/test.cpp

@@ -341,7 +341,7 @@ namespace {
             CHECK_INT(tgt <= STD chrono::system_clock::now(), true);
         }
     }
-} // anonymous namespace
+} // unnamed namespace
 
 void test_main() { // test header <thread>
     if (!terse) { // display value of __STDCPP_THREADS__