From: Joel Rosdahl Date: Sun, 13 Nov 2022 08:02:16 +0000 (+0100) Subject: refactor: Make LockFile guard itself without a LockFileGuard X-Git-Tag: v4.8~84 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c78a0825cc3a0b2216000b7a310aecf40c1d1aaa;p=thirdparty%2Fccache.git refactor: Make LockFile guard itself without a LockFileGuard --- diff --git a/src/storage/local/StatsFile.cpp b/src/storage/local/StatsFile.cpp index 4332d80ed..42987becc 100644 --- a/src/storage/local/StatsFile.cpp +++ b/src/storage/local/StatsFile.cpp @@ -62,9 +62,8 @@ std::optional StatsFile::update( std::function function) const { - util::ShortLivedLockFile lock_file(m_path); - util::LockFileGuard lock(lock_file); - if (!lock.acquired()) { + util::ShortLivedLockFile lock(m_path); + if (!lock.acquire()) { LOG("Failed to acquire lock for {}", m_path); return std::nullopt; } diff --git a/src/test_lockfile.cpp b/src/test_lockfile.cpp index 3240654a1..2e48fbb00 100644 --- a/src/test_lockfile.cpp +++ b/src/test_lockfile.cpp @@ -41,39 +41,33 @@ main(int argc, char** argv) const std::string path(argv[1]); const auto seconds = util::parse_signed(argv[2]); - const bool long_lived = std::string(argv[3]) == "long"; + const auto lock_type = std::string(argv[3]) == "long" + ? util::LockFile::Type::long_lived + : util::LockFile::Type::short_lived; const bool blocking = std::string(argv[4]) == "blocking"; if (!seconds) { PRINT_RAW(stderr, "Error: Failed to parse seconds\n"); return 1; } - using LockFilePtr = std::unique_ptr; - LockFilePtr lock_file; - lock_file = long_lived - ? LockFilePtr{std::make_unique(path)} - : LockFilePtr{std::make_unique(path)}; - const auto mode = blocking ? util::LockFileGuard::Mode::blocking - : util::LockFileGuard::Mode::non_blocking; - - PRINT(stdout, "{}\n", blocking ? "Acquiring" : "Trying to acquire"); - bool acquired = false; - { - util::LockFileGuard lock(*lock_file, mode); - acquired = lock.acquired(); - if (acquired) { - PRINT_RAW(stdout, "Acquired\n"); - PRINT( - stdout, "Sleeping {} second{}\n", *seconds, *seconds == 1 ? "" : "s"); - std::this_thread::sleep_for(std::chrono::seconds{*seconds}); - } else { - PRINT(stdout, "{} acquire\n", blocking ? "Failed to" : "Did not"); - } - if (acquired) { - PRINT_RAW(stdout, "Releasing\n"); - } + util::LockFile lock(path, lock_type); + if (blocking) { + PRINT_RAW(stdout, "Acquiring\n"); + lock.acquire(); + } else { + PRINT_RAW(stdout, "Trying to acquire\n"); + lock.try_acquire(); + } + if (lock.acquired()) { + PRINT_RAW(stdout, "Acquired\n"); + PRINT(stdout, "Sleeping {} second{}\n", *seconds, *seconds == 1 ? "" : "s"); + std::this_thread::sleep_for(std::chrono::seconds{*seconds}); + } else { + PRINT(stdout, "{} acquire\n", blocking ? "Failed to" : "Did not"); } - if (acquired) { + if (lock.acquired()) { + PRINT_RAW(stdout, "Releasing\n"); + lock.release(); PRINT_RAW(stdout, "Released\n"); } } diff --git a/src/util/LockFile.cpp b/src/util/LockFile.cpp index 4636e68e5..90c3020b1 100644 --- a/src/util/LockFile.cpp +++ b/src/util/LockFile.cpp @@ -72,13 +72,11 @@ private: namespace util { -LockFile::LockFile(const std::string& path) - : +LockFile::LockFile(const std::string& path, [[maybe_unused]] Type type) + : m_lock_file(path + ".lock"), #ifndef _WIN32 + m_type(type), m_alive_file(path + ".alive"), -#endif - m_lock_file(path + ".lock"), -#ifndef _WIN32 m_acquired(false) #else m_handle(INVALID_HANDLE_VALUE) @@ -109,7 +107,16 @@ LockFile::release() LOG("Releasing {}", m_lock_file); #ifndef _WIN32 - on_before_release(); + if (m_type == Type::long_lived && m_keep_alive_thread.joinable()) { + { + std::unique_lock lock(m_stop_keep_alive_mutex); + m_stop_keep_alive = true; + } + m_stop_keep_alive_condition.notify_one(); + m_keep_alive_thread.join(); + + Util::unlink_tmp(m_alive_file); + } Util::unlink_tmp(m_lock_file); #else CloseHandle(m_handle); @@ -142,9 +149,34 @@ LockFile::acquire(const bool blocking) #else m_handle = do_acquire(blocking); #endif + if (acquired()) { LOG("Acquired {}", m_lock_file); - on_after_acquire(); +#ifndef _WIN32 + if (m_type == Type::long_lived) { + const auto result = util::write_file(m_alive_file, ""); + if (!result) { + LOG("Failed to write {}: {}", m_alive_file, result.error()); + } + + LOG_RAW("Starting keep-alive thread"); + m_keep_alive_thread = std::thread([&] { + while (true) { + std::unique_lock lock(m_stop_keep_alive_mutex); + m_stop_keep_alive_condition.wait_for( + lock, + std::chrono::seconds(k_keep_alive_interval.sec()) + + std::chrono::nanoseconds(k_keep_alive_interval.nsec()), + [this] { return m_stop_keep_alive; }); + if (m_stop_keep_alive) { + return; + } + util::set_timestamps(m_alive_file); + } + }); + LOG_RAW("Started keep-alive thread"); + } +#endif } else { LOG("Failed to acquire lock {}", m_lock_file); } @@ -248,7 +280,7 @@ LockFile::do_acquire(const bool blocking) m_lock_file, inactive_duration.sec(), inactive_duration.nsec() / 1'000'000); - if (!on_before_break() || !Util::unlink_tmp(m_lock_file)) { + if (!Util::unlink_tmp(m_alive_file) || !Util::unlink_tmp(m_lock_file)) { return false; } @@ -347,84 +379,4 @@ LockFile::do_acquire(const bool blocking) #endif // !_WIN32 -ShortLivedLockFile::ShortLivedLockFile(const std::string& path) : LockFile(path) -{ -} - -LongLivedLockFile::LongLivedLockFile(const std::string& path) : LockFile(path) -{ -} - -#ifndef _WIN32 - -void -LongLivedLockFile::on_after_acquire() -{ - const auto result = util::write_file(m_alive_file, ""); - if (!result) { - LOG("Failed to write {}: {}", m_alive_file, result.error()); - } - - LOG_RAW("Starting keep-alive thread"); - m_keep_alive_thread = std::thread([&] { - while (true) { - std::unique_lock lock(m_stop_keep_alive_mutex); - m_stop_keep_alive_condition.wait_for( - lock, - std::chrono::seconds(k_keep_alive_interval.sec()) - + std::chrono::nanoseconds(k_keep_alive_interval.nsec()), - [this] { return m_stop_keep_alive; }); - if (m_stop_keep_alive) { - return; - } - util::set_timestamps(m_alive_file); - } - }); - LOG_RAW("Started keep-alive thread"); -} - -void -LongLivedLockFile::on_before_release() -{ - if (m_keep_alive_thread.joinable()) { - { - std::unique_lock lock(m_stop_keep_alive_mutex); - m_stop_keep_alive = true; - } - m_stop_keep_alive_condition.notify_one(); - m_keep_alive_thread.join(); - - Util::unlink_tmp(m_alive_file); - } -} - -bool -LongLivedLockFile::on_before_break() -{ - return Util::unlink_tmp(m_alive_file); -} - -#endif - -LockFileGuard::LockFileGuard(LockFile& lock_file, Mode mode) - : m_lock_file(lock_file) -{ - if (mode == Mode::blocking) { - lock_file.acquire(); - } else { - lock_file.try_acquire(); - } -} - -LockFileGuard::~LockFileGuard() noexcept -{ - m_lock_file.release(); -} - -bool -LockFileGuard::acquired() const -{ - return m_lock_file.acquired(); -} - } // namespace util diff --git a/src/util/LockFile.hpp b/src/util/LockFile.hpp index d5e2f85ae..b6cbae7ba 100644 --- a/src/util/LockFile.hpp +++ b/src/util/LockFile.hpp @@ -32,7 +32,12 @@ namespace util { class LockFile : NonCopyable { public: - virtual ~LockFile() noexcept = default; + enum class Type { long_lived, short_lived }; + + LockFile(const std::string& path, Type type); + + // Release the lock if previously acquired. + ~LockFile(); // Acquire lock, blocking. Returns true if acquired, otherwise false. bool acquire(); @@ -40,33 +45,30 @@ public: // Acquire lock, non-blocking. Returns true if acquired, otherwise false. bool try_acquire(); - // Release lock. If not previously acquired, nothing happens. + // Release lock early. If not previously acquired, nothing happens. void release(); - // Return whether the lock was acquired successfully. + // Return whether the lock is acquired successfully. bool acquired() const; -protected: - LockFile(const std::string& path); -#ifndef _WIN32 - std::string m_alive_file; -#endif - private: std::string m_lock_file; #ifndef _WIN32 + Type m_type; + std::string m_alive_file; bool m_acquired; + std::thread m_keep_alive_thread; + std::mutex m_stop_keep_alive_mutex; + bool m_stop_keep_alive = false; + std::condition_variable m_stop_keep_alive_condition; #else void* m_handle; #endif bool acquire(bool blocking); - virtual void on_after_acquire(); - virtual void on_before_release(); #ifndef _WIN32 bool do_acquire(bool blocking); std::optional get_last_lock_update(); - virtual bool on_before_break(); #else void* do_acquire(bool blocking); #endif @@ -90,52 +92,21 @@ class LongLivedLockFile : public LockFile { public: LongLivedLockFile(const std::string& path); - -private: -#ifndef _WIN32 - std::thread m_keep_alive_thread; - std::mutex m_stop_keep_alive_mutex; - bool m_stop_keep_alive = false; - std::condition_variable m_stop_keep_alive_condition; - - void on_after_acquire() override; - void on_before_release() override; - bool on_before_break() override; -#endif -}; - -class LockFileGuard : NonCopyable -{ -public: - enum class Mode { blocking, non_blocking }; - - LockFileGuard(LockFile& lock_file, Mode mode = Mode::blocking); - ~LockFileGuard() noexcept; - - bool acquired() const; - -private: - LockFile& m_lock_file; }; -inline void -LockFile::on_after_acquire() +inline LockFile::~LockFile() { + release(); } -inline void -LockFile::on_before_release() +inline ShortLivedLockFile::ShortLivedLockFile(const std::string& path) + : LockFile(path, Type::short_lived) { } -#ifndef _WIN32 - -inline bool -LockFile::on_before_break() +inline LongLivedLockFile::LongLivedLockFile(const std::string& path) + : LockFile(path, Type::long_lived) { - return true; } -#endif - } // namespace util diff --git a/unittest/test_util_LockFile.cpp b/unittest/test_util_LockFile.cpp index ff3153c66..7bed76629 100644 --- a/unittest/test_util_LockFile.cpp +++ b/unittest/test_util_LockFile.cpp @@ -40,14 +40,13 @@ TEST_CASE("Acquire and release short-lived lock file") { TestContext test_context; - util::ShortLivedLockFile lock_file("test"); + util::ShortLivedLockFile lock("test"); { - CHECK(!lock_file.acquired()); + CHECK(!lock.acquired()); CHECK(!Stat::lstat("test.lock")); CHECK(!Stat::lstat("test.alive")); - util::LockFileGuard lock(lock_file); - CHECK(lock_file.acquired()); + CHECK(lock.acquire()); CHECK(lock.acquired()); CHECK(!Stat::lstat("test.alive")); const auto st = Stat::lstat("test.lock"); @@ -59,7 +58,9 @@ TEST_CASE("Acquire and release short-lived lock file") #endif } - CHECK(!lock_file.acquired()); + lock.release(); + lock.release(); + CHECK(!lock.acquired()); CHECK(!Stat::lstat("test.lock")); CHECK(!Stat::lstat("test.alive")); } @@ -94,14 +95,13 @@ TEST_CASE("Acquire and release long-lived lock file") { TestContext test_context; - util::LongLivedLockFile lock_file("test"); + util::LongLivedLockFile lock("test"); { - CHECK(!lock_file.acquired()); + CHECK(!lock.acquired()); CHECK(!Stat::lstat("test.lock")); CHECK(!Stat::lstat("test.alive")); - util::LockFileGuard lock(lock_file); - CHECK(lock_file.acquired()); + CHECK(lock.acquire()); CHECK(lock.acquired()); #ifndef _WIN32 CHECK(Stat::lstat("test.alive")); @@ -115,7 +115,9 @@ TEST_CASE("Acquire and release long-lived lock file") #endif } - CHECK(!lock_file.acquired()); + lock.release(); + lock.release(); + CHECK(!lock.acquired()); CHECK(!Stat::lstat("test.lock")); CHECK(!Stat::lstat("test.alive")); } @@ -124,9 +126,8 @@ TEST_CASE("LockFile creates missing directories") { TestContext test_context; - util::ShortLivedLockFile lock_file("a/b/c/test"); - util::LockFileGuard lock(lock_file); - CHECK(lock.acquired()); + util::ShortLivedLockFile lock("a/b/c/test"); + CHECK(lock.acquire()); CHECK(Stat::lstat("a/b/c/test.lock")); } @@ -140,9 +141,8 @@ TEST_CASE("Break stale lock, blocking") util::set_timestamps("test.alive", long_time_ago); CHECK(symlink("foo", "test.lock") == 0); - util::LongLivedLockFile lock_file("test"); - util::LockFileGuard lock(lock_file); - CHECK(lock.acquired()); + util::LongLivedLockFile lock("test"); + CHECK(lock.acquire()); } TEST_CASE("Break stale lock, non-blocking") @@ -154,8 +154,8 @@ TEST_CASE("Break stale lock, non-blocking") util::set_timestamps("test.alive", long_time_ago); CHECK(symlink("foo", "test.lock") == 0); - util::LongLivedLockFile lock_file("test"); - util::LockFileGuard lock(lock_file, util::LockFileGuard::Mode::non_blocking); + util::LongLivedLockFile lock("test"); + CHECK(lock.try_acquire()); CHECK(lock.acquired()); } #endif // !_WIN32