diff --git a/.github/workflows/cmake.yml b/.github/workflows/cmake.yml index eb1e17f..aaef60f 100644 --- a/.github/workflows/cmake.yml +++ b/.github/workflows/cmake.yml @@ -142,18 +142,6 @@ jobs: os: windows-latest, compiler: { type: VISUAL, version: 19, cc: "cl", cxx: "cl" }, } - - { - name: "MacOS Apple Clang 15", - os: macos-14, - compiler: - { - type: APPLE_CLANG, - version: "15.0", - cc: "clang", - cxx: "clang++", - std: 20, - }, - } steps: - uses: actions/checkout@v4 - uses: seanmiddleditch/gha-setup-ninja@master diff --git a/DRAFT.md b/DRAFT.md index 737cdc2..ba9095d 100644 --- a/DRAFT.md +++ b/DRAFT.md @@ -77,6 +77,27 @@ the standard library header ``. ## Choices +### Using `lock_guard` or `unique_lock` + +Calling `lock` unconditionally locks the mutex, so intuitively it should return a +`lock_guard`, which is slightly faster than `unique_lock`, which needs to check if +the lock is held at destruction. The problem with this is that `lock_guard` is +incompatible with `condition_variable`, which only accept a `unique_lock`. +Alternatively we could expose the mutex and force people to use +`condition_variable_any`, but `lock_guard` doesn't expose `mutex` either. We could +package an extra reference to the underlying mutex, but that would cost additional +memory (unless the optimizer removes the unused reference). + +It seems returning a `unique_lock` is low enough cost to not be a big deal and +worth the additional compabitility. If the cost is too high, then you can avoid it +by using `with` instead of `lock`. Since the guard isn't exposed and therefore +can't be used with a condition variable `with` uses `lock_guard` for the additional +speed. + +One other minor argument for always using a `unique_lock` is so that the return +type doesn't change if you switch from `lock` to `try_lock`, where `unique_lock` +is needed. + ### Explicit or overloaded method names Is it better to be explicit with all the variants, or use overloads? We've @@ -93,7 +114,7 @@ though the templates, namespaces, `[[nodiscard]]` and concept constraints have been ommited for clarity. ```c++ - mutex_locked> lock(); + mutex_locked> lock(); mutex_locked> try_lock(); mutex_locked> try_lock_until(const time_point &timeout_time); mutex_locked> try_lock_for(const duration &timeout_duration); @@ -122,7 +143,7 @@ The alternative is to use overloads. A potential api is below, with the same simplifications as above. ```c++ - mutex_locked> lock(); + mutex_locked> lock(); mutex_locked> lock(const time_point &timeout_time); mutex_locked> lock(const duration &timeout_duration); @@ -148,7 +169,7 @@ support `try_lock` on a `std::mutex`. This suggests an alternative in between may be more reasonable: ```c++ - mutex_locked> lock(); + mutex_locked> lock(); mutex_locked> try_lock(); mutex_locked> try_lock(const time_point &timeout_time); mutex_locked> try_lock(const duration &timeout_duration); diff --git a/README.md b/README.md index 92a8b1b..15cdeb6 100644 --- a/README.md +++ b/README.md @@ -225,7 +225,42 @@ mutex_protected value; } ``` -### Condition variables +### Condition Variables + +Condition variables are used to wait for a particular change to occur on +internal data, but need access to either the underlying lock guard or the +underlying mutex. + +#### std::mutex + +`std::condition_variable` only works with a `std::mutex`, so when using +The `mutex_protected` the `mutex_locked` instance has a `guard` +method that returns the underlying `std::unique_lock`. + +```cpp +mutex_protected> data; +std::condition_variable cv; + +// Wait for some other thread to add data to the vector. +auto locked = data.lock(); +cv.wait(locked.guard(), [&locked]() { return !locked->empty(); }); +``` + +#### Other mutexes, eg: std::shared_mutex + +For all other mutex types, you'll need to use a `std::condition_variable_any` +which works on the mutex instead of on the guard. For that, you can use the +`mutex` method on the `mutex_locked`. This works regardless of whether the +lock is exclusive or shared. + +```cpp +mutex_protected, std::shared_mutex> data; +std::condition_variable_any cv; + +// Wait for some other thread to add data to the vector. +auto locked = data.lock(); +cv.wait(locked.mutex(), [&locked]() { return !locked->empty(); }); +``` ### Locking multiple mutexes simultaneously diff --git a/mutex_protected.h b/mutex_protected.h index 2a07136..a033820 100644 --- a/mutex_protected.h +++ b/mutex_protected.h @@ -52,13 +52,20 @@ class [[nodiscard]] mutex_locked { bool owns_lock() const noexcept requires std::is_member_function_pointer_v { - return guard.owns_lock(); + return g.owns_lock(); } operator bool() const noexcept requires std::is_member_function_pointer_v { - return guard.owns_lock(); + return g.owns_lock(); + } + + // Needed for use with `std::condition_variable`. + G &guard() noexcept + requires std::is_same_v> + { + return g; } mutex_locked(const mutex_locked &) = delete; @@ -67,15 +74,22 @@ class [[nodiscard]] mutex_locked { mutex_locked(mutex_locked &&m) noexcept requires std::move_constructible - : v(std::exchange(m.v, nullptr)), guard(std::move(m.guard)) {} + : v(std::exchange(m.v, nullptr)), g(std::move(m.g)) {} + + // Needed for use with `std::condition_variable_any`, ie if you are using a + // different mutex type. + mutex_type *mutex() noexcept + requires std::is_member_function_pointer_v + { + return g.mutex(); + } private: template - mutex_locked(T *v_, Args &&...args) - : v{v_}, guard{std::forward(args)...} {} + mutex_locked(T *v_, Args &&...args) : v{v_}, g{std::forward(args)...} {} T *v; - G guard; + G g; template friend class mutex_protected; @@ -92,8 +106,10 @@ class mutex_protected { mutex_protected(const T &v_) : mutex{}, v(v_) {} - mutex_locked> lock() { - return mutex_locked>(&v, mutex); + // Returning a `lock_guard` would be slightly faster, but wouldn't work with a + // condition variable. + mutex_locked> lock() { + return mutex_locked>(&v, mutex); } mutex_locked> try_lock() { diff --git a/mutex_protected_test.cc b/mutex_protected_test.cc index 5c9a3c0..5881ca1 100644 --- a/mutex_protected_test.cc +++ b/mutex_protected_test.cc @@ -1,6 +1,8 @@ #include "mutex_protected.h" #include +#include +#include #include #include #include @@ -461,4 +463,89 @@ TEST(SharedTimedMutexProtectedTest, TimeoutWorksCorrectly) { EXPECT_EQ(*write_locked, 1); } +TEST(CondVarMutexProtectedTest, ConditionVariableWorks) { + mutex_protected> inputs; + mutex_protected> outputs; + std::condition_variable in_cv; + std::condition_variable out_cv; + std::stop_source stop; + + const int items = 100; + const int thread_count = 2; + + std::vector threads; + threads.reserve(thread_count); + for (int i = 0; i < thread_count; ++i) { + threads.emplace_back([&]() { + while (!stop.stop_requested()) { + int value; + { + auto locked = inputs.lock(); + in_cv.wait(locked.guard(), [&stop, &locked]() { + return stop.stop_requested() || !locked->empty(); + }); + if (locked->empty()) { // Stop requested, exit + return; + } + value = locked->front(); + locked->pop(); + } + outputs.lock()->push(value); + out_cv.notify_one(); + } + }); + } + + std::this_thread::sleep_for(5ms); + int expected_total = 0; + for (int i = 0; i < items; ++i) { + inputs.with([&i](auto& in_q) { in_q.push(i); }); + in_cv.notify_one(); + expected_total += i; + } + + int total = 0; + for (int i = 0; i < items; ++i) { + auto locked = outputs.lock(); + out_cv.wait(locked.guard(), [&locked]() { return !locked->empty(); }); + total += locked->front(); + locked->pop(); + } + + stop.request_stop(); + in_cv.notify_all(); + for (auto& thread : threads) { + thread.join(); + } + + EXPECT_EQ(total, expected_total); +} + +TEST(CondVarMutexProtectedTest, ConditionVariableAnyWorks) { + mutex_protected data = 0; + std::condition_variable_any cv; + mutex_protected out = 0; + + const int thread_count = 2; + + std::vector threads; + threads.reserve(thread_count); + for (int i = 0; i < thread_count; ++i) { + threads.emplace_back([&]() { + auto locked = data.lock_shared(); + cv.wait(*locked.mutex(), [&locked]() { return *locked > 0; }); + EXPECT_EQ(*locked, 1); + *out.lock() += *locked; + }); + } + + std::this_thread::sleep_for(5ms); + *data.lock() = 1; + cv.notify_all(); + for (auto& thread : threads) { + thread.join(); + } + EXPECT_EQ(*out.lock(), thread_count); +} + } // namespace xyz