From 5a3e98badd6d7636d5379f15d625e92838ddd190 Mon Sep 17 00:00:00 2001 From: Marin Tomic Date: Mon, 22 Jul 2019 14:19:35 +0200 Subject: [PATCH] Implement Synchronized utility Reviewers: mferencevic, teon.banek Reviewed By: mferencevic Subscribers: pullbot Differential Revision: https://phabricator.memgraph.io/D2222 --- src/utils/synchronized.hpp | 93 +++++++++++++++++++++++++++++++ tests/unit/CMakeLists.txt | 3 + tests/unit/utils_synchronized.cpp | 81 +++++++++++++++++++++++++++ 3 files changed, 177 insertions(+) create mode 100644 src/utils/synchronized.hpp create mode 100644 tests/unit/utils_synchronized.cpp diff --git a/src/utils/synchronized.hpp b/src/utils/synchronized.hpp new file mode 100644 index 000000000..fa7b90e47 --- /dev/null +++ b/src/utils/synchronized.hpp @@ -0,0 +1,93 @@ +#pragma once + +#include +#include + +namespace utils { + +/// A simple utility for easier mutex-based concurrency (influenced by +/// Facebook's Folly) +/// +/// Many times we have an object that is accessed from multiple threads so it +/// has an associated lock: +/// +/// utils::SpinLock my_important_map_lock_; +/// std::map my_important_map_; +/// +/// Whenever we want to access the object, we have to remember that we have to +/// acquire the corresponding lock: +/// +/// std::lock_guard +/// my_important_map_guard(my_important_map_lock_); +/// my_important_map_[key] = value; +/// +/// Correctness of this approach depends on the programmer never forgetting to +/// acquire the lock. +/// +/// Synchronized encodes that information in the type information, and it is +/// much harder to use the object incorrectly. +/// +/// Synchronized, utils::SpinLock> +/// my_important_map_; +/// +/// Now we have multiple ways of accessing the map: +/// +/// 1. Acquiring a locked pointer: +/// auto my_map_ptr = my_important_map_.Lock(); +/// my_map_ptr->emplace(key, value); +/// +/// 2. Using the indirection operator: +/// +/// my_important_map_->emplace(key, value); +/// +/// 3. Using a lambda: +/// my_important_map_.WithLock([](auto &my_important_map) { +/// my_important_map[key] = value; +/// }); +/// +/// Approach 2 is probably the best to use for one-line operations, and +/// approach 3 for multi-line ops. +template +class Synchronized { + public: + template + explicit Synchronized(Args &&... args) + : object_(std::forward(args)...) {} + + Synchronized(const Synchronized &) = delete; + Synchronized(Synchronized &&) = delete; + Synchronized &operator=(const Synchronized &) = delete; + Synchronized &operator=(Synchronized &&) = delete; + ~Synchronized() = default; + + class LockedPtr { + private: + friend class Synchronized; + + LockedPtr(T *object_ptr, TMutex *mutex) + : object_ptr_(object_ptr), guard_(*mutex) {} + + public: + T *operator->() { return object_ptr_; } + T &operator*() { return *object_ptr_; } + + private: + T *object_ptr_; + std::lock_guard guard_; + }; + + LockedPtr Lock() { return LockedPtr(&object_, &mutex_); } + + template + auto WithLock(TCallable &&callable) { + return callable(*Lock()); + } + + LockedPtr operator->() { return LockedPtr(&object_, &mutex_); } + + private: + T object_; + TMutex mutex_; +}; + +} // namespace utils diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index 34916a69f..82976512a 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -288,6 +288,9 @@ target_link_libraries(${test_prefix}utils_signals mg-utils) add_unit_test(utils_string.cpp) target_link_libraries(${test_prefix}utils_string mg-utils) +add_unit_test(utils_synchronized.cpp) +target_link_libraries(${test_prefix}utils_synchronized mg-utils) + add_unit_test(utils_thread_pool.cpp) target_link_libraries(${test_prefix}utils_thread_pool mg-utils) diff --git a/tests/unit/utils_synchronized.cpp b/tests/unit/utils_synchronized.cpp new file mode 100644 index 000000000..2b51a688e --- /dev/null +++ b/tests/unit/utils_synchronized.cpp @@ -0,0 +1,81 @@ +#include + +#include "gtest/gtest.h" + +#include "utils/synchronized.hpp" + +class NoMoveNoCopy { + public: + NoMoveNoCopy(int, int) {} + + NoMoveNoCopy(const NoMoveNoCopy &) = delete; + NoMoveNoCopy(NoMoveNoCopy &&) = delete; + NoMoveNoCopy &operator=(const NoMoveNoCopy &) = delete; + NoMoveNoCopy &operator=(NoMoveNoCopy &&) = delete; + ~NoMoveNoCopy() = default; +}; + +// NOLINTNEXTLINE(hicpp-special-member-functions) +TEST(Synchronized, Constructors) { + { + utils::Synchronized> vec; + EXPECT_TRUE(vec->empty()); + } + { + std::vector data = {1, 2, 3}; + utils::Synchronized> vec(data); + EXPECT_EQ(data.size(), 3); + EXPECT_EQ(vec->size(), 3); + } + { + std::vector data = {1, 2, 3}; + utils::Synchronized> vec(std::move(data)); + // data is guaranteed by the standard to be empty after move + // NOLINTNEXTLINE(bugprone-use-after-move, hicpp-invalid-access-moved) + EXPECT_TRUE(data.empty()); + EXPECT_EQ(vec->size(), 3); + } + { utils::Synchronized object(3, 4); } +} + +bool test_lock_locked = false; + +class TestLock { + public: + void lock() { + ASSERT_FALSE(test_lock_locked); + test_lock_locked = true; + } + void unlock() { + ASSERT_TRUE(test_lock_locked); + test_lock_locked = false; + } +}; + +// NOLINTNEXTLINE(hicpp-special-member-functions) +TEST(Synchronized, Usage) { + utils::Synchronized, TestLock> my_vector; + { + // LockedPtr + auto ptr = my_vector.Lock(); + ASSERT_TRUE(test_lock_locked); + ptr->push_back(5); + } + ASSERT_FALSE(test_lock_locked); + + { + // Indirection operator + my_vector->push_back(6); + } + + { + // Lambda + my_vector.WithLock([](auto &my_vector) { + ASSERT_TRUE(test_lock_locked); + EXPECT_EQ(my_vector.size(), 2); + EXPECT_EQ(my_vector[0], 5); + EXPECT_EQ(my_vector[1], 6); + }); + ASSERT_FALSE(test_lock_locked); + } +}