Fixing use of invoke_wating() to wrap the mutex lock.

stlab · Jan 25, 2025 · 9b84be9 · 9b84be9
1 parent d59c4c1
commit 9b84be9
Show file tree

Hide file tree

Showing 5 changed files with 42 additions and 21 deletions.
diff --git a/CMakePresets.json b/CMakePresets.json
@@ -57,6 +57,18 @@
                 "CMAKE_BUILD_TYPE": "DEBUG"
             }
         },
+        {
+            "name": "xcode-cpp20-debug-portable",
+            "description": "",
+            "hidden": false,
+            "generator": "Xcode",
+            "binaryDir": "${sourceDir}/build/xcode-cpp20-debug-portable",
+            "cacheVariables": {
+                "CMAKE_CXX_STANDARD": "20",
+                "CMAKE_BUILD_TYPE": "DEBUG",
+                "STLAB_TASK_SYSTEM": "portable"
+            }
+        },
         {
             "name": "ninja-cpp17-debug",
             "description": "Ninja Debug Build",

diff --git a/include/stlab/concurrency/await.hpp b/include/stlab/concurrency/await.hpp
@@ -36,8 +36,9 @@ inline namespace STLAB_VERSION_NAMESPACE() {
 /**************************************************************************************************/
 
 /**
-    Assumes f _will wait_ and wakes or adds a thread to the thread pool (to the limit) before
-    invoking f.
+    Assumes `f` _will wait_ and wakes or adds a thread to the thread pool (to the limit) before
+    invoking `f`. If using a condition variable, wrap the duration of the mutex lock in `f` to avoid
+    deadlocks.
 */
 template <class F>
 auto invoke_waiting(F&& f) {
@@ -69,9 +70,10 @@ auto await(future<T>&& x) -> T {
             condition.notify_one(); // must notify under lock
         }
     });
-
-    std::unique_lock<std::mutex> lock{m};
-    invoke_waiting([&] { condition.wait(lock, [&] { return result.is_ready(); }); });
+    invoke_waiting([&] {
+        std::unique_lock<std::mutex> lock{m};
+        condition.wait(lock, [&] { return result.is_ready(); });
+    });
     return std::move(result).get_ready();
 }
 
@@ -111,9 +113,10 @@ struct blocking_get_guarded {
     }
 
     auto wait_for(const std::chrono::nanoseconds& timeout) -> future<T> {
-        std::unique_lock<std::mutex> lock{_mutex};
-        _timed_out = !invoke_waiting(
-            [&] { return _condition.wait_for(lock, timeout, [&] { return _result.valid(); }); });
+        _timed_out = !invoke_waiting([&] {
+            std::unique_lock<std::mutex> lock{_mutex};
+            return _condition.wait_for(lock, timeout, [&] { return _result.valid(); });
+        });
         return _timed_out ? future<T>{} : std::move(_result);
     }
 };

diff --git a/include/stlab/test/model.hpp b/include/stlab/test/model.hpp
@@ -43,8 +43,10 @@ struct annotate_counters {
     auto remaining() const -> std::size_t { return _copy_ctor + _move_ctor - _dtor + 1; }
 
     void wait(std::size_t count) {
-        std::unique_lock<std::mutex> lock(_mutex);
-        stlab::invoke_waiting([&] { _condition.wait(lock, [&] { return count == remaining(); }); });
+        stlab::invoke_waiting([&] {
+            std::unique_lock<std::mutex> lock(_mutex);
+            _condition.wait(lock, [&] { return count == remaining(); });
+        });
     }
 
     friend inline auto operator<<(std::ostream& out, const annotate_counters& x) -> std::ostream& {

diff --git a/test/executor_test.cpp b/test/executor_test.cpp
@@ -121,8 +121,10 @@ BOOST_AUTO_TEST_CASE(task_system_restarts_after_it_went_pending) {
     });
 
     {
-        unique_lock<mutex> block{m};
-        invoke_waiting([&] { cv.wait(block, [&] { return done; }); });
+        invoke_waiting([&] {
+            unique_lock<mutex> block{m};
+            cv.wait(block, [&] { return done; });
+        });
     }
 
     default_executor([&]() noexcept {
@@ -135,8 +137,10 @@ BOOST_AUTO_TEST_CASE(task_system_restarts_after_it_went_pending) {
     });
 
     {
-        unique_lock<mutex> block{m};
-        invoke_waiting([&] { cv.wait(block, [&] { return !done; }); });
+        invoke_waiting([&] {
+            unique_lock<mutex> block{m};
+            cv.wait(block, [&] { return !done; });
+        });
     }
 
     BOOST_REQUIRE(!done);
@@ -283,8 +287,8 @@ BOOST_AUTO_TEST_CASE(MeasureTiming) {
         }
     });
 
-    unique_lock<mutex> lock{block};
-    invoke_waiting([&]{ ready.wait(lock, [&]{ return done; }); });
+    invoke_waiting(
+    unique_lock<mutex> lock{block}; [&]{ ready.wait(lock, [&]{ return done; }); });
 
     while (counter < 3 * iterations) {
         rest();

diff --git a/test/future_test_helper.hpp b/test/future_test_helper.hpp
@@ -199,11 +199,11 @@ class blocking_policy {
     void set_context(thread_block_context* context) { _context = context; }
 
     void action() const {
-        lock_t lock(*_context->_mutex);
-
-        while (!_context->_go || !_context->_may_proceed) {
-            stlab::invoke_waiting([&]{ _context->_thread_block.wait(lock); });
-        }
+        stlab::invoke_waiting([&] {
+            lock_t lock(*_context->_mutex);
+            _context->_thread_block.wait(lock,
+                                         [&] { return _context->_go && _context->_may_proceed; });
+        });
     }
 };