diff --git a/lix/legacy/nix-env.cc b/lix/legacy/nix-env.cc index 7ac40fdce..d6b800569 100644 --- a/lix/legacy/nix-env.cc +++ b/lix/legacy/nix-env.cc @@ -1359,8 +1359,7 @@ static void opListGenerations(Globals & globals, Strings opFlags, Strings opArgs if (opArgs.size() != 0) throw UsageError("no arguments expected"); - PathLocks lock; - lockProfile(lock, globals.profile); + PathLock lock = lockProfile(globals.profile); auto [gens, curGen] = findGenerations(globals.profile); diff --git a/lix/legacy/user-env.cc b/lix/legacy/user-env.cc index fb94bbb1b..88cfec6fe 100644 --- a/lix/legacy/user-env.cc +++ b/lix/legacy/user-env.cc @@ -133,8 +133,7 @@ bool createUserEnv(EvalState & state, DrvInfos & elems, auto store2 = state.ctx.store.dynamic_pointer_cast(); if (store2) { - PathLocks lock; - lockProfile(lock, profile); + PathLock lock = lockProfile(profile); Path lockTokenCur = optimisticLockProfile(profile); if (lockToken != lockTokenCur) { diff --git a/lix/libfetchers/git.cc b/lix/libfetchers/git.cc index 7d352b65c..80ad1628c 100644 --- a/lix/libfetchers/git.cc +++ b/lix/libfetchers/git.cc @@ -558,7 +558,7 @@ struct GitInputScheme : InputScheme gitDir = "."; createDirs(dirOf(cacheDir)); - PathLocks cacheDirLock({cacheDir + ".lock"}); + PathLock cacheDirLock = lockPath(cacheDir + ".lock"); if (!pathExists(cacheDir)) { runProgram("git", true, { "-c", "init.defaultBranch=" + gitInitialBranch, "init", "--bare", repoDir }); diff --git a/lix/libstore/build/derivation-goal.cc b/lix/libstore/build/derivation-goal.cc index 2f90df95b..e2980a18e 100644 --- a/lix/libstore/build/derivation-goal.cc +++ b/lix/libstore/build/derivation-goal.cc @@ -734,7 +734,8 @@ retry: } } - if (!outputLocks.tryLockPaths(lockFiles)) { + outputLocks = tryLockPaths(lockFiles); + if (!outputLocks) { if (!actLock) actLock = std::make_unique(*logger, lvlWarn, actBuildWaiting, fmt("waiting for lock on %s", Magenta(showPaths(lockFiles)))); @@ -807,7 +808,7 @@ retry: if (!actLock) actLock = std::make_unique(*logger, lvlTalkative, actBuildWaiting, fmt("waiting for a machine to build '%s'", Magenta(worker.store.printStorePath(drvPath)))); - outputLocks.unlock(); + outputLocks.reset(); co_await waitForAWhile(); goto retry; } @@ -1086,11 +1087,11 @@ try { lockers will see that the output paths are valid; they will not create new lock files with the same names as the old (unlinked) lock files. */ - outputLocks.unlock(); + outputLocks.reset(); co_return done(BuildResult::Built, std::move(builtOutputs)); } catch (BuildError & e) { - outputLocks.unlock(); + outputLocks.reset(); BuildResult::Status st = BuildResult::MiscFailure; @@ -1712,7 +1713,7 @@ Goal::WorkResult DerivationGoal::done( { isDone = true; - outputLocks.unlock(); + outputLocks.reset(); buildResult.status = status; if (ex) buildResult.errorMsg = fmt("%s", Uncolored(ex->info().msg)); diff --git a/lix/libstore/build/derivation-goal.hh b/lix/libstore/build/derivation-goal.hh index c8a657bde..2a547ecb3 100644 --- a/lix/libstore/build/derivation-goal.hh +++ b/lix/libstore/build/derivation-goal.hh @@ -174,7 +174,7 @@ struct DerivationGoal : public Goal /** * Locks on (fixed) output paths. */ - PathLocks outputLocks; + std::optional outputLocks; /** * All input paths (that is, the union of FS closures of the diff --git a/lix/libstore/build/local-derivation-goal.cc b/lix/libstore/build/local-derivation-goal.cc index 554edfc5b..960608041 100644 --- a/lix/libstore/build/local-derivation-goal.cc +++ b/lix/libstore/build/local-derivation-goal.cc @@ -160,7 +160,7 @@ retry: #endif if (!slotToken.valid()) { - outputLocks.unlock(); + outputLocks.reset(); if (worker.localBuilds.capacity() > 0) { slotToken = co_await worker.localBuilds.acquire(); co_return co_await tryToBuild(); @@ -262,7 +262,7 @@ retry: } } catch (BuildError & e) { - outputLocks.unlock(); + outputLocks.reset(); buildUser.reset(); auto report = done(BuildResult::InputRejected, {}, std::move(e)); report.permanentFailure = true; @@ -2295,13 +2295,13 @@ try { /* Lock final output path, if not already locked. This happens with floating CA derivations and hash-mismatching fixed-output derivations. */ - PathLocks dynamicOutputLock; + std::optional dynamicOutputLock; auto optFixedPath = output->path(worker.store, drv->name, outputName); if (!optFixedPath || worker.store.printStorePath(*optFixedPath) != finalDestPath) { assert(newInfo.ca); - dynamicOutputLock.lockPaths({worker.store.toRealPath(finalDestPath)}); + dynamicOutputLock = lockPath(worker.store.toRealPath(finalDestPath)); } /* Move files, if needed */ diff --git a/lix/libstore/local-store.cc b/lix/libstore/local-store.cc index 92c4c6e07..53d497af1 100644 --- a/lix/libstore/local-store.cc +++ b/lix/libstore/local-store.cc @@ -1216,7 +1216,7 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source, if (repair || !isValidPath(info.path)) { - PathLocks outputLock; + std::optional outputLock; auto realPath = Store::toRealPath(info.path); @@ -1224,7 +1224,7 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source, from a build hook (whose parent process already acquired a lock on this path). */ if (!locksHeld.count(printStorePath(info.path))) - outputLock.lockPaths({realPath}); + outputLock = lockPath(realPath); if (repair || !isValidPath(info.path)) { @@ -1370,7 +1370,7 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name auto realPath = Store::toRealPath(dstPath); - PathLocks outputLock({realPath}); + PathLock outputLock = lockPath(realPath); if (repair || !isValidPath(dstPath)) { @@ -1435,7 +1435,7 @@ StorePath LocalStore::addTextToStore( auto realPath = Store::toRealPath(dstPath); - PathLocks outputLock({realPath}); + PathLock outputLock = lockPath(realPath); if (repair || !isValidPath(dstPath)) { diff --git a/lix/libstore/pathlocks.cc b/lix/libstore/pathlocks.cc index 28e995d5b..b0f991235 100644 --- a/lix/libstore/pathlocks.cc +++ b/lix/libstore/pathlocks.cc @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -131,80 +132,89 @@ void unlockFile(int fd) } -PathLocks::PathLocks() +std::optional +PathLock::lockImpl(const Path & path, std::string_view waitMsg, bool wait) { + Path lockPath = path + ".lock"; + + debug("locking path '%1%'", path); + + while (1) { + + /* Open/create the lock file. */ + auto fd = openLockFile(lockPath, true); + + /* Acquire an exclusive lock. */ + if (!tryLockFile(fd.get(), ltWrite)) { + if (wait) { + if (waitMsg != "") printError(std::string(waitMsg)); + lockFile(fd.get(), ltWrite); + } else { + return std::nullopt; + } + } + + debug("lock acquired on '%1%'", lockPath); + + /* Check that the lock file hasn't become stale (i.e., + hasn't been unlinked). */ + struct stat st; + if (fstat(fd.get(), &st) == -1) + throw SysError("statting lock file '%1%'", lockPath); + if (st.st_nlink == 0) + /* This lock file has been unlinked, so we're holding + a lock on a deleted file. This means that other + processes may create and acquire a lock on + `lockPath', and proceed. So we must retry. */ + debug("open lock file '%1%' has become stale", lockPath); + else + return PathLock{std::move(fd), lockPath}; + } } - -PathLocks::PathLocks(const PathSet & paths, const std::string & waitMsg) +PathLock lockPath(const Path & path, std::string_view waitMsg) { - lockPaths(paths, waitMsg); + return std::move(*PathLock::lockImpl(path, waitMsg, true)); } - -bool PathLocks::lockPathsImpl(const PathSet & paths, - const std::string & waitMsg, bool wait) +std::optional tryLockPath(const Path & path) { - assert(fds.empty()); + return PathLock::lockImpl(path, "", false); +} - /* Note that `fds' is built incrementally so that the destructor - will only release those locks that we have already acquired. */ +static std::optional +lockPathsImpl(const PathSet & paths, std::string_view waitMsg, bool wait) +{ + PathLocks result; /* Acquire the lock for each path in sorted order. This ensures that locks are always acquired in the same order, thus preventing deadlocks. */ for (auto & path : paths) { - checkInterrupt(); - Path lockPath = path + ".lock"; - - debug("locking path '%1%'", path); - - AutoCloseFD fd; - - while (1) { - - /* Open/create the lock file. */ - fd = openLockFile(lockPath, true); - - /* Acquire an exclusive lock. */ - if (!tryLockFile(fd.get(), ltWrite)) { - if (wait) { - if (waitMsg != "") printError(waitMsg); - lockFile(fd.get(), ltWrite); - } else { - /* Failed to lock this path; release all other - locks. */ - unlock(); - return false; - } - } - - debug("lock acquired on '%1%'", lockPath); - - /* Check that the lock file hasn't become stale (i.e., - hasn't been unlinked). */ - struct stat st; - if (fstat(fd.get(), &st) == -1) - throw SysError("statting lock file '%1%'", lockPath); - if (st.st_nlink == 0) - /* This lock file has been unlinked, so we're holding - a lock on a deleted file. This means that other - processes may create and acquire a lock on - `lockPath', and proceed. So we must retry. */ - debug("open lock file '%1%' has become stale", lockPath); - else - break; + if (wait) { + result.push_back(lockPath(path, waitMsg)); + } else if (auto p = tryLockPath(path); p) { + result.push_back(std::move(*p)); + } else { + return std::nullopt; } - - /* Use borrow so that the descriptor isn't closed. */ - fds.push_back(FDPair(fd.release(), lockPath)); } - return true; + return result; +} + +PathLocks lockPaths(const PathSet & paths, std::string_view waitMsg) +{ + return std::move(*lockPathsImpl(paths, waitMsg, true)); +} + +std::optional tryLockPaths(const PathSet & paths) +{ + return lockPathsImpl(paths, "", false); } -PathLocks::~PathLocks() +PathLock::~PathLock() { try { unlock(); @@ -214,25 +224,23 @@ PathLocks::~PathLocks() } -void PathLocks::unlock() +void PathLock::unlock() { - for (auto & i : fds) { + if (fd) { // delete the file. if another file descriptor is used to acquire a lock on // this file it will figure out that the file is stale once it calls stat() // and inspects the link count. if unlink fails we merely leave around some // stale lock file paths that can be reused or cleaned up by other threads. - unlink(i.second.c_str()); + unlink(path.c_str()); // clobber file contents for compatibility wither other nix implementations - writeFull(i.first, "d"); + writeFull(fd.get(), "d"); - if (close(i.first) == -1) { - printError("error (ignored): cannot close lock file on '%1%'", i.second); + if (close(fd.release()) == -1) { + printError("error (ignored): cannot close lock file on '%1%'", path); } - debug("lock released on '%1%'", i.second); + debug("lock released on '%1%'", path); } - - fds.clear(); } diff --git a/lix/libstore/pathlocks.hh b/lix/libstore/pathlocks.hh index 0220858d8..a3d6cf9e1 100644 --- a/lix/libstore/pathlocks.hh +++ b/lix/libstore/pathlocks.hh @@ -31,29 +31,35 @@ bool unsafeLockFileSingleThreaded(int fd, LockType lockType, std::chrono::second bool tryLockFile(int fd, LockType lockType); void unlockFile(int fd); -class PathLocks +class PathLock { -private: - typedef std::pair FDPair; - std::list fds; + friend PathLock lockPath(const Path & path, std::string_view waitMsg); + friend std::optional tryLockPath(const Path & path); - bool lockPathsImpl(const PathSet & _paths, const std::string & waitMsg, bool wait); + AutoCloseFD fd; + Path path; + + PathLock(AutoCloseFD fd, const Path & path): fd(std::move(fd)), path(path) {} + + static std::optional + lockImpl(const Path & path, std::string_view waitMsg, bool wait); public: - PathLocks(); - PathLocks(const PathSet & paths, const std::string & waitMsg = ""); - void lockPaths(const PathSet & _paths, const std::string & waitMsg = "") - { - lockPathsImpl(_paths, waitMsg, true); - } - bool tryLockPaths(const PathSet & _paths) - { - return lockPathsImpl(_paths, "", false); - } - ~PathLocks(); + PathLock(PathLock &&) = default; + PathLock & operator=(PathLock &&) = default; + ~PathLock(); + void unlock(); }; +PathLock lockPath(const Path & path, std::string_view waitMsg = ""); +std::optional tryLockPath(const Path & path); + +using PathLocks = std::list; + +PathLocks lockPaths(const PathSet & paths, std::string_view waitMsg = ""); +std::optional tryLockPaths(const PathSet & paths); + class FdLock { struct Unlocker diff --git a/lix/libstore/profiles.cc b/lix/libstore/profiles.cc index fd29eee78..95f146a4a 100644 --- a/lix/libstore/profiles.cc +++ b/lix/libstore/profiles.cc @@ -143,8 +143,7 @@ static void deleteGeneration2(const Path & profile, GenerationNumber gen, bool d void deleteGenerations(const Path & profile, const std::set & gensToDelete, bool dryRun) { - PathLocks lock; - lockProfile(lock, profile); + PathLock lock = lockProfile(profile); auto [gens, curGen] = findGenerations(profile); @@ -170,8 +169,7 @@ void deleteGenerationsGreaterThan(const Path & profile, GenerationNumber max, bo if (max == 0) throw Error("Must keep at least one generation, otherwise the current one would be deleted"); - PathLocks lock; - lockProfile(lock, profile); + PathLock lock = lockProfile(profile); auto [gens, _curGen] = findGenerations(profile); auto curGen = _curGen; @@ -191,8 +189,7 @@ void deleteGenerationsGreaterThan(const Path & profile, GenerationNumber max, bo void deleteOldGenerations(const Path & profile, bool dryRun) { - PathLocks lock; - lockProfile(lock, profile); + PathLock lock = lockProfile(profile); auto [gens, curGen] = findGenerations(profile); @@ -204,8 +201,7 @@ void deleteOldGenerations(const Path & profile, bool dryRun) void deleteGenerationsOlderThan(const Path & profile, time_t t, bool dryRun) { - PathLocks lock; - lockProfile(lock, profile); + PathLock lock = lockProfile(profile); auto [gens, curGen] = findGenerations(profile); @@ -265,8 +261,7 @@ void switchGeneration( std::optional dstGen, bool dryRun) { - PathLocks lock; - lockProfile(lock, profile); + PathLock lock = lockProfile(profile); auto [gens, curGen] = findGenerations(profile); @@ -291,9 +286,9 @@ void switchGeneration( } -void lockProfile(PathLocks & lock, const Path & profile) +PathLock lockProfile(const Path & profile) { - lock.lockPaths({profile}, fmt("waiting for lock on profile '%1%'", profile)); + return lockPath(profile, fmt("waiting for lock on profile '%1%'", profile)); } diff --git a/lix/libstore/profiles.hh b/lix/libstore/profiles.hh index 559f6ab57..bdf6d3b70 100644 --- a/lix/libstore/profiles.hh +++ b/lix/libstore/profiles.hh @@ -191,7 +191,7 @@ void switchGeneration( * Ensure exclusive access to a profile. Any command that modifies * the profile first acquires this lock. */ -void lockProfile(PathLocks & lock, const Path & profile); +PathLock lockProfile(const Path & profile); /** * Optimistic locking is used by long-running operations like `nix-env