Some improvements and more tests

arangodb · mchacki · Jul 12, 2021 · Jul 9, 2019 · Jul 18, 2019 · Oct 9, 2019
commit b8c7f3dbb20a54f17d39909060b378c3a7e129d0
diff --git a/lib/Basics/Mutex.cpp b/lib/Basics/Mutex.cpp
@@ -111,6 +111,10 @@ bool Mutex::tryLock() {
   return true;
 }
 
+bool Mutex::try_lock() {
+  return tryLock();
+}
+
 void Mutex::unlock() {
 #ifdef ARANGODB_ENABLE_DEADLOCK_DETECTION
   TRI_ASSERT(_holder == Thread::currentThreadId());

diff --git a/lib/Basics/Mutex.h b/lib/Basics/Mutex.h
@@ -65,6 +65,7 @@ class Mutex {
  public:
   void lock();
   bool tryLock();
+  bool try_lock();
   void unlock();
 
   // assert that the mutex is locked by the current thread. will do

diff --git a/lib/Utilities/Guarded.h b/lib/Utilities/Guarded.h
@@ -23,49 +23,62 @@
 #ifndef LIB_UTILITIES_GUARDED_H
 #define LIB_UTILITIES_GUARDED_H
 
-#include "Basics/MutexLocker.h"
+// #include "Basics/MutexLocker.h"
 
 #include <algorithm>
 #include <functional>
+#include <mutex>
+#include <optional>
+#include <stdexcept>
+#include <variant>
 
 namespace arangodb {
 
 class Mutex;
 
-// TODO should callbacks be passed as const& rather than by value?
-
-template <class T, class M = Mutex>
+template <class T, class L>
 class MutexGuard {
  public:
-  explicit MutexGuard(T& value, M& mutex);
-  ~MutexGuard();
+  explicit MutexGuard(T& value, L mutexLock);
+  ~MutexGuard() = default;
 
   auto get() noexcept -> T&;
   auto get() const noexcept -> T const&;
 
+  auto operator->() noexcept -> T*;
+  auto operator->() const noexcept -> T const*;
+
  private:
   T& _value;
-  M& _mutex;
+  L _mutexLock;
 };
 
-template <class T, class M>
-MutexGuard<T, M>::MutexGuard(T& value, M& mutex) : _value(value), _mutex(mutex) {
-  _mutex.lock();
+template <class T, class L>
+MutexGuard<T, L>::MutexGuard(T& value, L mutexLock)
+    : _value(value), _mutexLock(std::move(mutexLock)) {
+  if (ADB_UNLIKELY(!_mutexLock.owns_lock())) {
+    throw std::invalid_argument("Lock not owned");
+  }
 }
 
-template <class T, class M>
-MutexGuard<T, M>::~MutexGuard() {
-  _mutex.unlock();
+template <class T, class L>
+auto MutexGuard<T, L>::get() noexcept -> T& {
+  return _value;
 }
 
-template <class T, class M>
-auto MutexGuard<T, M>::get() noexcept -> T& {
+template <class T, class L>
+auto MutexGuard<T, L>::get() const noexcept -> T const& {
   return _value;
 }
 
-template <class T, class M>
-auto MutexGuard<T, M>::get() const noexcept -> T const& {
-  return _value;
+template <class T, class L>
+auto MutexGuard<T, L>::operator->() noexcept -> T* {
+  return std::addressof(get());
+}
+
+template <class T, class L>
+auto MutexGuard<T, L>::operator->() const noexcept -> T const* {
+  return std::addressof(get());
 }
 
 template <class T, class M = Mutex>
@@ -81,17 +94,24 @@ class Guarded {
   auto operator=(Guarded const&) -> Guarded& = delete;
   auto operator=(Guarded&&) -> Guarded& = delete;
 
-  // TODO with C++17, these could be noexcept depending on callback being noexcept
-  template <class F>
-  auto doUnderLock(F callback) -> std::result_of_t<F(T&)>;
-  template <class F>
-  auto doUnderLock(F callback) -> std::result_of_t<F(T const&)>;
+  template <class F, class R = std::invoke_result_t<F, T&>>
+  auto doUnderLock(F&& callback) -> R;
+  template <class F, class R = std::invoke_result_t<F, T&>>
+  auto doUnderLock(F&& callback) const -> R;
+
+  template <class F, class R = std::invoke_result_t<F, T&>,
+            class Q = std::conditional_t<std::is_void_v<R>, std::monostate, R>>
+  [[nodiscard]] auto tryUnderLock(F&& callback) -> std::optional<Q>;
+  template <class F, class R = std::invoke_result_t<F, T&>,
+            class Q = std::conditional_t<std::is_void_v<R>, std::monostate, R>>
+  [[nodiscard]] auto tryUnderLock(F&& callback) const -> std::optional<Q>;
 
-  // template <class F>
-  // decltype(F(false)) tryUnderLock(F callback);
-  // TODO add more types of "do under lock"?
+  using L = std::unique_lock<M>;
+  auto getLockedGuard() -> MutexGuard<T, L>;
+  auto getLockedGuard() const -> MutexGuard<T const, L>;
 
-  auto getLockedGuard() -> MutexGuard<T, M>;
+  auto tryLockedGuard() -> std::optional<MutexGuard<T, L>>;
+  auto tryLockedGuard() const -> std::optional<MutexGuard<T const, L>>;
   // TODO add more types of "get guard" (like try or eventual)
 
  private:
@@ -107,32 +127,99 @@ template <typename... Args>
 Guarded<T, M>::Guarded(Args... args) : _value{args...}, _mutex{} {}
 
 template <class T, class M>
-template <class F>
-auto Guarded<T, M>::doUnderLock(F callback) -> std::result_of_t<F(T&)> {
-  MUTEX_LOCKER(guard, _mutex);
+template <class F, class R>
+auto Guarded<T, M>::doUnderLock(F&& callback) -> R {
+  auto guard = std::unique_lock<M>(_mutex);
+  //MUTEX_LOCKER(guard, _mutex);
+
+  if constexpr (!std::is_void_v<R>) {
+    return std::forward<F>(callback)(_value);
+  } else {
+    std::forward<F>(callback)(_value);
+    return;
+  }
+}
+
+template <class T, class M>
+template <class F, class R>
+auto Guarded<T, M>::doUnderLock(F&& callback) const -> R {
+  auto guard = std::unique_lock<M>(_mutex);
+  // MUTEX_LOCKER(guard, _mutex);
+
+  if constexpr (!std::is_void_v<R>) {
+    return std::forward<F>(callback)(_value);
+  } else {
+    std::forward<F>(callback)(_value);
+    return;
+  }
+}
+
+template <class T, class M>
+template <class F, class R, class Q>
+auto Guarded<T, M>::tryUnderLock(F&& callback) -> std::optional<Q> {
+  auto guard = std::unique_lock<M>(_mutex, std::try_to_lock);
+  // TRY_MUTEX_LOCKER(guard, _mutex);
+
+  if (guard.owns_lock()) {
+    if constexpr (!std::is_void_v<R>) {
+      return std::forward<F>(callback)(_value);
+    } else {
+      std::forward<F>(callback)(_value);
+      return std::monostate{};
+    }
+  } else {
+    return std::nullopt;
+  }
+}
 
-  return callback(_value);
+template <class T, class M>
+template <class F, class R, class Q>
+auto Guarded<T, M>::tryUnderLock(F&& callback) const -> std::optional<Q> {
+  // TRY_MUTEX_LOCKER(guard, _mutex);
+  auto guard = std::unique_lock<M>(_mutex, std::try_to_lock);
+
+  if (guard.owns_lock()) {
+    if constexpr (!std::is_void_v<R>) {
+      return std::forward<F>(callback)(_value);
+    } else {
+      std::forward<F>(callback)(_value);
+      return std::monostate{};
+    }
+  } else {
+    return std::nullopt;
+  }
 }
 
 template <class T, class M>
-template <class F>
-auto Guarded<T, M>::doUnderLock(F callback) -> std::result_of_t<F(T const&)> {
-  MUTEX_LOCKER(guard, _mutex);
+auto Guarded<T, M>::getLockedGuard() -> MutexGuard<T, L> {
+  return MutexGuard(_value, L(_mutex));
+}
 
-  return callback(_value);
+template <class T, class M>
+auto Guarded<T, M>::getLockedGuard() const -> MutexGuard<T const, L> {
+  return MutexGuard(_value, L(_mutex));
 }
 
-// template <class T, class M>
-// template <class F>
-// decltype(F(false)) Guarded<T, M>::tryUnderLock(F callback) {
-//   TRY_MUTEX_LOCKER(guard, _mutex);
-//
-//   return callback(guard.isLocked());
-// }
+template <class T, class M>
+auto Guarded<T, M>::tryLockedGuard() -> std::optional<MutexGuard<T, L>> {
+  auto lock = L(_mutex, std::try_lock);
+
+  if (lock.owns_lock()) {
+    return MutexGuard(_value, std::move(lock));
+  } else {
+    return std::nullopt;
+  }
+}
 
 template <class T, class M>
-auto Guarded<T, M>::getLockedGuard() -> MutexGuard<T, M> {
-  return MutexGuard<T, M>(_value, _mutex);
+auto Guarded<T, M>::tryLockedGuard() const -> std::optional<MutexGuard<const T, L>> {
+  auto lock = L(_mutex, std::try_lock);
+
+  if (lock.owns_lock()) {
+    return MutexGuard(_value, std::move(lock));
+  } else {
+    return std::nullopt;
+  }
 }
 
 }  // namespace arangodb

diff --git a/tests/Basics/GuardedTest.cpp b/tests/Basics/GuardedTest.cpp
@@ -22,47 +22,116 @@
 
 #include "gtest/gtest.h"
 
-#include "Utilities/Guarded.h"
 #include "Basics/Mutex.h"
+#include "Utilities/Guarded.h"
 
 #include <mutex>
-
-using namespace arangodb;
+#include <thread>
 
 namespace arangodb::tests {
 
-template <typename Mutex>
+template <typename M>
 class GuardedTest : public ::testing::Test {
+ protected:
+  using Mutex = M;
+};
+
+struct UnderGuard {
+  int val{};
 };
 
 TYPED_TEST_CASE_P(GuardedTest);
 
-TYPED_TEST_P(GuardedTest, test_allows_access) {
-  struct UnderGuard {
-    int val{};
-  };
-  Guarded<UnderGuard, Mutex> guardedObj{UnderGuard{.val = 1}};
+TYPED_TEST_P(GuardedTest, test_guard_allows_access) {
+  auto guardedObj = Guarded<UnderGuard, typename TestFixture::Mutex>{UnderGuard{1}};
   {
-    MutexGuard<UnderGuard, Mutex> guard = guardedObj.getLockedGuard();
+    auto guard = guardedObj.getLockedGuard();
        EXPECT_EQ(1, guard.get().val);
     guard.get().val = 2;
     EXPECT_EQ(2, guard.get().val);
   }
-  guardedObj.doUnderLock([](UnderGuard& obj) {
-    EXPECT_EQ(2, obj.val);
-    obj.val = 3;
-    EXPECT_EQ(3, obj.val);
-  });
-  // guardedObj.tryUnderLock([](UnderGuard& obj) { ... });
+}
+TYPED_TEST_P(GuardedTest, test_guard_waits_for_access) {
+  auto guardedObj = Guarded<UnderGuard, typename TestFixture::Mutex>{UnderGuard{1}};
+  // TODO Get a lock first, then assert that trying to get a guard waits for the
+  //      lock to be released.
+}
+
+TYPED_TEST_P(GuardedTest, test_do_allows_access) {
+  auto guardedObj = Guarded<UnderGuard, typename TestFixture::Mutex>{UnderGuard{1}};
   {
-    MutexGuard<UnderGuard, Mutex> guard = guardedObj.getLockedGuard();
-    EXPECT_EQ(3, guard.get().val);
+    bool didExecute = false;
+    guardedObj.doUnderLock([&didExecute](UnderGuard& obj) {
+      EXPECT_EQ(1, obj.val);
+      obj.val = 2;
+      didExecute = true;
+      EXPECT_EQ(2, obj.val);
+    });
+    EXPECT_TRUE(didExecute);
+    {
+      auto guard = guardedObj.getLockedGuard();
+      EXPECT_EQ(2, guard.get().val);
+    }
   }
 }
 
-TYPED_TEST_P(GuardedTest, test_2) {}
+TYPED_TEST_P(GuardedTest, test_do_waits_for_access) {
+  auto guardedObj = Guarded<UnderGuard, typename TestFixture::Mutex>{UnderGuard{1}};
+  // TODO Get a lock first, then assert that doUnderLock waits for the lock
+  //      to be released.
+}
+
+TYPED_TEST_P(GuardedTest, test_try_allows_access) {
+  auto guardedObj = Guarded<UnderGuard, typename TestFixture::Mutex>{UnderGuard{1}};
+  {
+    bool didExecute = false;
+    auto const res = guardedObj.tryUnderLock([&didExecute](UnderGuard& obj) {
+      EXPECT_EQ(1, obj.val);
+      obj.val = 2;
+      didExecute = true;
+      EXPECT_EQ(2, obj.val);
+    });
+    static_assert(std::is_same_v<std::optional<std::monostate> const, decltype(res)>);
+    EXPECT_EQ(didExecute, res.has_value());
+    {
+      auto guard = guardedObj.getLockedGuard();
+      if (didExecute) {
+        EXPECT_EQ(guard->val, 2);
+      } else {
+        EXPECT_EQ(guard->val, 1);
+      }
+    }
+  }
+}
+
+TYPED_TEST_P(GuardedTest, test_try_fails_access) {
+  auto guardedObj = Guarded<UnderGuard, typename TestFixture::Mutex>{UnderGuard{1}};
+  {
+    auto guard = guardedObj.getLockedGuard();
+    bool threadStarted = false;
+    bool threadFinished = false;
+    auto thr = std::thread([&] {
+      threadStarted = true;
+      bool didExecute = false;
+      auto const res = guardedObj.tryUnderLock([&didExecute](UnderGuard& obj) {
+        EXPECT_EQ(1, obj.val);
+        obj.val = 2;
+        didExecute = true;
+        EXPECT_EQ(2, obj.val);
+      });
+      static_assert(std::is_same_v<std::optional<std::monostate> const, decltype(res)>);
+      EXPECT_FALSE(res.has_value());
+      EXPECT_FALSE(didExecute);
+      threadFinished = true;
+    });
+    thr.join();
+    EXPECT_EQ(guard->val, 1);
+  }
+}
 
-REGISTER_TYPED_TEST_CASE_P(GuardedTest, test_allows_access, test_2);
+REGISTER_TYPED_TEST_CASE_P(GuardedTest, test_guard_allows_access, test_guard_waits_for_access,
+                           test_do_allows_access, test_do_waits_for_access,
+                           test_try_allows_access, test_try_fails_access);
 
 using TestedMutexes = ::testing::Types<std::mutex, arangodb::Mutex>;