From fc888bfcacb875c45bc8f9d7ca1357ab70a30490 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Tue, 13 Feb 2018 14:12:30 +0100 Subject: util: Fix multiple use of LockDirectory This commit fixes problems with calling LockDirectory multiple times on the same directory, or from multiple threads. It also fixes the build on OpenBSD. - Wrap the boost::interprocess::file_lock in a std::unique_ptr inside the map that keeps track of per-directory locks. This fixes a build issue with the clang 4.0.0+boost-1.58.0p8 version combo on OpenBSD 6.2, and should have no observable effect otherwise. - Protect the locks map using a mutex. - Make sure that only locks that are successfully acquired are inserted in the map. - Open the lock file for appending only if we know we don't have the lock yet - The `FILE* file = fsbridge::fopen(pathLockFile, "a");` wipes the 'we own this lock' administration, likely because it opens a new fd for the locked file then closes it. --- src/util.cpp | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) (limited to 'src/util.cpp') diff --git a/src/util.cpp b/src/util.cpp index 6738bbc6e..49f40dc94 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -375,18 +375,32 @@ int LogPrintStr(const std::string &str) bool LockDirectory(const fs::path& directory, const std::string lockfile_name, bool probe_only) { + // A map that contains all the currently held directory locks. After + // successful locking, these will be held here until the global + // destructor cleans them up and thus automatically unlocks them. + static std::map> locks; + // Protect the map with a mutex + static std::mutex cs; + std::lock_guard ulock(cs); fs::path pathLockFile = directory / lockfile_name; - FILE* file = fsbridge::fopen(pathLockFile, "a"); // empty lock file; created if it doesn't exist. + + // If a lock for this directory already exists in the map, don't try to re-lock it + if (locks.count(pathLockFile.string())) { + return true; + } + + // Create empty lock file if it doesn't exist. + FILE* file = fsbridge::fopen(pathLockFile, "a"); if (file) fclose(file); try { - static std::map locks; - boost::interprocess::file_lock& lock = locks.emplace(pathLockFile.string(), pathLockFile.string().c_str()).first->second; - if (!lock.try_lock()) { + auto lock = MakeUnique(pathLockFile.string().c_str()); + if (!lock->try_lock()) { return false; } - if (probe_only) { - lock.unlock(); + if (!probe_only) { + // Lock successful and we're not just probing, put it into the map + locks.emplace(pathLockFile.string(), std::move(lock)); } } catch (const boost::interprocess::interprocess_exception& e) { return error("Error while attempting to lock directory %s: %s", directory.string(), e.what()); -- cgit v1.2.3 From 1d4cbd26e4220982f7f2f60e447199d6f62ae254 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Tue, 13 Feb 2018 13:53:17 +0100 Subject: test: Add unit test for LockDirectory Add a unit test for LockDirectory, introduced in #11281. --- src/util.cpp | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) (limited to 'src/util.cpp') diff --git a/src/util.cpp b/src/util.cpp index 49f40dc94..dcf7ed38b 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -373,19 +373,22 @@ int LogPrintStr(const std::string &str) return ret; } +/** A map that contains all the currently held directory locks. After + * successful locking, these will be held here until the global destructor + * cleans them up and thus automatically unlocks them, or ReleaseDirectoryLocks + * is called. + */ +static std::map> dir_locks; +/** Mutex to protect dir_locks. */ +static std::mutex cs_dir_locks; + bool LockDirectory(const fs::path& directory, const std::string lockfile_name, bool probe_only) { - // A map that contains all the currently held directory locks. After - // successful locking, these will be held here until the global - // destructor cleans them up and thus automatically unlocks them. - static std::map> locks; - // Protect the map with a mutex - static std::mutex cs; - std::lock_guard ulock(cs); + std::lock_guard ulock(cs_dir_locks); fs::path pathLockFile = directory / lockfile_name; // If a lock for this directory already exists in the map, don't try to re-lock it - if (locks.count(pathLockFile.string())) { + if (dir_locks.count(pathLockFile.string())) { return true; } @@ -400,7 +403,7 @@ bool LockDirectory(const fs::path& directory, const std::string lockfile_name, b } if (!probe_only) { // Lock successful and we're not just probing, put it into the map - locks.emplace(pathLockFile.string(), std::move(lock)); + dir_locks.emplace(pathLockFile.string(), std::move(lock)); } } catch (const boost::interprocess::interprocess_exception& e) { return error("Error while attempting to lock directory %s: %s", directory.string(), e.what()); @@ -408,6 +411,12 @@ bool LockDirectory(const fs::path& directory, const std::string lockfile_name, b return true; } +void ReleaseDirectoryLocks() +{ + std::lock_guard ulock(cs_dir_locks); + dir_locks.clear(); +} + /** Interpret string as boolean, for argument parsing */ static bool InterpretBool(const std::string& strValue) { -- cgit v1.2.3