From: Razvan Becheriu Date: Mon, 15 Jun 2020 15:59:42 +0000 (+0300) Subject: [#1239] addressed comments X-Git-Tag: Kea-1.7.9~51 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=64ca9fbb493802b703381bc9ab5a001f7dde50e4;p=thirdparty%2Fkea.git [#1239] addressed comments --- diff --git a/src/lib/util/tests/Makefile.am b/src/lib/util/tests/Makefile.am index 6c5e9638af..a5668e3334 100644 --- a/src/lib/util/tests/Makefile.am +++ b/src/lib/util/tests/Makefile.am @@ -59,6 +59,7 @@ run_unittests_SOURCES += range_utilities_unittest.cc run_unittests_SOURCES += readwrite_mutex_unittest.cc run_unittests_SOURCES += signal_set_unittest.cc run_unittests_SOURCES += stopwatch_unittest.cc +run_unittests_SOURCES += unlock_guard_unittests.cc run_unittests_SOURCES += versioned_csv_file_unittest.cc run_unittests_SOURCES += watch_socket_unittests.cc run_unittests_SOURCES += watched_thread_unittest.cc diff --git a/src/lib/util/tests/unlock_guard_unittests.cc b/src/lib/util/tests/unlock_guard_unittests.cc new file mode 100644 index 0000000000..8ace16bbe2 --- /dev/null +++ b/src/lib/util/tests/unlock_guard_unittests.cc @@ -0,0 +1,239 @@ +// Copyright (C) 2020 Internet Systems Consortium, Inc. ("ISC") +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +#include + +#include + +#include +#include + +#include +#include +#include +#include +#include + +using namespace isc::util; +using namespace std; + +namespace { + +/// @brief test mutex class used to check the internal state of a 'fictional' +/// mutex so that the functionality of the UnlockGuard can be tested +/// @node the test mutex can be recursive which means that a lock can be called +/// on the same thread and not resulting in a dead lock +class TestMutex { +public: + /// @brief Constructor + /// + /// @param recursive sets the mutex as recursive mutex + TestMutex(bool recursive = false) : lock_(0), dead_lock_(false), + lock_count_(0), unlock_count_(0), recursive_(recursive) { + } + + /// @brief lock the mutex + void lock() { + lock_guard lk(mutex_); + if (lock_ >= 1) { + // mutex is already locked + if (!recursive_) { + // lock on a non-recursive mutex resulting in a dead lock + dead_lock_ = true; + isc_throw(isc::InvalidOperation, + "recursive lock on already locked mutex resulting in " + "dead lock"); + } else { + // lock on a recursive mutex + if (this_thread::get_id() != id_) { + // lock on a recursive mutex on a different thread resulting + // in a dead lock + dead_lock_ = true; + isc_throw(isc::InvalidOperation, + "recursive lock on a different thread on already " + "locked mutex resulting in dead lock"); + } + } + } + // increment the total number of locks + lock_count_++; + // increment the lock state + lock_++; + // save the thread id + id_ = this_thread::get_id(); + } + + /// @brief unlock the mutex + void unlock() { + lock_guard lk(mutex_); + if (lock_ <= 0) { + // unlock an unlocked mutex + isc_throw(isc::InvalidOperation, "unlock on non locked mutex " + "resulting in undefined behavior"); + } + if (lock_ == 1) { + // only one thread has the lock + // self healing mutex reseting the dead lock flag + dead_lock_ = false; + // reset the thread id + id_ = std::thread::id(); + } + // increment the total number of unlocks + unlock_count_++; + // decrement the lock state + lock_--; + } + + /// @brief get the mutex lock state + /// + /// @return the mutex lock state + int32_t getLock() { + lock_guard lk(mutex_); + return lock_; + } + + /// @brief get the mutex dead lock state + /// + /// @return the mutex dead lock state + bool getDeadLock() { + lock_guard lk(mutex_); + return dead_lock_; + } + + /// @brief get the number of locks performed on mutex + /// + /// @return the mutex number of locks + uint32_t getLockCount() { + lock_guard lk(mutex_); + return lock_count_; + } + + /// @brief get the number of unlocks performed on mutex + /// + /// @return the mutex number of unlocks + uint32_t getUnlockCount() { + lock_guard lk(mutex_); + return unlock_count_; + } + + /// @brief test the internal state of the mutex + /// + /// @param expected_lock check equality of this value with lock state + /// @param expected_lock_count check equality of this value with lock count + /// @param expected_unlock_count check equality of this value with unlock count + /// @param expected_dead_lock check equality of this value with dead lock state + void testMutexState(int32_t expected_lock, + uint32_t expected_lock_count, + uint32_t expected_unlock_count, + bool expected_dead_lock) { + ASSERT_EQ(getLock(), expected_lock); + ASSERT_EQ(getLockCount(), expected_lock_count); + ASSERT_EQ(getUnlockCount(), expected_unlock_count); + ASSERT_EQ(getDeadLock(), expected_dead_lock); + } + +private: + /// @brief internal lock state of the mutex + int32_t lock_; + + /// @brief state which indicates that the mutex is in dead lock + bool dead_lock_; + + /// @brief total number of locks performed on the mutex + uint32_t lock_count_; + + /// @brief total number of unlocks performed on the mutex + uint32_t unlock_count_; + + /// @brief flag to indicate if the mutex is recursive or not + bool recursive_; + + /// @brief mutex used to keep the internal state consistent + mutex mutex_; + + /// @brief the id of the thread holding the mutex + std::thread::id id_; +}; + +/// @brief Test Fixture for testing isc::util::UnlockGuard +class UnlockGuardTest : public ::testing::Test { +}; + +/// @brief test TestMutex functionality with non-recursive mutex, and recursive +/// mutex +TEST_F(UnlockGuardTest, testMutex) { + shared_ptr test_mutex; + // test non-recursive lock + test_mutex = make_shared(); + test_mutex->testMutexState(0, 0, 0, false); + { + // call lock_guard constructor which locks mutex + lock_guard lock(*test_mutex.get()); + // expect lock 1 lock_count 1 unlock_count 0 dead_lock false + test_mutex->testMutexState(1, 1, 0, false); + { + // call lock_guard constructor which locks mutex resulting in an + // exception as the mutex is already locked (dead lock) + EXPECT_THROW(lock_guard lock(*test_mutex.get()), + isc::InvalidOperation); + // expect lock 1 lock_count 1 unlock_count 0 dead_lock true + // you should not be able to get here...using a real mutex + test_mutex->testMutexState(1, 1, 0, true); + } + // expect lock 1 lock_count 1 unlock_count 0 dead_lock true + // you should not be able to get here...using a real mutex + test_mutex->testMutexState(1, 1, 0, true); + } + // expect lock 0 lock_count 1 unlock_count 1 dead_lock false + // the implementation is self healing when completely unlocking the mutex + test_mutex->testMutexState(0, 1, 1, false); + // test recursive lock + test_mutex = make_shared(true); + test_mutex->testMutexState(0, 0, 0, false); + { + // call lock_guard constructor which locks mutex + lock_guard lock(*test_mutex.get()); + // expect lock 1 lock_count 1 unlock_count 0 dead_lock false + test_mutex->testMutexState(1, 1, 0, false); + { + // call lock_guard constructor which locks mutex but does not block + // as this is done on the same thread and the mutex is recursive + EXPECT_NO_THROW(lock_guard lock(*test_mutex.get())); + // expect lock 1 lock_count 2 unlock_count 1 dead_lock false + // the destructor was already called in EXPECT_NO_THROW scope + test_mutex->testMutexState(1, 2, 1, false); + } + // expect lock 1 lock_count 2 unlock_count 1 dead_lock false + test_mutex->testMutexState(1, 2, 1, false); + } + // expect lock 0 lock_count 2 unlock_count 2 dead_lock false + test_mutex->testMutexState(0, 2, 2, false); +} + +/// @brief test UnlockGuard functionality with non-recursive mutex +TEST_F(UnlockGuardTest, testUnlockGuard) { + shared_ptr test_mutex; + // test non-recursive lock + test_mutex = make_shared(); + test_mutex->testMutexState(0, 0, 0, false); + { + // call lock_guard constructor which locks mutex + lock_guard lock(*test_mutex.get()); + // expect lock 1 lock_count 1 unlock_count 0 dead_lock false + test_mutex->testMutexState(1, 1, 0, false); + { + UnlockGuard unlock_guard(*test_mutex.get()); + // expect lock 0 lock_count 1 unlock_count 1 dead_lock false + test_mutex->testMutexState(0, 1, 1, false); + } + // expect lock 1 lock_count 2 unlock_count 1 dead_lock false + test_mutex->testMutexState(1, 2, 1, false); + } + // expect lock 0 lock_count 2 unlock_count 2 dead_lock false + test_mutex->testMutexState(0, 2, 2, false); +} + +} // namespace diff --git a/src/lib/util/unlock_guard.h b/src/lib/util/unlock_guard.h index dcbea94d9b..30be51486c 100644 --- a/src/lib/util/unlock_guard.h +++ b/src/lib/util/unlock_guard.h @@ -15,13 +15,17 @@ namespace util { /// @brief Unlock Guard. /// /// Acts as a reverse std::lock_guard. -template +/// +/// @tparam Mutex a mutex object. +template class UnlockGuard : public boost::noncopyable { public: /// @brief Constructor. /// /// Unlock mutex object on constructor. - explicit UnlockGuard(Lock& lock) : lock_(lock) { + /// + /// @param lock the mutex used for unlocking and locking. + explicit UnlockGuard(Mutex& lock) : lock_(lock) { lock_.unlock(); } @@ -34,7 +38,7 @@ public: private: /// @brief The mutex object. - Lock& lock_; + Mutex& lock_; }; } // namespace util