From 333d579ee0cd69a49131eb212769f006f8b5b807 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Tue, 9 Jul 2019 16:49:03 +0200 Subject: [PATCH 001/517] Began work on a value guarded by mutex implementation --- lib/CMakeLists.txt | 1 + lib/Utilities/Guarded.cpp | 23 ++++++++ lib/Utilities/Guarded.h | 120 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+) create mode 100644 lib/Utilities/Guarded.cpp create mode 100644 lib/Utilities/Guarded.h diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt index fb65f6e5607b..d209905ab627 100644 --- a/lib/CMakeLists.txt +++ b/lib/CMakeLists.txt @@ -226,6 +226,7 @@ add_library(arango STATIC Ssl/SslFeature.cpp Ssl/SslInterface.cpp Ssl/ssl-helper.cpp + Utilities/Guarded.cpp Utilities/LineEditor.cpp Utilities/ScriptLoader.cpp Utilities/ShellBase.cpp diff --git a/lib/Utilities/Guarded.cpp b/lib/Utilities/Guarded.cpp new file mode 100644 index 000000000000..299efa875842 --- /dev/null +++ b/lib/Utilities/Guarded.cpp @@ -0,0 +1,23 @@ +//////////////////////////////////////////////////////////////////////////////// +/// DISCLAIMER +/// +/// Copyright 2019 ArangoDB GmbH, Cologne, Germany +/// +/// Licensed under the Apache License, Version 2.0 (the "License"); +/// you may not use this file except in compliance with the License. +/// You may obtain a copy of the License at +/// +/// http://www.apache.org/licenses/LICENSE-2.0 +/// +/// Unless required by applicable law or agreed to in writing, software +/// distributed under the License is distributed on an "AS IS" BASIS, +/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +/// See the License for the specific language governing permissions and +/// limitations under the License. +/// +/// Copyright holder is ArangoDB GmbH, Cologne, Germany +/// +/// @author Tobias Gödderz +//////////////////////////////////////////////////////////////////////////////// + +#include "Guarded.h" diff --git a/lib/Utilities/Guarded.h b/lib/Utilities/Guarded.h new file mode 100644 index 000000000000..e15f15abc112 --- /dev/null +++ b/lib/Utilities/Guarded.h @@ -0,0 +1,120 @@ +//////////////////////////////////////////////////////////////////////////////// +/// DISCLAIMER +/// +/// Copyright 2019 ArangoDB GmbH, Cologne, Germany +/// +/// Licensed under the Apache License, Version 2.0 (the "License"); +/// you may not use this file except in compliance with the License. +/// You may obtain a copy of the License at +/// +/// http://www.apache.org/licenses/LICENSE-2.0 +/// +/// Unless required by applicable law or agreed to in writing, software +/// distributed under the License is distributed on an "AS IS" BASIS, +/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +/// See the License for the specific language governing permissions and +/// limitations under the License. +/// +/// Copyright holder is ArangoDB GmbH, Cologne, Germany +/// +/// @author Tobias Gödderz +//////////////////////////////////////////////////////////////////////////////// + +#ifndef LIB_UTILITIES_GUARDED_H +#define LIB_UTILITIES_GUARDED_H + +#include "Basics/MutexLocker.h" + +#include +#include + +namespace arangodb { + +class Mutex; + +template +class MutexGuard { + public: + explicit MutexGuard(T& value, M mutex); + ~MutexGuard(); + + T& get() noexcept; + T const& get() const noexcept; + + private: + T& _value; + M& _mutex; +}; + +template +MutexGuard::MutexGuard(T& value, M mutex) : _value(value), _mutex(mutex) { + _mutex.lock(); +} + +template +MutexGuard::~MutexGuard() { + _mutex.unlock(); +} + +template +T& MutexGuard::get() noexcept { + return _value; +} + +template +T const& MutexGuard::get() const noexcept { + return _value; +} + +template +class Guarded { + public: + explicit Guarded(T&& value = T(), M&& mutex = M()); + explicit Guarded(T value = T(), M mutex = M()); + + // TODO with C++17, these could be noexcept depending on callback being noexcept + template + decltype(F()) doUnderLock(F callback); + template + decltype(F(false)) tryUnderLock(F callback); + // TODO add more types of "do under lock"? + + MutexGuard getLockedGuard(); + // TODO add more types of "get guard" (like try or eventual) + + private: + T _value; + M _mutex; +}; + +template +Guarded::Guarded(T&& value, M&& mutex) + : _value(std::forward(value)), _mutex(std::forward(mutex)) {} + +template +Guarded::Guarded(T value, M mutex) + : _value(std::move(value)), _mutex(std::move(mutex)) {} + +template +template +decltype(F()) Guarded::doUnderLock(F callback) { + MUTEX_LOCKER(guard, _mutex); + + return callback(); +} + +template +template +decltype(F(false)) Guarded::tryUnderLock(F callback) { + TRY_MUTEX_LOCKER(guard, _mutex); + + return callback(guard.isLocked()); +} +template +MutexGuard Guarded::getLockedGuard() { + return MutexGuard(_value, _mutex); +} + +} // namespace arangodb + +#endif // LIB_UTILITIES_GUARDED_H From 465775789b64342e6717afda395093a06e6a17c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Thu, 18 Jul 2019 12:32:23 +0200 Subject: [PATCH 002/517] Added (empty) test file --- tests/Basics/GuardedTest.cpp | 29 +++++++++++++++++++++++++++++ tests/CMakeLists.txt | 1 + 2 files changed, 30 insertions(+) create mode 100644 tests/Basics/GuardedTest.cpp diff --git a/tests/Basics/GuardedTest.cpp b/tests/Basics/GuardedTest.cpp new file mode 100644 index 000000000000..b930be86018c --- /dev/null +++ b/tests/Basics/GuardedTest.cpp @@ -0,0 +1,29 @@ +//////////////////////////////////////////////////////////////////////////////// +/// DISCLAIMER +/// +/// Copyright 2019 ArangoDB GmbH, Cologne, Germany +/// +/// Licensed under the Apache License, Version 2.0 (the "License"); +/// you may not use this file except in compliance with the License. +/// You may obtain a copy of the License at +/// +/// http://www.apache.org/licenses/LICENSE-2.0 +/// +/// Unless required by applicable law or agreed to in writing, software +/// distributed under the License is distributed on an "AS IS" BASIS, +/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +/// See the License for the specific language governing permissions and +/// limitations under the License. +/// +/// Copyright holder is ArangoDB GmbH, Cologne, Germany +/// +/// @author Tobias Gödderz +//////////////////////////////////////////////////////////////////////////////// + +#include "gtest/gtest.h" + +#include + +TEST(GuardedTest, basics) { + +} diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index a28e7ee82975..4fddf4847b6e 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -154,6 +154,7 @@ set(ARANGODB_TESTS_SOURCES Basics/CompileTimeStrlenTest.cpp Basics/EndpointTest.cpp Basics/FixedSizeAllocatorTest.cpp + Basics/GuardedTest.cpp Basics/InifileParserTest.cpp Basics/LoggerTest.cpp Basics/RandomTest.cpp From 86ff2c1b26e3b614b08a731422b8b82be2758800 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Wed, 9 Oct 2019 17:34:03 +0200 Subject: [PATCH 003/517] A first test, and some fixes --- lib/Utilities/Guarded.h | 46 +++++++++++++++++++++------------- tests/Basics/GuardedTest.cpp | 48 ++++++++++++++++++++++++++++++++++-- 2 files changed, 75 insertions(+), 19 deletions(-) diff --git a/lib/Utilities/Guarded.h b/lib/Utilities/Guarded.h index e15f15abc112..aa7befc2050f 100644 --- a/lib/Utilities/Guarded.h +++ b/lib/Utilities/Guarded.h @@ -35,7 +35,7 @@ class Mutex; template class MutexGuard { public: - explicit MutexGuard(T& value, M mutex); + explicit MutexGuard(T& value, M& mutex); ~MutexGuard(); T& get() noexcept; @@ -47,7 +47,7 @@ class MutexGuard { }; template -MutexGuard::MutexGuard(T& value, M mutex) : _value(value), _mutex(mutex) { +MutexGuard::MutexGuard(T& value, M& mutex) : _value(value), _mutex(mutex) { _mutex.lock(); } @@ -69,14 +69,18 @@ T const& MutexGuard::get() const noexcept { template class Guarded { public: - explicit Guarded(T&& value = T(), M&& mutex = M()); - explicit Guarded(T value = T(), M mutex = M()); + explicit Guarded(T&& value = T()); + template + explicit Guarded(Args...); // TODO with C++17, these could be noexcept depending on callback being noexcept template - decltype(F()) doUnderLock(F callback); + std::result_of_t doUnderLock(F callback); template - decltype(F(false)) tryUnderLock(F callback); + std::result_of_t doUnderLock(F callback); + + // template + // decltype(F(false)) tryUnderLock(F callback); // TODO add more types of "do under lock"? MutexGuard getLockedGuard(); @@ -88,31 +92,39 @@ class Guarded { }; template -Guarded::Guarded(T&& value, M&& mutex) - : _value(std::forward(value)), _mutex(std::forward(mutex)) {} +Guarded::Guarded(T&& value) : _value{std::move(value)}, _mutex{} {} template -Guarded::Guarded(T value, M mutex) - : _value(std::move(value)), _mutex(std::move(mutex)) {} +template +Guarded::Guarded(Args... args) : _value{args...}, _mutex{} {} template template -decltype(F()) Guarded::doUnderLock(F callback) { +std::result_of_t Guarded::doUnderLock(F callback) { MUTEX_LOCKER(guard, _mutex); - return callback(); + return callback(_value); } template template -decltype(F(false)) Guarded::tryUnderLock(F callback) { - TRY_MUTEX_LOCKER(guard, _mutex); +std::result_of_t Guarded::doUnderLock(F callback) { + MUTEX_LOCKER(guard, _mutex); - return callback(guard.isLocked()); + return callback(_value); } + +// template +// template +// decltype(F(false)) Guarded::tryUnderLock(F callback) { +// TRY_MUTEX_LOCKER(guard, _mutex); +// +// return callback(guard.isLocked()); +// } + template -MutexGuard Guarded::getLockedGuard() { - return MutexGuard(_value, _mutex); +MutexGuard Guarded::getLockedGuard() { + return MutexGuard(_value, _mutex); } } // namespace arangodb diff --git a/tests/Basics/GuardedTest.cpp b/tests/Basics/GuardedTest.cpp index b930be86018c..0a1f600dc768 100644 --- a/tests/Basics/GuardedTest.cpp +++ b/tests/Basics/GuardedTest.cpp @@ -22,8 +22,52 @@ #include "gtest/gtest.h" -#include +#include "Utilities/Guarded.h" +#include "Basics/Mutex.h" -TEST(GuardedTest, basics) { +#include +using namespace arangodb; + +namespace arangodb { +namespace tests { + +template +class GuardedTest : public ::testing::Test { +}; + +TYPED_TEST_CASE_P(GuardedTest); + +TYPED_TEST_P(GuardedTest, test_allows_access) { + struct UnderGuard { + int val{}; + }; + Guarded guardedObj{UnderGuard{.val = 1}}; + { + MutexGuard 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) { ... }); + { + MutexGuard guard = guardedObj.getLockedGuard(); + EXPECT_EQ(3, guard.get().val); + } } + +TYPED_TEST_P(GuardedTest, test_2) {} + +REGISTER_TYPED_TEST_CASE_P(GuardedTest, test_allows_access, test_2); + +using TestedMutexes = ::testing::Types; + +INSTANTIATE_TYPED_TEST_CASE_P(GuardedTestInstantiation, GuardedTest, TestedMutexes); + +} // namespace tests +} // namespace arangodb From 08adbf44ea1e389c3f57f41c292f2523d759a973 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Thu, 10 Oct 2019 09:22:14 +0200 Subject: [PATCH 004/517] Deleted copy&move constructors --- lib/Utilities/Guarded.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/Utilities/Guarded.h b/lib/Utilities/Guarded.h index aa7befc2050f..982c1c0101cd 100644 --- a/lib/Utilities/Guarded.h +++ b/lib/Utilities/Guarded.h @@ -73,6 +73,11 @@ class Guarded { template explicit Guarded(Args...); + Guarded(Guarded const&) = delete; + Guarded(Guarded&&) = delete; + Guarded& operator=(Guarded const&) = delete; + Guarded& operator=(Guarded&&) = delete; + // TODO with C++17, these could be noexcept depending on callback being noexcept template std::result_of_t doUnderLock(F callback); From ea01ee26e059150da973d232ca04ac9e14f68b32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Thu, 10 Oct 2019 09:26:09 +0200 Subject: [PATCH 005/517] explicitly defaulted destructor --- lib/Utilities/Guarded.h | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/Utilities/Guarded.h b/lib/Utilities/Guarded.h index 982c1c0101cd..cf9b60442ad6 100644 --- a/lib/Utilities/Guarded.h +++ b/lib/Utilities/Guarded.h @@ -73,6 +73,7 @@ class Guarded { template explicit Guarded(Args...); + ~Guarded() = default; Guarded(Guarded const&) = delete; Guarded(Guarded&&) = delete; Guarded& operator=(Guarded const&) = delete; From 4326973476dc83f765f4717a18373a16f301a99b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Fri, 2 Oct 2020 17:47:06 +0200 Subject: [PATCH 006/517] Switched to trailing return types --- lib/Utilities/Guarded.h | 26 ++++++++++++++------------ tests/Basics/GuardedTest.cpp | 6 ++---- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/lib/Utilities/Guarded.h b/lib/Utilities/Guarded.h index cf9b60442ad6..32bbcbea51f2 100644 --- a/lib/Utilities/Guarded.h +++ b/lib/Utilities/Guarded.h @@ -32,14 +32,16 @@ namespace arangodb { class Mutex; +// TODO should callbacks be passed as const& rather than by value? + template class MutexGuard { public: explicit MutexGuard(T& value, M& mutex); ~MutexGuard(); - T& get() noexcept; - T const& get() const noexcept; + auto get() noexcept -> T&; + auto get() const noexcept -> T const&; private: T& _value; @@ -57,12 +59,12 @@ MutexGuard::~MutexGuard() { } template -T& MutexGuard::get() noexcept { +auto MutexGuard::get() noexcept -> T& { return _value; } template -T const& MutexGuard::get() const noexcept { +auto MutexGuard::get() const noexcept -> T const& { return _value; } @@ -76,20 +78,20 @@ class Guarded { ~Guarded() = default; Guarded(Guarded const&) = delete; Guarded(Guarded&&) = delete; - Guarded& operator=(Guarded const&) = delete; - Guarded& operator=(Guarded&&) = delete; + 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 - std::result_of_t doUnderLock(F callback); + auto doUnderLock(F callback) -> std::result_of_t; template - std::result_of_t doUnderLock(F callback); + auto doUnderLock(F callback) -> std::result_of_t; // template // decltype(F(false)) tryUnderLock(F callback); // TODO add more types of "do under lock"? - MutexGuard getLockedGuard(); + auto getLockedGuard() -> MutexGuard; // TODO add more types of "get guard" (like try or eventual) private: @@ -106,7 +108,7 @@ Guarded::Guarded(Args... args) : _value{args...}, _mutex{} {} template template -std::result_of_t Guarded::doUnderLock(F callback) { +auto Guarded::doUnderLock(F callback) -> std::result_of_t { MUTEX_LOCKER(guard, _mutex); return callback(_value); @@ -114,7 +116,7 @@ std::result_of_t Guarded::doUnderLock(F callback) { template template -std::result_of_t Guarded::doUnderLock(F callback) { +auto Guarded::doUnderLock(F callback) -> std::result_of_t { MUTEX_LOCKER(guard, _mutex); return callback(_value); @@ -129,7 +131,7 @@ std::result_of_t Guarded::doUnderLock(F callback) { // } template -MutexGuard Guarded::getLockedGuard() { +auto Guarded::getLockedGuard() -> MutexGuard { return MutexGuard(_value, _mutex); } diff --git a/tests/Basics/GuardedTest.cpp b/tests/Basics/GuardedTest.cpp index 0a1f600dc768..fbff925d93e0 100644 --- a/tests/Basics/GuardedTest.cpp +++ b/tests/Basics/GuardedTest.cpp @@ -29,8 +29,7 @@ using namespace arangodb; -namespace arangodb { -namespace tests { +namespace arangodb::tests { template class GuardedTest : public ::testing::Test { @@ -69,5 +68,4 @@ using TestedMutexes = ::testing::Types; INSTANTIATE_TYPED_TEST_CASE_P(GuardedTestInstantiation, GuardedTest, TestedMutexes); -} // namespace tests -} // namespace arangodb +} // namespace arangodb::tests From b8c7f3dbb20a54f17d39909060b378c3a7e129d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Fri, 13 Nov 2020 16:04:56 +0100 Subject: [PATCH 007/517] Some improvements and more tests --- lib/Basics/Mutex.cpp | 4 + lib/Basics/Mutex.h | 1 + lib/Utilities/Guarded.h | 175 ++++++++++++++++++++++++++--------- tests/Basics/GuardedTest.cpp | 109 ++++++++++++++++++---- 4 files changed, 225 insertions(+), 64 deletions(-) diff --git a/lib/Basics/Mutex.cpp b/lib/Basics/Mutex.cpp index 4582fbc3e887..b5a650f88c34 100644 --- 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 index 7fcc9adfb7cd..7a3f46fc1f39 100644 --- 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 index 32bbcbea51f2..db75fcc7811c 100644 --- 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 #include +#include +#include +#include +#include namespace arangodb { class Mutex; -// TODO should callbacks be passed as const& rather than by value? - -template +template 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 -MutexGuard::MutexGuard(T& value, M& mutex) : _value(value), _mutex(mutex) { - _mutex.lock(); +template +MutexGuard::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 -MutexGuard::~MutexGuard() { - _mutex.unlock(); +template +auto MutexGuard::get() noexcept -> T& { + return _value; } -template -auto MutexGuard::get() noexcept -> T& { +template +auto MutexGuard::get() const noexcept -> T const& { return _value; } -template -auto MutexGuard::get() const noexcept -> T const& { - return _value; +template +auto MutexGuard::operator->() noexcept -> T* { + return std::addressof(get()); +} + +template +auto MutexGuard::operator->() const noexcept -> T const* { + return std::addressof(get()); } template @@ -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 - auto doUnderLock(F callback) -> std::result_of_t; - template - auto doUnderLock(F callback) -> std::result_of_t; + template > + auto doUnderLock(F&& callback) -> R; + template > + auto doUnderLock(F&& callback) const -> R; + + template , + class Q = std::conditional_t, std::monostate, R>> + [[nodiscard]] auto tryUnderLock(F&& callback) -> std::optional; + template , + class Q = std::conditional_t, std::monostate, R>> + [[nodiscard]] auto tryUnderLock(F&& callback) const -> std::optional; - // template - // decltype(F(false)) tryUnderLock(F callback); - // TODO add more types of "do under lock"? + using L = std::unique_lock; + auto getLockedGuard() -> MutexGuard; + auto getLockedGuard() const -> MutexGuard; - auto getLockedGuard() -> MutexGuard; + auto tryLockedGuard() -> std::optional>; + auto tryLockedGuard() const -> std::optional>; // TODO add more types of "get guard" (like try or eventual) private: @@ -107,32 +127,99 @@ template Guarded::Guarded(Args... args) : _value{args...}, _mutex{} {} template -template -auto Guarded::doUnderLock(F callback) -> std::result_of_t { - MUTEX_LOCKER(guard, _mutex); +template +auto Guarded::doUnderLock(F&& callback) -> R { + auto guard = std::unique_lock(_mutex); + //MUTEX_LOCKER(guard, _mutex); + + if constexpr (!std::is_void_v) { + return std::forward(callback)(_value); + } else { + std::forward(callback)(_value); + return; + } +} + +template +template +auto Guarded::doUnderLock(F&& callback) const -> R { + auto guard = std::unique_lock(_mutex); + // MUTEX_LOCKER(guard, _mutex); + + if constexpr (!std::is_void_v) { + return std::forward(callback)(_value); + } else { + std::forward(callback)(_value); + return; + } +} + +template +template +auto Guarded::tryUnderLock(F&& callback) -> std::optional { + auto guard = std::unique_lock(_mutex, std::try_to_lock); + // TRY_MUTEX_LOCKER(guard, _mutex); + + if (guard.owns_lock()) { + if constexpr (!std::is_void_v) { + return std::forward(callback)(_value); + } else { + std::forward(callback)(_value); + return std::monostate{}; + } + } else { + return std::nullopt; + } +} - return callback(_value); +template +template +auto Guarded::tryUnderLock(F&& callback) const -> std::optional { + // TRY_MUTEX_LOCKER(guard, _mutex); + auto guard = std::unique_lock(_mutex, std::try_to_lock); + + if (guard.owns_lock()) { + if constexpr (!std::is_void_v) { + return std::forward(callback)(_value); + } else { + std::forward(callback)(_value); + return std::monostate{}; + } + } else { + return std::nullopt; + } } template -template -auto Guarded::doUnderLock(F callback) -> std::result_of_t { - MUTEX_LOCKER(guard, _mutex); +auto Guarded::getLockedGuard() -> MutexGuard { + return MutexGuard(_value, L(_mutex)); +} - return callback(_value); +template +auto Guarded::getLockedGuard() const -> MutexGuard { + return MutexGuard(_value, L(_mutex)); } -// template -// template -// decltype(F(false)) Guarded::tryUnderLock(F callback) { -// TRY_MUTEX_LOCKER(guard, _mutex); -// -// return callback(guard.isLocked()); -// } +template +auto Guarded::tryLockedGuard() -> std::optional> { + auto lock = L(_mutex, std::try_lock); + + if (lock.owns_lock()) { + return MutexGuard(_value, std::move(lock)); + } else { + return std::nullopt; + } +} template -auto Guarded::getLockedGuard() -> MutexGuard { - return MutexGuard(_value, _mutex); +auto Guarded::tryLockedGuard() const -> std::optional> { + 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 index fbff925d93e0..49c77a9325b8 100644 --- 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 - -using namespace arangodb; +#include namespace arangodb::tests { -template +template 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 guardedObj{UnderGuard{.val = 1}}; +TYPED_TEST_P(GuardedTest, test_guard_allows_access) { + auto guardedObj = Guarded{UnderGuard{1}}; { - MutexGuard 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{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{1}}; { - MutexGuard 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{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{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 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{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 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; From 4c07156bc39243ab642cd04fad9545f6a3466b47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Thu, 19 Nov 2020 10:44:04 +0100 Subject: [PATCH 008/517] Rename arangodb::Mutex::tryLock to make Mutex more compatible with std::mutex --- lib/Basics/Mutex.cpp | 6 +----- lib/Basics/Mutex.h | 6 +++--- lib/Basics/MutexLocker.h | 2 +- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/lib/Basics/Mutex.cpp b/lib/Basics/Mutex.cpp index b5a650f88c34..fd899e86fd3b 100644 --- a/lib/Basics/Mutex.cpp +++ b/lib/Basics/Mutex.cpp @@ -84,7 +84,7 @@ void Mutex::lock() { #endif } -bool Mutex::tryLock() { +bool Mutex::try_lock() { #ifdef ARANGODB_ENABLE_DEADLOCK_DETECTION // we must not hold the lock ourselves here TRI_ASSERT(_holder != Thread::currentThreadId()); @@ -111,10 +111,6 @@ 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 index 7a3f46fc1f39..ff7db2a65424 100644 --- a/lib/Basics/Mutex.h +++ b/lib/Basics/Mutex.h @@ -54,17 +54,17 @@ namespace arangodb { class Mutex { - private: + public: Mutex(Mutex const&) = delete; Mutex& operator=(Mutex const&) = delete; - public: Mutex(); ~Mutex(); public: void lock(); - bool tryLock(); + // purposefully violate our naming convention (try_lock instead of tryLock) + // in order to be compatible with std::mutex bool try_lock(); void unlock(); diff --git a/lib/Basics/MutexLocker.h b/lib/Basics/MutexLocker.h index a0d42af3957e..304ff54ed3a9 100644 --- a/lib/Basics/MutexLocker.h +++ b/lib/Basics/MutexLocker.h @@ -127,7 +127,7 @@ class MutexLocker { bool tryLock() { TRI_ASSERT(!_isLocked); - if (_mutex->tryLock()) { + if (_mutex->try_lock()) { _isLocked = true; } return _isLocked; From 612aee7a4c0e44c5cb7d82fd161cbb9e2a1b27f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Thu, 19 Nov 2020 11:03:54 +0100 Subject: [PATCH 009/517] Moved Guarded files to Basics --- lib/{Utilities => Basics}/Guarded.cpp | 0 lib/{Utilities => Basics}/Guarded.h | 0 lib/CMakeLists.txt | 2 +- tests/Basics/GuardedTest.cpp | 2 +- 4 files changed, 2 insertions(+), 2 deletions(-) rename lib/{Utilities => Basics}/Guarded.cpp (100%) rename lib/{Utilities => Basics}/Guarded.h (100%) diff --git a/lib/Utilities/Guarded.cpp b/lib/Basics/Guarded.cpp similarity index 100% rename from lib/Utilities/Guarded.cpp rename to lib/Basics/Guarded.cpp diff --git a/lib/Utilities/Guarded.h b/lib/Basics/Guarded.h similarity index 100% rename from lib/Utilities/Guarded.h rename to lib/Basics/Guarded.h diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt index d209905ab627..6a8940b61d78 100644 --- a/lib/CMakeLists.txt +++ b/lib/CMakeLists.txt @@ -126,6 +126,7 @@ add_library(arango STATIC Basics/Exceptions.cpp Basics/FileUtils.cpp Basics/FunctionUtils.cpp + Basics/Guarded.cpp Basics/HybridLogicalClock.cpp Basics/LdapUrlParser.cpp Basics/LocalTaskQueue.cpp @@ -226,7 +227,6 @@ add_library(arango STATIC Ssl/SslFeature.cpp Ssl/SslInterface.cpp Ssl/ssl-helper.cpp - Utilities/Guarded.cpp Utilities/LineEditor.cpp Utilities/ScriptLoader.cpp Utilities/ShellBase.cpp diff --git a/tests/Basics/GuardedTest.cpp b/tests/Basics/GuardedTest.cpp index 49c77a9325b8..1dd3f0f85c33 100644 --- a/tests/Basics/GuardedTest.cpp +++ b/tests/Basics/GuardedTest.cpp @@ -22,8 +22,8 @@ #include "gtest/gtest.h" +#include "Basics/Guarded.h" #include "Basics/Mutex.h" -#include "Utilities/Guarded.h" #include #include From d691764d02865b589f93028872de752e524f598f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Thu, 19 Nov 2020 11:08:42 +0100 Subject: [PATCH 010/517] minor cleanup --- lib/Basics/Guarded.h | 6 ------ 1 file changed, 6 deletions(-) diff --git a/lib/Basics/Guarded.h b/lib/Basics/Guarded.h index db75fcc7811c..7d9303503945 100644 --- a/lib/Basics/Guarded.h +++ b/lib/Basics/Guarded.h @@ -23,8 +23,6 @@ #ifndef LIB_UTILITIES_GUARDED_H #define LIB_UTILITIES_GUARDED_H -// #include "Basics/MutexLocker.h" - #include #include #include @@ -130,7 +128,6 @@ template template auto Guarded::doUnderLock(F&& callback) -> R { auto guard = std::unique_lock(_mutex); - //MUTEX_LOCKER(guard, _mutex); if constexpr (!std::is_void_v) { return std::forward(callback)(_value); @@ -144,7 +141,6 @@ template template auto Guarded::doUnderLock(F&& callback) const -> R { auto guard = std::unique_lock(_mutex); - // MUTEX_LOCKER(guard, _mutex); if constexpr (!std::is_void_v) { return std::forward(callback)(_value); @@ -158,7 +154,6 @@ template template auto Guarded::tryUnderLock(F&& callback) -> std::optional { auto guard = std::unique_lock(_mutex, std::try_to_lock); - // TRY_MUTEX_LOCKER(guard, _mutex); if (guard.owns_lock()) { if constexpr (!std::is_void_v) { @@ -175,7 +170,6 @@ auto Guarded::tryUnderLock(F&& callback) -> std::optional { template template auto Guarded::tryUnderLock(F&& callback) const -> std::optional { - // TRY_MUTEX_LOCKER(guard, _mutex); auto guard = std::unique_lock(_mutex, std::try_to_lock); if (guard.owns_lock()) { From 1eec0966f1a7370948565b33ee3fedf9c69be6cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Thu, 19 Nov 2020 12:00:44 +0100 Subject: [PATCH 011/517] added comment --- lib/Basics/Guarded.h | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/lib/Basics/Guarded.h b/lib/Basics/Guarded.h index 7d9303503945..1921801ea7ac 100644 --- a/lib/Basics/Guarded.h +++ b/lib/Basics/Guarded.h @@ -30,6 +30,37 @@ #include #include +/** + * SYNOPSIS + * + * The class Guarded contains a value and an associated mutex. It does allow + * access to the value only while owning a lock to the mutex. + * + * For example, given a + * + * struct UnderGuard { int value; } + * + * , define a guarded object as follows: + * + * Guarded guarded(7); + * + * The constructor's arguments will be forwarded. + * + * It can either be accessed by passing a lambda: + * + * guarded.doUnderLock([](UnderGuard& obj) { obj.value = 12; }); + * + * This will lock the mutex before the lambda's execution, and release it after. + * + * Or it can be accessed by creating a MutexGuard: + * + * auto mutexGuard = guarded.getLockedGuard(); + * mutexGuard->value = 13; + * + * getLockedGuard() will lock the mutex, and mutexGuard will release it upon + * destruction. + */ + namespace arangodb { class Mutex; From 6ea04ccc9173930b1e9be88d7e5fc8dc93431108 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Thu, 19 Nov 2020 15:52:02 +0100 Subject: [PATCH 012/517] parametrized lock --- lib/Basics/Guarded.h | 85 +++++++++++++++++++----------------- tests/Basics/GuardedTest.cpp | 42 ++++++++++++------ 2 files changed, 74 insertions(+), 53 deletions(-) diff --git a/lib/Basics/Guarded.h b/lib/Basics/Guarded.h index 1921801ea7ac..f6ed4549794e 100644 --- a/lib/Basics/Guarded.h +++ b/lib/Basics/Guarded.h @@ -23,6 +23,8 @@ #ifndef LIB_UTILITIES_GUARDED_H #define LIB_UTILITIES_GUARDED_H +#include "Basics/system-compiler.h" + #include #include #include @@ -110,9 +112,13 @@ auto MutexGuard::operator->() const noexcept -> T const* { return std::addressof(get()); } -template +template class L = std::unique_lock> class Guarded { public: + using value_type = T; + using mutex_type = M; + using lock_type = L; + explicit Guarded(T&& value = T()); template explicit Guarded(Args...); @@ -123,42 +129,41 @@ class Guarded { auto operator=(Guarded const&) -> Guarded& = delete; auto operator=(Guarded&&) -> Guarded& = delete; - template > + template > auto doUnderLock(F&& callback) -> R; - template > + template > auto doUnderLock(F&& callback) const -> R; - template , + template , class Q = std::conditional_t, std::monostate, R>> [[nodiscard]] auto tryUnderLock(F&& callback) -> std::optional; - template , + template , class Q = std::conditional_t, std::monostate, R>> [[nodiscard]] auto tryUnderLock(F&& callback) const -> std::optional; - using L = std::unique_lock; - auto getLockedGuard() -> MutexGuard; - auto getLockedGuard() const -> MutexGuard; + auto getLockedGuard() -> MutexGuard; + auto getLockedGuard() const -> MutexGuard; - auto tryLockedGuard() -> std::optional>; - auto tryLockedGuard() const -> std::optional>; + auto tryLockedGuard() -> std::optional>; + auto tryLockedGuard() const -> std::optional>; // TODO add more types of "get guard" (like try or eventual) private: - T _value; - M _mutex; + value_type _value; + mutex_type _mutex; }; -template -Guarded::Guarded(T&& value) : _value{std::move(value)}, _mutex{} {} +template class L> +Guarded::Guarded(T&& value) : _value{std::move(value)}, _mutex{} {} -template +template class L> template -Guarded::Guarded(Args... args) : _value{args...}, _mutex{} {} +Guarded::Guarded(Args... args) : _value{args...}, _mutex{} {} -template +template class L> template -auto Guarded::doUnderLock(F&& callback) -> R { - auto guard = std::unique_lock(_mutex); +auto Guarded::doUnderLock(F&& callback) -> R { + auto guard = L(_mutex); if constexpr (!std::is_void_v) { return std::forward(callback)(_value); @@ -168,10 +173,10 @@ auto Guarded::doUnderLock(F&& callback) -> R { } } -template +template class L> template -auto Guarded::doUnderLock(F&& callback) const -> R { - auto guard = std::unique_lock(_mutex); +auto Guarded::doUnderLock(F&& callback) const -> R { + auto guard = L(_mutex); if constexpr (!std::is_void_v) { return std::forward(callback)(_value); @@ -181,10 +186,10 @@ auto Guarded::doUnderLock(F&& callback) const -> R { } } -template +template class L> template -auto Guarded::tryUnderLock(F&& callback) -> std::optional { - auto guard = std::unique_lock(_mutex, std::try_to_lock); +auto Guarded::tryUnderLock(F&& callback) -> std::optional { + auto guard = L(_mutex, std::try_to_lock); if (guard.owns_lock()) { if constexpr (!std::is_void_v) { @@ -198,10 +203,10 @@ auto Guarded::tryUnderLock(F&& callback) -> std::optional { } } -template +template class L> template -auto Guarded::tryUnderLock(F&& callback) const -> std::optional { - auto guard = std::unique_lock(_mutex, std::try_to_lock); +auto Guarded::tryUnderLock(F&& callback) const -> std::optional { + auto guard = lock_type(_mutex, std::try_to_lock); if (guard.owns_lock()) { if constexpr (!std::is_void_v) { @@ -215,19 +220,19 @@ auto Guarded::tryUnderLock(F&& callback) const -> std::optional { } } -template -auto Guarded::getLockedGuard() -> MutexGuard { - return MutexGuard(_value, L(_mutex)); +template class L> +auto Guarded::getLockedGuard() -> MutexGuard> { + return MutexGuard(_value, lock_type{_mutex}); } -template -auto Guarded::getLockedGuard() const -> MutexGuard { - return MutexGuard(_value, L(_mutex)); +template class L> +auto Guarded::getLockedGuard() const -> MutexGuard> { + return MutexGuard(_value, lock_type(_mutex)); } -template -auto Guarded::tryLockedGuard() -> std::optional> { - auto lock = L(_mutex, std::try_lock); +template class L> +auto Guarded::tryLockedGuard() -> std::optional>> { + auto lock = lock_type(_mutex, std::try_lock); if (lock.owns_lock()) { return MutexGuard(_value, std::move(lock)); @@ -236,9 +241,9 @@ auto Guarded::tryLockedGuard() -> std::optional> { } } -template -auto Guarded::tryLockedGuard() const -> std::optional> { - auto lock = L(_mutex, std::try_lock); +template class L> +auto Guarded::tryLockedGuard() const -> std::optional>> { + auto lock = lock_type(_mutex, std::try_lock); if (lock.owns_lock()) { return MutexGuard(_value, std::move(lock)); diff --git a/tests/Basics/GuardedTest.cpp b/tests/Basics/GuardedTest.cpp index 1dd3f0f85c33..69d03d2018b3 100644 --- a/tests/Basics/GuardedTest.cpp +++ b/tests/Basics/GuardedTest.cpp @@ -24,26 +24,33 @@ #include "Basics/Guarded.h" #include "Basics/Mutex.h" +#include "Basics/MutexLocker.h" #include #include namespace arangodb::tests { -template -class GuardedTest : public ::testing::Test { - protected: - using Mutex = M; -}; struct UnderGuard { int val{}; }; +template +class GuardedTest : public ::testing::Test { + protected: + + using Mutex = typename T::first_type; + template + using Lock = typename T::second_type::template instantiate; + template + using Guarded = Guarded; +}; + TYPED_TEST_CASE_P(GuardedTest); TYPED_TEST_P(GuardedTest, test_guard_allows_access) { - auto guardedObj = Guarded{UnderGuard{1}}; + auto guardedObj = typename TestFixture::template Guarded{1}; { auto guard = guardedObj.getLockedGuard(); EXPECT_EQ(1, guard.get().val); @@ -52,13 +59,13 @@ TYPED_TEST_P(GuardedTest, test_guard_allows_access) { } } TYPED_TEST_P(GuardedTest, test_guard_waits_for_access) { - auto guardedObj = Guarded{UnderGuard{1}}; + auto guardedObj = typename TestFixture::template Guarded{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{1}}; + auto guardedObj = typename TestFixture::template Guarded{1}; { bool didExecute = false; guardedObj.doUnderLock([&didExecute](UnderGuard& obj) { @@ -76,13 +83,13 @@ TYPED_TEST_P(GuardedTest, test_do_allows_access) { } TYPED_TEST_P(GuardedTest, test_do_waits_for_access) { - auto guardedObj = Guarded{UnderGuard{1}}; + auto guardedObj = typename TestFixture::template Guarded{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{1}}; + auto guardedObj = typename TestFixture::template Guarded{1}; { bool didExecute = false; auto const res = guardedObj.tryUnderLock([&didExecute](UnderGuard& obj) { @@ -105,7 +112,7 @@ TYPED_TEST_P(GuardedTest, test_try_allows_access) { } TYPED_TEST_P(GuardedTest, test_try_fails_access) { - auto guardedObj = Guarded{UnderGuard{1}}; + auto guardedObj = typename TestFixture::template Guarded{1}; { auto guard = guardedObj.getLockedGuard(); bool threadStarted = false; @@ -133,8 +140,17 @@ REGISTER_TYPED_TEST_CASE_P(GuardedTest, test_guard_allows_access, test_guard_wai test_do_allows_access, test_do_waits_for_access, test_try_allows_access, test_try_fails_access); -using TestedMutexes = ::testing::Types; +template