From 3ca8330ab2bee8ee61772f64373faf6c6187fd7f Mon Sep 17 00:00:00 2001 From: Luke Tokheim Date: Thu, 13 Apr 2023 14:28:13 -0700 Subject: [PATCH] Improve guard usage pattern - Use guard objects in a scope rather than if statement. - Updated docs, tests, and benchmarks. --- README.md | 42 +++++++++++--- benchmarks/README.md | 2 +- benchmarks/bench_guarded.cpp | 26 +++++---- include/lockables/guarded.hpp | 76 +++++++++++++++--------- tests/test.cpp | 66 +++++++++++++++------ tests/test_antipatterns.cpp | 16 ++++- tests/test_guarded.cpp | 106 +++++++++++++++++++++------------- 7 files changed, 224 insertions(+), 110 deletions(-) diff --git a/README.md b/README.md index b08400c..f88ba82 100644 --- a/README.md +++ b/README.md @@ -17,14 +17,19 @@ int main() { lockables::Guarded 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> 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,6 +103,8 @@ 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. @@ -103,23 +112,36 @@ outside the locked scope. lockables::Guarded 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 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. diff --git a/benchmarks/README.md b/benchmarks/README.md index 2db9999..364d63a 100644 --- a/benchmarks/README.md +++ b/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: diff --git a/benchmarks/bench_guarded.cpp b/benchmarks/bench_guarded.cpp index b146c6a..e190d38 100644 --- a/benchmarks/bench_guarded.cpp +++ b/benchmarks/bench_guarded.cpp @@ -6,7 +6,8 @@ void BM_Guarded_Shared(benchmark::State& state) { lockables::Guarded 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 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 value1; lockables::Guarded 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 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; } diff --git a/include/lockables/guarded.hpp b/include/lockables/guarded.hpp index dcebaa1..a9ff389 100644 --- a/include/lockables/guarded.hpp +++ b/include/lockables/guarded.hpp @@ -23,14 +23,18 @@ Usage: Guarded 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> value{1, 2, 3, 4, 5}; // Reader with shared lock. - if (const auto guard = value.with_shared()) { - // Deference pointer like object GuardedScope> + { + 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 value; - if (const auto guard = value.with_shared()) { - const int copy = *guard; + { + const auto guard = value.with_shared(); + + const int copy = *guard; } Guarded> 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 value; - if (auto guard = value.with_exclusive()) { - *guard = 10; + { + auto guard = value.with_exclusive(); + + *guard = 10; } Guarded> 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::with_exclusive() -> exclusive_scope { return exclusive_scope{&value_, mutex_}; } -/* +/** The with_exclusive function provides access to one or more Guarded objects from a user supplied callback. Basic usage: Guarded 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 objects. The with_exclusive function relies on std::scoped_lock for deadlock avoidance. @@ -209,10 +230,13 @@ auto Guarded::with_exclusive() -> exclusive_scope { Guarded value1{1}; Guarded 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 with_exclusive( Type trait to select which lock is used in GuardedScope. Use these std lock types internally for shared access from readers. - - std::scoped_lock lock(...); - - std::shared_lock lock(...); + - std::scoped_lock + - std::shared_lock - Always use std::scoped_lock for writers. + Always use std::scoped_lock for exclusive access from writers. This means that if the user chooses std::mutex the shared and exclusive locks are the same type. diff --git a/tests/test.cpp b/tests/test.cpp index 19c7b2c..8f08b10 100644 --- a/tests/test.cpp +++ b/tests/test.cpp @@ -10,14 +10,18 @@ TEST_CASE("README", "[lockables][examples]") { { lockables::Guarded 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()) { + { + auto guard = value.with_shared(); + // Reader lock. copy = *guard; } @@ -28,12 +32,14 @@ TEST_CASE("README", "[lockables][examples]") { { lockables::Guarded> 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; }); @@ -66,14 +72,18 @@ TEST_CASE("README", "[lockables][examples]") { TEST_CASE("Guarded example", "[lockables][examples][Guarded]") { lockables::Guarded 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; } - [[maybe_unused]] int copy = 0; - if (auto guard = value.with_shared()) { + int copy = 0; + { // Reader access. + auto guard = value.with_shared(); + copy = *guard; // *guard += 10; // will not compile! } @@ -86,15 +96,19 @@ TEST_CASE("Guarded vector example", "[lockables][examples][Guarded]") { lockables::Guarded> value{1, 2, 3, 4, 5}; // Reader with shared lock. - if (const auto guard = value.with_shared()) { - // Guard is a std::unique_ptr> + { + const auto guard = value.with_shared(); + + // Deference pointer like object guard if (!guard->empty()) { - [[maybe_unused]] const int copy = guard->back(); + 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(); } @@ -102,12 +116,16 @@ TEST_CASE("Guarded vector example", "[lockables][examples][Guarded]") { TEST_CASE("Guarded::with_shared example", "[lockables][examples][Guarded]") { lockables::Guarded value; - if (const auto guard = value.with_shared()) { + { + const auto guard = value.with_shared(); + [[maybe_unused]] const int copy = *guard; } lockables::Guarded> list; - if (const auto guard = list.with_shared()) { + { + const auto guard = list.with_shared(); + if (!guard->empty()) { [[maybe_unused]] const int copy = guard->back(); } @@ -116,20 +134,31 @@ TEST_CASE("Guarded::with_shared example", "[lockables][examples][Guarded]") { TEST_CASE("Guarded::with_exclusive example", "[lockables][examples][Guarded]") { lockables::Guarded value; - if (auto guard = value.with_exclusive()) { + { + auto guard = value.with_exclusive(); + *guard = 10; } lockables::Guarded> list; - if (auto guard = list.with_exclusive()) { + { + auto guard = list.with_exclusive(); + guard->push_back(100); + guard->push_back(10); } } TEST_CASE("Guarded with_exclusive single example", "[lockables][examples][Guarded][with_exclusive]") { lockables::Guarded value; - lockables::with_exclusive([](int& x) { x += 10; }, value); + + lockables::with_exclusive( + [](int& x) { + // Writer with exclusive lock on value. + x += 10; + }, + value); } TEST_CASE("Guarded with_exclusive multiple example", @@ -139,6 +168,7 @@ TEST_CASE("Guarded with_exclusive multiple example", lockables::with_exclusive( [](int& x, int& y) { + // Writer with exclusive lock on value1 and value2. x += y; y /= 2; }, diff --git a/tests/test_antipatterns.cpp b/tests/test_antipatterns.cpp index 4a3502f..6972c9a 100644 --- a/tests/test_antipatterns.cpp +++ b/tests/test_antipatterns.cpp @@ -14,7 +14,7 @@ TEST_CASE("Anti-pattern: Call with_exclusive with no values", // Solution: Just FYI. } -TEST_CASE("Anti-pattern: Stealing an unguarded pointer", +TEST_CASE("Anti-pattern: Data race by keeping an unguarded pointer", "[lockables][antipatterns][Guarded]") { lockables::Guarded value; @@ -28,7 +28,9 @@ TEST_CASE("Anti-pattern: Stealing an unguarded pointer", }); [[maybe_unused]] 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); @@ -44,9 +46,17 @@ TEST_CASE("Anti-pattern: Stealing an unguarded pointer", // No! Data race. 1 thread writing, 1 reading at the same time. // int oops = *unguarded_pointer; + // 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. 2 threads writing at the same time. + // unguarded_reference = -20; + future.wait(); - if (auto guard = value.with_exclusive()) { + { + auto guard = value.with_exclusive(); REQUIRE(*guard == 1000); } diff --git a/tests/test_guarded.cpp b/tests/test_guarded.cpp index 9e2d96d..0825793 100644 --- a/tests/test_guarded.cpp +++ b/tests/test_guarded.cpp @@ -17,17 +17,20 @@ TEMPLATE_PRODUCT_TEST_CASE("read and write PODs", "[lockables][Guarded]", TestType value{kExpected}; - if (const auto guard = value.with_shared()) { + { + const auto guard = value.with_shared(); const auto x = *guard; CHECK(x == kExpected); } - if (auto guard = value.with_exclusive()) { + { + auto guard = value.with_exclusive(); CHECK(*guard == kExpected); *guard += 1; } - if (const auto guard = value.with_shared()) { + { + const auto guard = value.with_shared(); const auto x = *guard; CHECK(x == kExpected + 1); } @@ -53,17 +56,20 @@ TEMPLATE_TEST_CASE("read and write struct", "[lockables][Guarded]", TestType value{expected}; - if (auto guard = value.with_shared()) { + { + auto guard = value.with_shared(); CHECK(*guard == expected); } - if (auto guard = value.with_exclusive()) { + { + auto guard = value.with_exclusive(); CHECK(*guard == expected); (*guard).field1 += 1; guard->field2 += 1592; } - if (auto guard = value.with_shared()) { + { + auto guard = value.with_shared(); CHECK(guard->field1 == expected.field1 + 1); CHECK(*guard != expected); } @@ -75,11 +81,13 @@ TEMPLATE_TEST_CASE("read and write container", "[lockables][Guarded]", TestType value; for (int i = 0; i < 100; ++i) { - if (auto guard = value.with_exclusive()) { + { + auto guard = value.with_exclusive(); guard->push_back(i); } - if (const auto guard = value.with_shared()) { + { + const auto guard = value.with_shared(); CHECK(!guard->empty()); CHECK(guard->back() == i); } @@ -97,29 +105,23 @@ TEMPLATE_TEST_CASE("M reader threads, N writer threads", "[lockables][Guarded]", const auto writer_func = [&value]() -> std::size_t { for (auto i = 1; i <= kTarget; ++i) { - if (auto guard = value.with_exclusive()) { - *guard = i; - } else { - CHECK(false); - } + auto guard = value.with_exclusive(); + *guard = i; } return 0; }; const auto reader_func = [&value]() -> std::size_t { - for (std::size_t i = 1; i < std::numeric_limits::max(); ++i) { - if (auto guard = value.with_shared()) { - if (*guard >= kTarget) { - return i; - } - } else { - CHECK(false); + std::size_t i = 1; + for (; i < std::numeric_limits::max(); ++i) { + auto guard = value.with_shared(); + if (*guard >= kTarget) { + break; } } - CHECK(false); - return 0; + return i; }; // Test all combinations of M reader + N writer @@ -225,7 +227,8 @@ TEMPLATE_TEST_CASE("all the mutex types", "[lockables][Guarded]", std::mutex, lockables::Guarded value{10}; int copy = 0; - if (auto guard = value.with_shared()) { + { + const auto guard = value.with_shared(); copy = *guard; using lock_type = typename decltype(guard)::lock_type; @@ -241,7 +244,8 @@ TEMPLATE_TEST_CASE("all the mutex types", "[lockables][Guarded]", std::mutex, CHECK(copy == 10); - if (auto guard = value.with_exclusive()) { + { + auto guard = value.with_exclusive(); *guard = copy * 2; } @@ -250,33 +254,35 @@ TEMPLATE_TEST_CASE("all the mutex types", "[lockables][Guarded]", std::mutex, CHECK(copy == 20); } -TEST_CASE("constructor", "[lockables][examples][Guarded]") { +TEST_CASE("constructor", "[lockables][Guarded]") { { lockables::Guarded value{-1}; - if (auto guard = value.with_shared()) { + { + auto guard = value.with_shared(); CHECK(*guard == -1); } } { - const int x = 10; - - lockables::Guarded value{x}; - if (auto guard = value.with_shared()) { + lockables::Guarded value{10}; + { + auto guard = value.with_shared(); CHECK(*guard == 10); } } { auto value = std::make_unique>(101); - if (auto guard = value->with_shared()) { + { + auto guard = value->with_shared(); CHECK(*guard == 101); } } { auto value = std::make_shared>(101); - if (auto guard = value->with_shared()) { + { + auto guard = value->with_shared(); CHECK(*guard == 101); } } @@ -285,21 +291,24 @@ TEST_CASE("constructor", "[lockables][examples][Guarded]") { const std::vector x(100, 1); lockables::Guarded> value{x}; - if (auto guard = value.with_shared()) { + { + auto guard = value.with_shared(); CHECK(*guard == std::vector(100, 1)); } } { lockables::Guarded> value{std::vector{1, 2, 3}}; - if (auto guard = value.with_shared()) { + { + auto guard = value.with_shared(); CHECK(*guard == std::vector{1, 2, 3}); } } { lockables::Guarded> value{4, 5, 6}; - if (auto guard = value.with_shared()) { + { + auto guard = value.with_shared(); CHECK(*guard == std::vector{4, 5, 6}); } } @@ -308,11 +317,13 @@ TEST_CASE("constructor", "[lockables][examples][Guarded]") { using hash_map = std::unordered_map; lockables::Guarded value{hash_map{{"Hello", 15}, {"World", 10}}}; - if (auto guard = value.with_shared()) { + { + auto guard = value.with_shared(); CHECK(guard->at("Hello") == 15); } - if (auto guard = value.with_exclusive()) { + { + auto guard = value.with_exclusive(); CHECK((*guard)["Hello"] == 15); CHECK((*guard)["Nope"] == 0); } @@ -323,7 +334,8 @@ TEST_CASE("constructor", "[lockables][examples][Guarded]") { hash_map map{{"Hello", 15}, {"World", 10}}; lockables::Guarded value{map}; - if (auto guard = value.with_shared()) { + { + auto guard = value.with_shared(); CHECK(guard->at("Hello") == 15); } } @@ -333,7 +345,8 @@ TEST_CASE("constructor", "[lockables][examples][Guarded]") { hash_map map{{"Hello", 15}, {"World", 10}}; lockables::Guarded value{std::move(map)}; - if (auto guard = value.with_shared()) { + { + auto guard = value.with_shared(); CHECK(guard->at("Hello") == 15); } } @@ -344,8 +357,19 @@ TEST_CASE("constructor", "[lockables][examples][Guarded]") { std::make_unique(hash_map{{"Hello", 15}, {"World", 10}}); lockables::Guarded> value{std::move(map)}; - if (auto guard = value.with_shared()) { + { + auto guard = value.with_shared(); CHECK((*guard)->at("Hello") == 15); } } -} \ No newline at end of file + + { + using hash_map = std::unordered_map>; + + lockables::Guarded value; + { + auto guard = value.with_shared(); + CHECK(guard->empty()); + } + } +}