Improve guard usage pattern

- Use guard objects in a scope rather than if statement.
- Updated docs, tests, and benchmarks.

Author: luketokheim

README.md

@@ -17,14 +17,19 @@ int main()
 {
   lockables::Guarded<int> value{100};
 
-  // The guard is a pointer like object that owns a lock on value.
-  if (auto guard = value.with_exclusive()) {
+  {
+    // The guard is a pointer like object that owns a lock on value.
+    auto guard = value.with_exclusive();
+
     // Writer lock until guard goes out of scope.
     *guard += 10;
   }
 
   int copy = 0;
-  if (auto guard = value.with_shared()) {
+  {
+    // Reader lock.
+    const auto guard = value.with_shared();
+
     // Reader lock.
     copy = *guard;
   }
@@ -47,12 +52,14 @@ int main()
 {
   lockables::Guarded<std::vector<int>> value{1, 2, 3, 4, 5};
 
-  // The guard allows for multiple operations in the lock scope.
-  if (auto guard = value.with_exclusive()) {
+  // The guard allows for multiple operations in one locked scope.
+  {
+    auto guard = value.with_exclusive();
+
     // sum = value[0] + ... + value[n - 1]
     const int sum = std::reduce(guard->begin(), guard->end());
 
-    // value = value + sum(value)
+    // value[i] = value[i] + sum(value)
     std::transform(guard->begin(), guard->end(), guard->begin(),
                    [sum](int x) { return x + sum; });
 
@@ -96,30 +103,45 @@ int main()
 
 ## Anti-patterns: Do not do this!
 
+Problem: Data race by keeping an unguarded pointer.
+
 Solution: The user must not keep a pointer or reference to the guarded value
 outside the locked scope.
 
 ```cpp
 lockables::Guarded<int> value;
 
 int* unguarded_pointer{};
-if (auto guard = value.with_exclusive()) {
+{
+  auto guard = value.with_exclusive();
+
   // No! User must not keep a pointer or reference outside the guarded
   // scope.
   unguarded_pointer = &(*guard);
 }
 
 // No! Data race if another thread is accessing value.
-// *unguarded_pointer = 1;
+// *unguarded_pointer = -10;
+
+// No! User must not keep a reference to the guarded value.
+int& unguarded_reference =
+    lockables::with_exclusive([](int& x) -> int& { return x; }, value);
+
+// No! Data race if another thread is accessing value.
+// unguarded_reference = -20;
 ```
 
+Problem: Deadlock with recursive guards.
+
 Solution: A calling thread must not own the mutex prior to calling any of the
 locking functions.
 
 ```cpp
 lockables::Guarded<int> value;
 
-if (auto guard = value.with_exclusive()) {
+{
+  auto guard = value.with_exclusive();
+
   // No! Deadlock since this thread already owns a lock on value.
   // auto recursive_reader = value.with_shared();
 
@@ -131,6 +153,8 @@ if (auto guard = value.with_exclusive()) {
 }
 ```
 
+Problem: Deadlock with multiple guards.
+
 Solution: To lock multiple values, use the ``with_exclusive`` function which
 avoids deadlock.
 

benchmarks/README.md

@@ -2,7 +2,7 @@
 
 This project uses the excellent [Benchmark](https://github.com/google/benchmark)
 library from Google. Most of the benchmarks compare ``std::mutex`` and
-``std::shared_metux`` performance over various numbers of reader and writer
+``std::shared_mutex`` performance over various numbers of reader and writer
 threads.
 
 For example, the test case named:

benchmarks/bench_guarded.cpp

@@ -6,7 +6,8 @@ void BM_Guarded_Shared(benchmark::State& state) {
   lockables::Guarded<T, Mutex> value;
   for (auto _ : state) {
     T copy{};
-    if (auto guard = value.with_shared()) {
+    {
+      const auto guard = value.with_shared();
       copy = *guard;
     }
 
@@ -22,7 +23,8 @@ void BM_Guarded_Exclusive(benchmark::State& state) {
   lockables::Guarded<T, Mutex> value;
   for (auto _ : state) {
     T copy{};
-    if (auto guard = value.with_exclusive()) {
+    {
+      auto guard = value.with_exclusive();
       copy = *guard;
     }
 
@@ -38,8 +40,8 @@ void BM_Guarded_Multiple(benchmark::State& state) {
   lockables::Guarded<T, Mutex> value1;
   lockables::Guarded<T, Mutex> value2;
   for (auto _ : state) {
-    auto copy = lockables::with_exclusive(
-        [](const auto& x, const auto& y) { return x + y; }, value1, value2);
+    T copy = lockables::with_exclusive(
+        [](auto& x, auto& y) -> T { return x + y; }, value1, value2);
 
     benchmark::DoNotOptimize(copy);
   }
@@ -58,15 +60,13 @@ struct BM_Guarded_Fixture : benchmark::Fixture {
   lockables::Guarded<int64_t, Mutex> value{};
 
   void SetUp(const benchmark::State& state) override {
-    if (auto guard = value.with_exclusive()) {
-      *guard = 0;
-    }
+    auto guard = value.with_exclusive();
+    *guard = 0;
   }
 
   void TearDown(const benchmark::State& state) override {
-    if (auto guard = value.with_exclusive()) {
-      assert(*guard == state.iterations() * state.range(0));
-    }
+    auto guard = value.with_shared();
+    assert(*guard == state.iterations() * state.range(0));
   }
 
   void BenchmarkCase(benchmark::State& state) override {
@@ -84,7 +84,8 @@ struct BM_Guarded_Fixture : benchmark::Fixture {
   void RunWriter(benchmark::State& state) {
     for (auto _ : state) {
       int64_t copy{};
-      if (auto guard = value.with_exclusive()) {
+      {
+        auto guard = value.with_exclusive();
         *guard += 1;
         copy = *guard;
       }
@@ -96,7 +97,8 @@ struct BM_Guarded_Fixture : benchmark::Fixture {
   void RunReader(benchmark::State& state) {
     for (auto _ : state) {
       int64_t copy{};
-      if (auto guard = value.with_shared()) {
+      {
+        const auto guard = value.with_shared();
         copy = *guard;
       }
 

include/lockables/guarded.hpp

@@ -23,14 +23,18 @@
   Usage:
 
   Guarded<int> value{9};
-  if (auto guard = value.with_exclusive()) {
+  {
     // Writer access. The mutex is locked until guard goes out of scope.
+    auto guard = value.with_exclusive();
+
     *guard += 10;
   }
 
   int copy = 0;
-  if (auto guard = value.with_shared()) {
+  {
     // Reader access.
+    auto guard = value.with_shared();
+
     copy = *guard;
     // *guard += 10;  // will not compile!
   }
@@ -80,15 +84,19 @@ class GuardedScope;
   Guarded<std::vector<int>> value{1, 2, 3, 4, 5};
 
   // Reader with shared lock.
-  if (const auto guard = value.with_shared()) {
-    // Deference pointer like object GuardedScope<std::vector<int>>
+  {
+    const auto guard = value.with_shared();
+
+    // Deference pointer like object guard
     if (!guard->empty()) {
       int copy = guard->back();
     }
   }
 
   // Writer with exclusive lock.
-  if (auto guard = value.with_exclusive()) {
+  {
+    auto guard = value.with_exclusive();
+
     guard->push_back(100);
     (*guard).clear();
   }
@@ -130,15 +138,19 @@ class Guarded {
     Usage:
 
     Guarded<int> value;
-    if (const auto guard = value.with_shared()) {
-        const int copy = *guard;
+    {
+      const auto guard = value.with_shared();
+
+      const int copy = *guard;
     }
 
     Guarded<std::vector<int>> list;
-    if (const auto guard = list.with_shared()) {
-        if (!guard->empty()) {
-          const int copy = guard->back();
-        }
+    {
+      const auto guard = list.with_shared();
+
+      if (!guard->empty()) {
+        const int copy = guard->back();
+      }
     }
   */
   [[nodiscard]] shared_scope with_shared() const;
@@ -153,13 +165,18 @@ class Guarded {
     Usage:
 
     Guarded<int> value;
-    if (auto guard = value.with_exclusive()) {
-        *guard = 10;
+    {
+      auto guard = value.with_exclusive();
+
+      *guard = 10;
     }
 
     Guarded<std::vector<int>> list;
-    if (auto guard = list.with_exclusive()) {
-        guard->push_back(100);
+    {
+      auto guard = list.with_exclusive();
+
+      guard->push_back(100);
+      guard->push_back(10);
     }
    */
   [[nodiscard]] exclusive_scope with_exclusive();
@@ -190,16 +207,20 @@ auto Guarded<T, Mutex>::with_exclusive() -> exclusive_scope {
   return exclusive_scope{&value_, mutex_};
 }
 
-/*
+/**
   The with_exclusive function provides access to one or more Guarded<T> objects
   from a user supplied callback.
 
   Basic usage:
 
   Guarded<int> value;
-  with_exclusive([](int& x) {
-    x += 10;
-  }, value);
+
+  with_exclusive(
+      [](int& x) {
+        // Writer with exclusive lock on value.
+        x += 10;
+      },
+      value);
 
   The intent is to support locking of multiple Guarded<T> objects. The
   with_exclusive function relies on std::scoped_lock for deadlock avoidance.
@@ -209,10 +230,13 @@ auto Guarded<T, Mutex>::with_exclusive() -> exclusive_scope {
   Guarded<int> value1{1};
   Guarded<int> value2{2};
 
-  with_exclusive([](int& x, int& y) {
-    x += y;
-    y /= 2;
-  }, value1, value2);
+  with_exclusive(
+      [](int& x, int& y) {
+        // Writer with exclusive lock on value1 and value2.
+        x += y;
+        y /= 2;
+      },
+      value1, value2);
 
   References:
 
@@ -232,10 +256,10 @@ std::invoke_result_t<F, ValueTypes&...> with_exclusive(
   Type trait to select which lock is used in GuardedScope<T>.
 
   Use these std lock types internally for shared access from readers.
-   - std::scoped_lock<std::mutex> lock(...);
-   - std::shared_lock<std::shared_mutex> lock(...);
+   - std::scoped_lock<std::mutex>
+   - std::shared_lock<std::shared_mutex>
 
-  Always use std::scoped_lock<Mutex> for writers.
+  Always use std::scoped_lock<Mutex> for exclusive access from writers.
 
   This means that if the user chooses std::mutex the shared and exclusive locks
   are the same type.