From 0760860c6d8a244f522dbe8a08fdaccf303d9916 Mon Sep 17 00:00:00 2001 From: Martchus Date: Thu, 25 Feb 2021 16:59:36 +0100 Subject: [PATCH] Log lock acquisitions/releases --- librepomgr/CMakeLists.txt | 3 + librepomgr/buildactions/buildaction.h | 24 ++--- librepomgr/buildactions/conductbuild.cpp | 4 +- librepomgr/buildactions/customcommand.cpp | 5 +- librepomgr/buildactions/reloaddatabase.cpp | 4 +- librepomgr/buildactions/repomanagement.cpp | 6 +- librepomgr/logcontext.h | 33 +++++++ librepomgr/logging.h | 8 +- librepomgr/namedlockable.cpp | 9 ++ librepomgr/namedlockable.h | 105 +++++++++++++++++++++ librepomgr/serversetup.cpp | 5 +- librepomgr/serversetup.h | 23 ++--- 12 files changed, 183 insertions(+), 46 deletions(-) create mode 100644 librepomgr/logcontext.h create mode 100644 librepomgr/namedlockable.cpp create mode 100644 librepomgr/namedlockable.h diff --git a/librepomgr/CMakeLists.txt b/librepomgr/CMakeLists.txt index 1321280..9a56efa 100644 --- a/librepomgr/CMakeLists.txt +++ b/librepomgr/CMakeLists.txt @@ -6,8 +6,10 @@ set(HEADER_FILES serversetup.h helper.h json.h + logcontext.h logging.h multisession.h + namedlockable.h authentication.h webapi/server.h webapi/session.h @@ -30,6 +32,7 @@ set(SRC_FILES json.cpp errorhandling.cpp serversetup.cpp + namedlockable.cpp authentication.cpp webapi/server.cpp webapi/session.cpp diff --git a/librepomgr/buildactions/buildaction.h b/librepomgr/buildactions/buildaction.h index b48fb54..38bcef6 100644 --- a/librepomgr/buildactions/buildaction.h +++ b/librepomgr/buildactions/buildaction.h @@ -7,6 +7,9 @@ #include "../webapi/routes.h" +#include "../logcontext.h" +#include "../namedlockable.h" + #include "../../libpkg/data/config.h" #include "../../libpkg/data/lockable.h" @@ -35,22 +38,6 @@ class BuildActionsTests; namespace LibRepoMgr { -struct LogContext { - explicit LogContext(BuildAction *buildAction = nullptr); - LogContext &operator=(const LogContext &) = delete; - template LogContext &operator()(CppUtilities::EscapeCodes::Phrases phrase, Args &&...args); - template LogContext &operator()(Args &&...args); - template LogContext &operator()(std::string &&msg); - -private: - BuildAction *const m_buildAction; -}; - -inline LogContext::LogContext(BuildAction *buildAction) - : m_buildAction(buildAction) -{ -} - struct ServiceSetup; namespace WebAPI { @@ -60,7 +47,7 @@ class Session; struct InternalBuildAction; -using AssociatedLocks = std::vector, std::unique_lock>>; +using AssociatedLocks = std::vector>; struct LIBREPOMGR_EXPORT PackageBuildData : public ReflectiveRapidJSON::JsonSerializable, public ReflectiveRapidJSON::BinarySerializable { @@ -362,3 +349,6 @@ struct LIBREPOMGR_EXPORT BuildActionBasicInfo : public ReflectiveRapidJSON::Json } // namespace LibRepoMgr #endif // LIBREPOMGR_BUILD_ACTION_H + +// avoid making LogContext available without also defining overloads for operator() which would possibly lead to linker errors +#include "../logging.h" diff --git a/librepomgr/buildactions/conductbuild.cpp b/librepomgr/buildactions/conductbuild.cpp index b1b9044..e2ec460 100644 --- a/librepomgr/buildactions/conductbuild.cpp +++ b/librepomgr/buildactions/conductbuild.cpp @@ -934,7 +934,7 @@ InvocationResult ConductBuild::invokeMakechrootpkg( // lock the chroot directory to prevent other build tasks from using it m_buildAction->log()(Phrases::InfoMessage, "Building ", packageName, '\n'); - auto chrootLock = m_setup.locks.acquireToWrite(buildRoot); + auto chrootLock = m_setup.locks.acquireToWrite(m_buildAction->log(), std::string(buildRoot)); // copy config files into chroot directory try { @@ -1117,7 +1117,7 @@ void ConductBuild::addPackageToRepo( auto processSession = m_buildAction->makeBuildProcess("repo-add for " + packageName, packageProgress.buildDirectory + "/repo-add.log", std::bind(&ConductBuild::handleRepoAddErrorsAndMakeNextPackage, this, makepkgchrootSession, std::ref(packageName), std::ref(packageProgress), std::placeholders::_1, std::placeholders::_2)); - processSession->locks().emplace_back(m_setup.locks.acquireToWrite( + processSession->locks().emplace_back(m_setup.locks.acquireToWrite(m_buildAction->log(), ServiceSetup::Locks::forDatabase(needsStaging ? m_buildPreparation.targetDb : m_buildPreparation.stagingDb, m_buildPreparation.targetArch))); processSession->launch(boost::process::start_dir(repoPath), m_repoAddPath, dbFilePath, binaryPackageNames); m_buildAction->log()(Phrases::InfoMessage, "Adding ", packageName, " to repo\n", ps(Phrases::SubMessage), "repo path: ", repoPath, '\n', diff --git a/librepomgr/buildactions/customcommand.cpp b/librepomgr/buildactions/customcommand.cpp index 05dedb2..1b3c958 100644 --- a/librepomgr/buildactions/customcommand.cpp +++ b/librepomgr/buildactions/customcommand.cpp @@ -80,15 +80,16 @@ void CustomCommand::run() const auto sharedLockNames = splitStringSimple>(findSetting(sharedLocksSetting), ","); const auto exclusiveLockNames = splitStringSimple>(findSetting(exclusiveLocksSetting), ","); auto &locks = process->locks(); + auto &log = m_buildAction->log(); locks.reserve(sharedLockNames.size() + exclusiveLockNames.size()); for (const auto &lockName : sharedLockNames) { if (!lockName.empty()) { - locks.emplace_back(m_setup.locks.acquireToRead(lockName)); + locks.emplace_back(m_setup.locks.acquireToRead(log, std::string(lockName))); } } for (const auto &lockName : exclusiveLockNames) { if (!lockName.empty()) { - locks.emplace_back(m_setup.locks.acquireToWrite(lockName)); + locks.emplace_back(m_setup.locks.acquireToWrite(log, std::string(lockName))); } } diff --git a/librepomgr/buildactions/reloaddatabase.cpp b/librepomgr/buildactions/reloaddatabase.cpp index 4c6b952..1a182b0 100644 --- a/librepomgr/buildactions/reloaddatabase.cpp +++ b/librepomgr/buildactions/reloaddatabase.cpp @@ -74,11 +74,11 @@ void ReloadDatabase::run() m_buildAction->appendOutput( Phrases::InfoMessage, "Loading database \"", dbName, '@', dbArch, "\" from local file \"", dbPath, "\"\n"); try { - auto dbFileLock = m_setup.locks.acquireToRead({ ServiceSetup::Locks::forDatabase(dbName, dbArch) }); + auto dbFileLock = m_setup.locks.acquireToRead(m_buildAction->log(), ServiceSetup::Locks::forDatabase(dbName, dbArch)); const auto lastModified = LibPkg::lastModified(dbPath); auto dbFile = LibPkg::extractFiles(dbPath, &LibPkg::Database::isFileRelevant); auto packages = LibPkg::Package::fromDatabaseFile(move(dbFile)); - dbFileLock.unlock(); + dbFileLock.lock().unlock(); const auto configLock = m_setup.config.lockToWrite(); auto *const db = m_setup.config.findDatabase(dbName, dbArch); if (!db) { diff --git a/librepomgr/buildactions/repomanagement.cpp b/librepomgr/buildactions/repomanagement.cpp index cafb346..e503098 100644 --- a/librepomgr/buildactions/repomanagement.cpp +++ b/librepomgr/buildactions/repomanagement.cpp @@ -156,7 +156,7 @@ void RemovePackages::run() // remove package from database file auto repoRemoveProcess = m_buildAction->makeBuildProcess("repo-remove", m_workingDirectory + "/repo-remove.log", std::bind(&RemovePackages::handleRepoRemoveResult, this, std::placeholders::_1, std::placeholders::_2)); - repoRemoveProcess->locks().emplace_back(m_setup.locks.acquireToWrite(m_destinationDatabaseLockName)); + repoRemoveProcess->locks().emplace_back(m_setup.locks.acquireToWrite(m_buildAction->log(), std::move(m_destinationDatabaseLockName))); repoRemoveProcess->launch( boost::process::start_dir(m_destinationRepoDirectory), m_repoRemovePath, m_destinationDatabaseFile, m_result.processedPackages); m_buildAction->log()(Phrases::InfoMessage, "Invoking repo-remove within \"", m_destinationRepoDirectory, "\" for \"", m_destinationDatabaseFile, @@ -273,13 +273,13 @@ void MovePackages::run() // add packages to database file of destination repo auto repoAddProcess = m_buildAction->makeBuildProcess("repo-add", m_workingDirectory + "/repo-add.log", std::bind(&MovePackages::handleRepoAddResult, this, processSession, std::placeholders::_1, std::placeholders::_2)); - repoAddProcess->locks().emplace_back(m_setup.locks.acquireToWrite(m_destinationDatabaseLockName)); + repoAddProcess->locks().emplace_back(m_setup.locks.acquireToWrite(m_buildAction->log(), std::move(m_destinationDatabaseLockName))); repoAddProcess->launch(boost::process::start_dir(m_destinationRepoDirectory), m_repoAddPath, m_destinationDatabaseFile, m_fileNames); // remove package from database file of source repo auto repoRemoveProcess = m_buildAction->makeBuildProcess("repo-remove", m_workingDirectory + "/repo-remove.log", std::bind(&MovePackages::handleRepoRemoveResult, this, processSession, std::placeholders::_1, std::placeholders::_2)); - repoRemoveProcess->locks().emplace_back(m_setup.locks.acquireToWrite(m_sourceDatabaseLockName)); + repoRemoveProcess->locks().emplace_back(m_setup.locks.acquireToWrite(m_buildAction->log(), std::move(m_sourceDatabaseLockName))); repoRemoveProcess->launch(boost::process::start_dir(m_sourceRepoDirectory), m_repoRemovePath, m_sourceDatabaseFile, m_result.processedPackages); m_buildAction->log()(ps(Phrases::InfoMessage), "Invoking repo-add within \"", m_destinationRepoDirectory, "\" for \"", m_destinationDatabaseFile, diff --git a/librepomgr/logcontext.h b/librepomgr/logcontext.h new file mode 100644 index 0000000..3cdfe6a --- /dev/null +++ b/librepomgr/logcontext.h @@ -0,0 +1,33 @@ +#ifndef LIBREPOMGR_LOGCONTEXT_H +#define LIBREPOMGR_LOGCONTEXT_H + +// Do NOT include this header directly, include "loggin.h" instead. This header only exists to resolve the +// cyclic dependency between LogContext and BuildAction but lacks definitions of operator(). + +#include "./global.h" + +#include + +namespace LibRepoMgr { + +struct BuildAction; + +struct LIBREPOMGR_EXPORT LogContext { + explicit LogContext(BuildAction *buildAction = nullptr); + LogContext &operator=(const LogContext &) = delete; + template LogContext &operator()(CppUtilities::EscapeCodes::Phrases phrase, Args &&...args); + template LogContext &operator()(Args &&...args); + template LogContext &operator()(std::string &&msg); + +private: + BuildAction *const m_buildAction; +}; + +inline LogContext::LogContext(BuildAction *buildAction) + : m_buildAction(buildAction) +{ +} + +} // namespace LibRepoMgr + +#endif // LIBREPOMGR_LOGCONTEXT_H diff --git a/librepomgr/logging.h b/librepomgr/logging.h index 6738f5c..a1b1598 100644 --- a/librepomgr/logging.h +++ b/librepomgr/logging.h @@ -1,6 +1,8 @@ #ifndef LIBREPOMGR_LOGGING_H #define LIBREPOMGR_LOGGING_H +#include "./logcontext.h" + #include "./buildactions/buildaction.h" namespace LibRepoMgr { @@ -10,7 +12,7 @@ inline auto ps(CppUtilities::EscapeCodes::Phrases phrase) return CppUtilities::EscapeCodes::formattedPhraseString(phrase); } -template LogContext &LogContext::operator()(std::string &&msg) +template LIBREPOMGR_EXPORT LogContext &LogContext::operator()(std::string &&msg) { std::cerr << msg; if (m_buildAction) { @@ -19,12 +21,12 @@ template LogContext &LogContext::operator()(std::string &&msg return *this; } -template LogContext &LogContext::operator()(CppUtilities::EscapeCodes::Phrases phrase, Args &&...args) +template inline LogContext &LogContext::operator()(CppUtilities::EscapeCodes::Phrases phrase, Args &&...args) { return (*this)(CppUtilities::argsToString(CppUtilities::EscapeCodes::formattedPhraseString(phrase), std::forward(args)...)); } -template LogContext &LogContext::operator()(Args &&...args) +template inline LogContext &LogContext::operator()(Args &&...args) { return (*this)(CppUtilities::argsToString( CppUtilities::EscapeCodes::formattedPhraseString(CppUtilities::EscapeCodes::Phrases::InfoMessage), std::forward(args)...)); diff --git a/librepomgr/namedlockable.cpp b/librepomgr/namedlockable.cpp new file mode 100644 index 0000000..fc25d37 --- /dev/null +++ b/librepomgr/namedlockable.cpp @@ -0,0 +1,9 @@ +#include "./namedlockable.h" +#include "./logging.h" + +namespace LibRepoMgr { + +template struct NamedLock>; +template struct NamedLock>; + +} // namespace LibRepoMgr diff --git a/librepomgr/namedlockable.h b/librepomgr/namedlockable.h new file mode 100644 index 0000000..b101fe2 --- /dev/null +++ b/librepomgr/namedlockable.h @@ -0,0 +1,105 @@ +#ifndef LIBREPOMGR_NAMED_LOCKABLE_H +#define LIBREPOMGR_NAMED_LOCKABLE_H + +#include +#include + +#include "./global.h" + +namespace LibRepoMgr { + +struct LogContext; + +template struct NamedLock { + template NamedLock(LogContext &log, std::string &&name, Args &&...args); + NamedLock(NamedLock &&) = default; + ~NamedLock(); + + const std::string &name() const + { + return m_name; + }; + UnderlyingLockType &lock() + { + return m_lock; + }; + +private: + LogContext &m_log; + std::string m_name; + UnderlyingLockType m_lock; +}; + +constexpr std::string_view lockName(std::shared_lock &) +{ + return "shared"; +} +constexpr std::string_view lockName(std::unique_lock &) +{ + return "exclusive"; +} + +template +template +inline NamedLock::NamedLock(LogContext &log, std::string &&name, Args &&...args) + : m_log(log) + , m_name(std::move(name)) +{ + m_log("Acquiring ", lockName(m_lock), " lock \"", m_name, "\"\n"); + m_lock = UnderlyingLockType(std::forward(args)...); +} + +template inline NamedLock::~NamedLock() +{ + if (m_lock) { + m_lock.unlock(); + m_log("Released ", lockName(m_lock), " lock \"", m_name, "\"\n"); + } +} + +using SharedNamedLock = NamedLock>; +using UniqueNamedLock = NamedLock>; + +extern template struct LIBREPOMGR_EXPORT NamedLock>; +extern template struct LIBREPOMGR_EXPORT NamedLock>; + +struct NamedLockable { + [[nodiscard]] SharedNamedLock lockToRead(LogContext &log, std::string &&name) const; + [[nodiscard]] UniqueNamedLock lockToWrite(LogContext &log, std::string &&name); + [[nodiscard]] SharedNamedLock tryLockToRead(LogContext &log, std::string &&name) const; + [[nodiscard]] UniqueNamedLock tryLockToWrite(LogContext &log, std::string &&name); + [[nodiscard]] UniqueNamedLock lockToWrite(LogContext &log, std::string &&name, SharedNamedLock &readLock); + +private: + mutable std::shared_mutex m_mutex; +}; + +inline SharedNamedLock NamedLockable::lockToRead(LogContext &log, std::string &&name) const +{ + return SharedNamedLock(log, std::move(name), m_mutex); +} + +inline UniqueNamedLock NamedLockable::lockToWrite(LogContext &log, std::string &&name) +{ + return UniqueNamedLock(log, std::move(name), m_mutex); +} + +inline SharedNamedLock NamedLockable::tryLockToRead(LogContext &log, std::string &&name) const +{ + return SharedNamedLock(log, std::move(name), m_mutex, std::try_to_lock); +} + +inline UniqueNamedLock NamedLockable::tryLockToWrite(LogContext &log, std::string &&name) +{ + return UniqueNamedLock(log, std::move(name), m_mutex, std::try_to_lock); +} + +inline UniqueNamedLock NamedLockable::lockToWrite(LogContext &log, std::string &&name, SharedNamedLock &readLock) +{ + readLock.lock().unlock(); + return UniqueNamedLock(log, std::move(name), m_mutex); +} + +} // namespace LibRepoMgr + +#endif // LIBREPOMGR_NAMED_LOCKABLE_H diff --git a/librepomgr/serversetup.cpp b/librepomgr/serversetup.cpp index 26df2a0..e3dddb0 100644 --- a/librepomgr/serversetup.cpp +++ b/librepomgr/serversetup.cpp @@ -654,10 +654,11 @@ void ServiceSetup::run() void ServiceSetup::Locks::clear() { + auto log = LogContext(); const auto lock = std::lock_guard(m_mutex); for (auto i = m_locksByName.begin(), end = m_locksByName.end(); i != end; ++i) { - if (auto lock2 = i->second.tryLockToWrite()) { // check whether nobody holds the lock anymore - lock2.unlock(); // ~shared_mutex(): The behavior is undefined if the mutex is owned by any thread [...]. + if (auto lock2 = i->second.tryLockToWrite(log, std::string(i->first)); lock2.lock()) { // check whether nobody holds the lock anymore + lock2.lock().unlock(); // ~shared_mutex(): The behavior is undefined if the mutex is owned by any thread [...]. m_locksByName.erase(i); // we can be sure no other thead aquires i->second in the meantime because we're holding m_mutex } } diff --git a/librepomgr/serversetup.h b/librepomgr/serversetup.h index 426de05..bb21e57 100644 --- a/librepomgr/serversetup.h +++ b/librepomgr/serversetup.h @@ -4,6 +4,7 @@ #include "./authentication.h" #include "./buildactions/buildaction.h" #include "./buildactions/buildactiontemplate.h" +#include "./namedlockable.h" #include "../libpkg/data/config.h" #include "../libpkg/data/lockable.h" @@ -121,16 +122,15 @@ struct LIBREPOMGR_EXPORT ServiceSetup : public LibPkg::Lockable { } auth; struct LIBREPOMGR_EXPORT Locks { - [[nodiscard]] std::shared_lock acquireToRead(const std::string &lockName); - [[nodiscard]] std::unique_lock acquireToWrite(const std::string &lockName); - [[nodiscard]] std::unique_lock acquireToWrite(std::shared_lock &readLock, const std::string &lockName); + [[nodiscard]] SharedNamedLock acquireToRead(LogContext &log, std::string &&lockName); + [[nodiscard]] UniqueNamedLock acquireToWrite(LogContext &log, std::string &&lockName); void clear(); static std::string forDatabase(std::string_view dbName, std::string_view dbArch); static std::string forDatabase(const LibPkg::Database &db); private: std::mutex m_mutex; - std::unordered_map m_locksByName; + std::unordered_map m_locksByName; } locks; void loadConfigFiles(bool restoreStateAndDiscardDatabases); @@ -149,23 +149,16 @@ inline std::shared_ptr ServiceSetup::BuildSetup::getBuildAction(Bui return id < actions.size() ? actions[id] : nullptr; } -inline std::shared_lock ServiceSetup::Locks::acquireToRead(const std::string &lockName) +inline SharedNamedLock ServiceSetup::Locks::acquireToRead(LogContext &log, std::string &&lockName) { const auto lock = std::lock_guard(m_mutex); - return m_locksByName[lockName].lockToRead(); + return m_locksByName[lockName].lockToRead(log, std::move(lockName)); } -inline std::unique_lock ServiceSetup::Locks::acquireToWrite(const std::string &lockName) +inline UniqueNamedLock ServiceSetup::Locks::acquireToWrite(LogContext &log, std::string &&lockName) { const auto lock = std::lock_guard(m_mutex); - return m_locksByName[lockName].lockToWrite(); -} - -inline std::unique_lock ServiceSetup::Locks::acquireToWrite( - std::shared_lock &readLock, const std::string &lockName) -{ - readLock.unlock(); - return acquireToWrite(lockName); + return m_locksByName[lockName].lockToWrite(log, std::move(lockName)); } struct LIBREPOMGR_EXPORT ServiceStatus : public ReflectiveRapidJSON::JsonSerializable {