From a94ea0fd61777fdb5693b0fe3c280d5b7b9f06be Mon Sep 17 00:00:00 2001 From: Wouter den Breejen Date: Mon, 8 Oct 2007 14:04:55 +0000 Subject: [PATCH] Merged R9217 --- src/libstore/build.cc | 101 ++++++++++++++++++++++++++++---- src/libstore/pathlocks.cc | 7 +++ src/libstore/pathlocks.hh | 3 + src/libstore/worker-protocol.hh | 9 ++- src/nix-worker/nix-worker.cc | 5 +- tests/fixed.nix.in | 6 +- 6 files changed, 114 insertions(+), 17 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 4bd4e4de1..b870bfc05 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -183,6 +183,9 @@ private: WeakGoalMap derivationGoals; WeakGoalMap substitutionGoals; + /* Goals waiting for busy paths to be unlocked. */ + WeakGoals waitingForAnyGoal; + public: Worker(); @@ -224,6 +227,10 @@ public: get info about `storePath' (with --query-info). We combine substituter invocations to reduce overhead. */ void waitForInfo(GoalPtr goal, Path sub, Path storePath); + + /* Wait for any goal to finish. Pretty indiscriminate way to + wait for some resource that some other goal is holding. */ + void waitForAnyGoal(GoalPtr goal); /* Loop until the specified top-level goals have finished. */ void run(const Goals & topGoals); @@ -654,7 +661,7 @@ private: void buildDone(); /* Is the build hook willing to perform the build? */ - typedef enum {rpAccept, rpDecline, rpPostpone, rpDone} HookReply; + typedef enum {rpAccept, rpDecline, rpPostpone, rpDone, rpRestart} HookReply; HookReply tryBuildHook(); /* Synchronously wait for a build hook to finish. */ @@ -663,9 +670,13 @@ private: /* Acquires locks on the output paths and gathers information about the build (e.g., the input closures). During this process its possible that we find out that the build is - unnecessary, in which case we return false (this is not an - error condition!). */ - bool prepareBuild(); + unnecessary, in which case we return prDone. It's also + possible that some other goal is already building/substituting + the output paths, in which case we return prRestart (go back to + the haveDerivation() state). Otherwise, prProceed is + returned. */ + typedef enum {prProceed, prDone, prRestart} PrepareBuildReply; + PrepareBuildReply prepareBuild(); /* Start building a derivation. */ void startBuilder(); @@ -805,6 +816,20 @@ void DerivationGoal::haveDerivation() return; } + /* If this is a fixed-output derivation, it is possible that some + other goal is already building the output paths. (The case + where some other process is building it is handled through + normal locking mechanisms.) So if any output paths are already + being built, put this goal to sleep. */ + for (PathSet::iterator i = invalidOutputs.begin(); + i != invalidOutputs.end(); ++i) + if (pathIsLockedByMe(*i)) { + /* Wait until any goal finishes (hopefully the one that is + locking *i), then retry haveDerivation(). */ + worker.waitForAnyGoal(shared_from_this()); + return; + } + /* We are first going to try to create the invalid output paths through substitutes. If that doesn't work, we'll build them. */ @@ -898,6 +923,12 @@ void DerivationGoal::tryToBuild() /* Somebody else did it. */ amDone(ecSuccess); return; + case rpRestart: + /* Somebody else is building this output path. + Restart from haveDerivation(). */ + state = &DerivationGoal::haveDerivation; + worker.waitForAnyGoal(shared_from_this()); + return; } /* Make sure that we are allowed to start a build. */ @@ -908,9 +939,14 @@ void DerivationGoal::tryToBuild() /* Acquire locks and such. If we then see that the build has been done by somebody else, we're done. */ - if (!prepareBuild()) { + PrepareBuildReply preply = prepareBuild(); + if (preply == prDone) { amDone(ecSuccess); return; + } else if (preply == prRestart) { + state = &DerivationGoal::haveDerivation; + worker.waitForAnyGoal(shared_from_this()); + return; } /* Okay, we have to build. */ @@ -1209,11 +1245,12 @@ DerivationGoal::HookReply DerivationGoal::tryBuildHook() /* Acquire locks and such. If we then see that the output paths are now valid, we're done. */ - if (!prepareBuild()) { + PrepareBuildReply preply = prepareBuild(); + if (preply == prDone || preply == prRestart) { /* Tell the hook to exit. */ writeLine(toHook.writeSide, "cancel"); terminateBuildHook(); - return rpDone; + return preply == prDone ? rpDone : rpRestart; } printMsg(lvlInfo, format("running hook to build path(s) %1%") @@ -1289,11 +1326,27 @@ void DerivationGoal::terminateBuildHook(bool kill) } -bool DerivationGoal::prepareBuild() +DerivationGoal::PrepareBuildReply DerivationGoal::prepareBuild() { + /* Check for the possibility that some other goal in this process + has locked the output since we checked in haveDerivation(). + (It can't happen between here and the lockPaths() call below + because we're not allowing multi-threading.) */ + for (DerivationOutputs::iterator i = drv.outputs.begin(); + i != drv.outputs.end(); ++i) + if (pathIsLockedByMe(i->second.path)) { + debug(format("restarting derivation `%1%' because `%2%' is locked by another goal") + % drvPath % i->second.path); + return prRestart; + } + /* Obtain locks on all output paths. The locks are automatically released when we exit this function or Nix crashes. */ /* !!! BUG: this could block, which is not allowed. */ + /* !!! and once we make this non-blocking, we should carefully + consider the case where some but not all locks are required; we + should then release the acquired locks so that the other + processes and the pathIsLockedByMe() test don't get confused. */ outputLocks.lockPaths(outputPaths(drv.outputs), (format("waiting for lock on %1%") % showPaths(outputPaths(drv.outputs))).str()); @@ -1309,7 +1362,7 @@ bool DerivationGoal::prepareBuild() debug(format("skipping build of derivation `%1%', someone beat us to it") % drvPath); outputLocks.setDeletion(true); - return false; + return prDone; } if (validPaths.size() > 0) { @@ -1380,7 +1433,7 @@ bool DerivationGoal::prepareBuild() allPaths.insert(inputPaths.begin(), inputPaths.end()); allStatePaths.insert(inputStatePaths.begin(), inputStatePaths.end()); - return true; + return prProceed; } @@ -2171,6 +2224,17 @@ void SubstitutionGoal::tryToRun() return; } + /* Maybe a derivation goal has already locked this path + (exceedingly unlikely, since it should have used a substitute + first, but let's be defensive). */ + outputLock.reset(); // make sure this goal's lock is gone + if (pathIsLockedByMe(storePath)) { + debug(format("restarting substitution of `%1%' because it's locked by another goal") + % storePath); + worker.waitForAnyGoal(shared_from_this()); + return; /* restart in the tryToRun() state when another goal finishes */ + } + /* Acquire a lock on the output path. */ outputLock = boost::shared_ptr(new PathLocks); outputLock->lockPaths(singleton(storePath), @@ -2400,6 +2464,16 @@ void Worker::removeGoal(GoalPtr goal) if (goal->getExitCode() == Goal::ecFailed && !keepGoing) topGoals.clear(); } + + /* Wake up goals waiting for any goal to finish. */ + for (WeakGoals::iterator i = waitingForAnyGoal.begin(); + i != waitingForAnyGoal.end(); ++i) + { + GoalPtr goal = i->lock(); + if (goal) wakeUp(goal); + } + + waitingForAnyGoal.clear(); } @@ -2486,6 +2560,13 @@ void Worker::waitForInfo(GoalPtr goal, Path sub, Path storePath) } +void Worker::waitForAnyGoal(GoalPtr goal) +{ + debug("wait for any goal"); + waitingForAnyGoal.insert(goal); +} + + void Worker::getInfo() { for (map::iterator i = requestedInfo.begin(); diff --git a/src/libstore/pathlocks.cc b/src/libstore/pathlocks.cc index 821d4d02f..9d582206d 100644 --- a/src/libstore/pathlocks.cc +++ b/src/libstore/pathlocks.cc @@ -223,5 +223,12 @@ void PathLocks::setDeletion(bool deletePaths) this->deletePaths = deletePaths; } + +bool pathIsLockedByMe(const Path & path) +{ + Path lockPath = path + ".lock"; + return lockedPaths.find(lockPath) != lockedPaths.end(); +} + } diff --git a/src/libstore/pathlocks.hh b/src/libstore/pathlocks.hh index 87bb7bf3e..0ca1ce687 100644 --- a/src/libstore/pathlocks.hh +++ b/src/libstore/pathlocks.hh @@ -40,6 +40,9 @@ public: }; +bool pathIsLockedByMe(const Path & path); + + } diff --git a/src/libstore/worker-protocol.hh b/src/libstore/worker-protocol.hh index 46d03ea79..66ccc1da1 100644 --- a/src/libstore/worker-protocol.hh +++ b/src/libstore/worker-protocol.hh @@ -58,9 +58,12 @@ typedef enum { #define STDERR_ERROR 0x63787470 -/* The default location of the daemon socket, relative to - nixStateDir. */ -#define DEFAULT_SOCKET_PATH "/daemon.socket" +/* The default location of the daemon socket, relative to nixStateDir. + The socket is in a directory to allow you to control access to the + Nix daemon by setting the mode/ownership of the directory + appropriately. (This wouldn't work on the socket itself since it + must be deleted and recreated on startup.) */ +#define DEFAULT_SOCKET_PATH "/daemon-socket/socket" Path readStorePath(Source & from); diff --git a/src/nix-worker/nix-worker.cc b/src/nix-worker/nix-worker.cc index 6b6e601fe..f901649a9 100644 --- a/src/nix-worker/nix-worker.cc +++ b/src/nix-worker/nix-worker.cc @@ -753,6 +753,8 @@ static void daemonLoop() string socketPath = nixStateDir + DEFAULT_SOCKET_PATH; + createDirs(dirOf(socketPath)); + struct sockaddr_un addr; addr.sun_family = AF_UNIX; if (socketPath.size() >= sizeof(addr.sun_path)) @@ -762,7 +764,8 @@ static void daemonLoop() unlink(socketPath.c_str()); /* Make sure that the socket is created with 0666 permission - (everybody can connect). */ + (everybody can connect --- provided they have access to the + directory containing the socket). */ mode_t oldMode = umask(0111); int res = bind(fdSocket, (struct sockaddr *) &addr, sizeof(addr)); umask(oldMode); diff --git a/tests/fixed.nix.in b/tests/fixed.nix.in index 47eff91e0..c7dd99e6f 100644 --- a/tests/fixed.nix.in +++ b/tests/fixed.nix.in @@ -41,8 +41,8 @@ rec { # Test for building two derivations in parallel that produce the # same output path because they're fixed-output derivations. parallelSame = [ - (f2 "foo" ./fixed.builder2.sh "flat" "md5" "3670af73070fa14077ad74e0f5ea4e42") - (f2 "bar" ./fixed.builder2.sh "flat" "md5" "3670af73070fa14077ad74e0f5ea4e42") + (f2 "foo" ./fixed.builder2.sh "recursive" "md5" "3670af73070fa14077ad74e0f5ea4e42") + (f2 "bar" ./fixed.builder2.sh "recursive" "md5" "3670af73070fa14077ad74e0f5ea4e42") ]; -} \ No newline at end of file +}