From 4f413e86bae33b3189d98b2dde308e4e8918e2ed Mon Sep 17 00:00:00 2001 From: Martchus Date: Fri, 30 Dec 2022 21:12:49 +0100 Subject: [PATCH] Allow to abort build action immediately while waiting for chroot locks * Avoid aborted build actions from being stuck in running state while waiting until the chroot locks can be acquired; allow aborting action immediately instead * Use async locks when invoking makechrootpkg and split affected function accordingly --- librepomgr/buildactions/buildaction.cpp | 49 +++++++++++++++++++- librepomgr/buildactions/buildaction.h | 2 + librepomgr/buildactions/buildactionprivate.h | 8 ++++ librepomgr/buildactions/conductbuild.cpp | 41 ++++++++++++++-- 4 files changed, 95 insertions(+), 5 deletions(-) diff --git a/librepomgr/buildactions/buildaction.cpp b/librepomgr/buildactions/buildaction.cpp index e490255..07712c5 100644 --- a/librepomgr/buildactions/buildaction.cpp +++ b/librepomgr/buildactions/buildaction.cpp @@ -345,9 +345,56 @@ void BuildAction::assignStartAfter(const std::vectorbuilding.ioContext.get_executor(), m_stopHandler); } + if (m_waitingOnAsyncLock) { + const auto buildActionLock = m_setup->building.lockToWrite(); + if (isExecuting()) { + conclude(BuildActionResult::Aborted); + } + } +} + +void LibRepoMgr::BuildAction::acquireToWrite(std::string &&lockName, std::move_only_function &&callback) +{ + // flag this action as "waiting for async lock" so the abort() function is allowed to conclude the action right away (and thus the + // build action is not stuck in "running" until the lock is acquired) + m_waitingOnAsyncLock.store(true); + + // handle abortion if aborted (instead of acquiring the lock) + if (m_aborted) { + auto buildActionLock = m_setup->building.lockToWrite(); + if (isExecuting()) { + conclude(BuildActionResult::Aborted); + } + return; + } + + // acquire the lock asynchronously + m_setup->locks.acquireToWrite( + log(), std::move(lockName), [t = shared_from_this(), callback = std::move(callback)](UniqueLoggingLock &&lock) mutable { + // stop the abort() function from immediately concluding the build action again + t->m_waitingOnAsyncLock.store(false); + + // execute the callback in another building thread to avoid interferances with the thread that invoked this callback (when releasing its own lock) + boost::asio::post(t->m_setup->building.ioContext.get_executor(), [t = t, callback = std::move(callback), lock = std::move(lock)] mutable { + // conclude the action as aborted if it has been aborted meanwhile + auto buildActionLock = t->m_setup->building.lockToWrite(); + if (t->m_aborted && t->isExecuting()) { + t->conclude(BuildActionResult::Aborted); + } + + // execute the callback only if the action hasn't been aborted + if (!t->m_aborted) { + buildActionLock.unlock(); + callback(std::move(lock)); + } + }); + }); } template void BuildAction::post() diff --git a/librepomgr/buildactions/buildaction.h b/librepomgr/buildactions/buildaction.h index a9bbf3c..f13f7f9 100644 --- a/librepomgr/buildactions/buildaction.h +++ b/librepomgr/buildactions/buildaction.h @@ -215,6 +215,7 @@ public: LibPkg::StorageID start(ServiceSetup &setup, std::unique_ptr &&secrets); void assignStartAfter(const std::vector> &startsAfterBuildActions); void abort(); + void acquireToWrite(std::string &&lockName, std::move_only_function &&callback); void appendOutput(std::string_view output); void appendOutput(std::string &&output); template void appendOutput(Args &&...args); @@ -254,6 +255,7 @@ private: LogContext m_log; ServiceSetup *m_setup = nullptr; std::atomic_bool m_aborted = false; + std::atomic_bool m_waitingOnAsyncLock = false; std::function m_stopHandler; std::function m_concludeHandler; std::mutex m_processesMutex; diff --git a/librepomgr/buildactions/buildactionprivate.h b/librepomgr/buildactions/buildactionprivate.h index 1a1d730..03526cf 100644 --- a/librepomgr/buildactions/buildactionprivate.h +++ b/librepomgr/buildactions/buildactionprivate.h @@ -596,6 +596,14 @@ private: const std::string &packageName, PackageBuildProgress &packageProgress, const std::string &buildDirectory); void invokeMakechrootpkg(const BatchProcessingSession::SharedPointerType &makepkgchrootSession, const std::string &packageName, bool hasFailuresInPreviousBatches, std::move_only_function &&cb); + void invokeMakechrootpkgStep2(const BatchProcessingSession::SharedPointerType &makepkgchrootSession, const std::string &packageName, + UniqueLoggingLock &&chrootLock, const std::string &chrootDir, const std::string &buildRoot, + const std::vector &makechrootpkgFlags, const std::vector &makepkgFlags, const std::vector &sudoArgs, + std::move_only_function &&cb); + void invokeMakechrootpkgStep3(std::shared_ptr &processSession, const std::string &packageName, + UniqueLoggingLock &&chrootLock, UniqueLoggingLock &&chrootUserLock, const std::string &chrootDir, + const std::vector &makechrootpkgFlags, const std::vector &makepkgFlags, const std::vector &sudoArgs, + std::move_only_function &&cb); void invokeMakecontainerpkg(const BatchProcessingSession::SharedPointerType &makepkgchrootSession, const std::string &packageName, PackageBuildProgress &packageProgress, const std::vector &makepkgFlags, std::shared_lock &&lock, std::move_only_function &&cb); diff --git a/librepomgr/buildactions/conductbuild.cpp b/librepomgr/buildactions/conductbuild.cpp index 270b1ca..fa86a1a 100644 --- a/librepomgr/buildactions/conductbuild.cpp +++ b/librepomgr/buildactions/conductbuild.cpp @@ -1063,9 +1063,21 @@ void ConductBuild::invokeMakechrootpkg(const BatchProcessingSession::SharedPoint } // lock the chroot directory to prevent other build tasks from using it + auto lockName = std::string(buildRoot); m_buildAction->log()(Phrases::InfoMessage, "Building ", packageName, '\n'); - auto chrootLock = m_setup.locks.acquireToWrite(m_buildAction->log(), std::string(buildRoot)); + m_buildAction->acquireToWrite(std::move(lockName), + [this, makepkgchrootSession, &packageName, chrootDir = std::move(chrootDir), buildRoot = std::move(buildRoot), + makechrootpkgFlags = std::move(makechrootpkgFlags), makepkgFlags = std::move(makepkgFlags), sudoArgs = std::move(sudoArgs), + cb = std::move(cb)](UniqueLoggingLock &&chrootLock) mutable { + invokeMakechrootpkgStep2(makepkgchrootSession, packageName, std::move(chrootLock), chrootDir, buildRoot, makechrootpkgFlags, makepkgFlags, + sudoArgs, std::move(cb)); + }); +} +void ConductBuild::invokeMakechrootpkgStep2(const BatchProcessingSession::SharedPointerType &makepkgchrootSession, const std::string &packageName, + UniqueLoggingLock &&chrootLock, const std::string &chrootDir, const std::string &buildRoot, const std::vector &makechrootpkgFlags, + const std::vector &makepkgFlags, const std::vector &sudoArgs, std::move_only_function &&cb) +{ // copy config files into chroot directory try { std::filesystem::copy_file(makepkgchrootSession->isStagingEnabled() ? m_pacmanStagingConfigPath : m_pacmanConfigPath, @@ -1073,13 +1085,15 @@ void ConductBuild::invokeMakechrootpkg(const BatchProcessingSession::SharedPoint std::filesystem::copy_file(m_makepkgConfigPath, buildRoot + "/etc/makepkg.conf", std::filesystem::copy_options::overwrite_existing); } catch (const std::filesystem::filesystem_error &e) { chrootLock.lock().unlock(); - auto writeLock = lockToWrite(lock); - packageProgress.error = "Unable to configure chroot \"" % buildRoot % "\": " + e.what(); + auto writeLock = lockToWrite(); + m_buildProgress.progressByPackage[packageName].error = "Unable to configure chroot \"" % buildRoot % "\": " + e.what(); writeLock.unlock(); return cb(InvocationResult::Error); } // prepare process session (after configuring chroot so we don't get stuck if configuring chroot fails) + auto lock = lockToRead(); + auto &packageProgress = m_buildProgress.progressByPackage[packageName]; auto processSession = m_buildAction->makeBuildProcess(packageName + " build", packageProgress.buildDirectory + "/build.log", std::bind(&ConductBuild::handleMakechrootpkgErrorsAndAddPackageToRepo, this, makepkgchrootSession, std::ref(packageName), std::ref(packageProgress), std::placeholders::_1, std::placeholders::_2)); @@ -1088,6 +1102,7 @@ void ConductBuild::invokeMakechrootpkg(const BatchProcessingSession::SharedPoint &ConductBuild::assignNewVersion, this, std::ref(packageName), std::ref(packageProgress), std::placeholders::_1, std::placeholders::_2))); processSession->registerNewDataHandler(BufferSearch("Synchronizing chroot copy", "\n", std::string_view(), [processSession = processSession.get()](BufferSearch &, std::string &&) { processSession->locks().pop_back(); })); + lock.unlock(); // invoke makechrootpkg to build package m_buildAction->log()(Phrases::InfoMessage, "Invoking makechrootpkg for ", packageName, " via ", m_makeChrootPkgPath.string(), '\n', @@ -1095,8 +1110,26 @@ void ConductBuild::invokeMakechrootpkg(const BatchProcessingSession::SharedPoint ps(Phrases::SubMessage), "chroot user: ", packageProgress.chrootUser, '\n'); auto &locks = processSession->locks(); locks.reserve(2); - locks.emplace_back(m_setup.locks.acquireToWrite(m_buildAction->log(), chrootDir % '/' + packageProgress.chrootUser)); + m_buildAction->acquireToWrite(chrootDir % '/' + packageProgress.chrootUser, + [this, processSession, &packageName, chrootLock = std::move(chrootLock), chrootDir = std::move(chrootDir), + makechrootpkgFlags = std::move(makechrootpkgFlags), makepkgFlags = std::move(makepkgFlags), sudoArgs = std::move(sudoArgs), + cb = std::move(cb)](UniqueLoggingLock &&chrootUserLock) mutable { + invokeMakechrootpkgStep3(processSession, packageName, std::move(chrootLock), std::move(chrootUserLock), chrootDir, makechrootpkgFlags, + makepkgFlags, sudoArgs, std::move(cb)); + }); +} + +void ConductBuild::invokeMakechrootpkgStep3(std::shared_ptr &processSession, const std::string &packageName, + UniqueLoggingLock &&chrootLock, UniqueLoggingLock &&chrootUserLock, const std::string &chrootDir, + const std::vector &makechrootpkgFlags, const std::vector &makepkgFlags, const std::vector &sudoArgs, + std::move_only_function &&cb) +{ + // invoke makechrootpkg to build package + auto &locks = processSession->locks(); + locks.emplace_back(std::move(chrootUserLock)); locks.emplace_back(std::move(chrootLock)); + auto lock = lockToRead(); + auto &packageProgress = m_buildProgress.progressByPackage[packageName]; processSession->launch(boost::process::start_dir(packageProgress.buildDirectory), m_makeChrootPkgPath, sudoArgs, makechrootpkgFlags, "-C", m_globalPackageCacheDir, "-r", chrootDir, "-l", packageProgress.chrootUser, packageProgress.makechrootpkgFlags, "--", makepkgFlags, packageProgress.makepkgFlags, boost::process::std_in < boost::asio::buffer(m_sudoPassword));