From 8a29052cb2f52ef2c82c36fb3818fd0f66349729 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 7 Apr 2021 12:21:31 +0200 Subject: [PATCH] PathSubstitutionGoal: Clean up pipe If there were many top-level goals (which are not destroyed until the very end), commands like $ nix copy --to 'ssh://localhost?remote-store=/tmp/nix' \ /run/current-system --no-check-sigs --substitute-on-destination could fail with "Too many open files". So now we do some explicit cleanup from amDone(). It would be cleaner to separate goals from their temporary internal state, but that would be a bigger refactor. --- src/libstore/build/goal.cc | 2 ++ src/libstore/build/goal.hh | 2 ++ src/libstore/build/substitution-goal.cc | 31 +++++++++++++++++-------- src/libstore/build/substitution-goal.hh | 4 +++- src/libstore/build/worker.cc | 1 + src/libutil/util.cc | 17 ++++++++++---- src/libutil/util.hh | 3 ++- 7 files changed, 43 insertions(+), 17 deletions(-) diff --git a/src/libstore/build/goal.cc b/src/libstore/build/goal.cc index 2dd7a4d37..9de40bdf2 100644 --- a/src/libstore/build/goal.cc +++ b/src/libstore/build/goal.cc @@ -78,6 +78,8 @@ void Goal::amDone(ExitCode result, std::optional ex) } waiters.clear(); worker.removeGoal(shared_from_this()); + + cleanup(); } diff --git a/src/libstore/build/goal.hh b/src/libstore/build/goal.hh index fca4f2d00..e6bf628cb 100644 --- a/src/libstore/build/goal.hh +++ b/src/libstore/build/goal.hh @@ -100,6 +100,8 @@ struct Goal : public std::enable_shared_from_this virtual string key() = 0; void amDone(ExitCode result, std::optional ex = {}); + + virtual void cleanup() { } }; void addToWeakGoals(WeakGoals & goals, GoalPtr p); diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index 7b1ac126e..e56cfadbe 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -20,15 +20,7 @@ PathSubstitutionGoal::PathSubstitutionGoal(const StorePath & storePath, Worker & PathSubstitutionGoal::~PathSubstitutionGoal() { - try { - if (thr.joinable()) { - // FIXME: signal worker thread to quit. - thr.join(); - worker.childTerminated(this); - } - } catch (...) { - ignoreException(); - } + cleanup(); } @@ -63,6 +55,8 @@ void PathSubstitutionGoal::tryNext() { trace("trying next substituter"); + cleanup(); + if (subs.size() == 0) { /* None left. Terminate this goal and let someone else deal with it. */ @@ -205,7 +199,7 @@ void PathSubstitutionGoal::tryToRun() thr = std::thread([this]() { try { /* Wake up the worker loop when we're done. */ - Finally updateStats([this]() { outPipe.writeSide = -1; }); + Finally updateStats([this]() { outPipe.writeSide.close(); }); Activity act(*logger, actSubstitute, Logger::Fields{worker.store.printStorePath(storePath), sub->getUri()}); PushActivity pact(act.id); @@ -288,4 +282,21 @@ void PathSubstitutionGoal::handleEOF(int fd) if (fd == outPipe.readSide.get()) worker.wakeUp(shared_from_this()); } + +void PathSubstitutionGoal::cleanup() +{ + try { + if (thr.joinable()) { + // FIXME: signal worker thread to quit. + thr.join(); + worker.childTerminated(this); + } + + outPipe.close(); + } catch (...) { + ignoreException(); + } +} + + } diff --git a/src/libstore/build/substitution-goal.hh b/src/libstore/build/substitution-goal.hh index 3b3cb7e32..70c806d23 100644 --- a/src/libstore/build/substitution-goal.hh +++ b/src/libstore/build/substitution-goal.hh @@ -14,7 +14,7 @@ struct PathSubstitutionGoal : public Goal StorePath storePath; /* The path the substituter refers to the path as. This will be - * different when the stores have different names. */ + different when the stores have different names. */ std::optional subPath; /* The remaining substituters. */ @@ -79,6 +79,8 @@ public: /* Callback used by the worker to write to the log. */ void handleChildOutput(int fd, const string & data) override; void handleEOF(int fd) override; + + void cleanup() override; }; } diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index 6c04d3ed3..0f2ade348 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -128,6 +128,7 @@ void Worker::removeGoal(GoalPtr goal) nix::removeGoal(subGoal, drvOutputSubstitutionGoals); else assert(false); + if (topGoals.find(goal) != topGoals.end()) { topGoals.erase(goal); /* If a top-level goal failed, then kill all other goals diff --git a/src/libutil/util.cc b/src/libutil/util.cc index dea9c74b7..a6fcabbec 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -752,13 +752,13 @@ AutoCloseFD::AutoCloseFD() : fd{-1} {} AutoCloseFD::AutoCloseFD(int fd) : fd{fd} {} -AutoCloseFD::AutoCloseFD(AutoCloseFD&& that) : fd{that.fd} +AutoCloseFD::AutoCloseFD(AutoCloseFD && that) : fd{that.fd} { that.fd = -1; } -AutoCloseFD& AutoCloseFD::operator =(AutoCloseFD&& that) +AutoCloseFD & AutoCloseFD::operator =(AutoCloseFD && that) { close(); fd = that.fd; @@ -789,6 +789,7 @@ void AutoCloseFD::close() if (::close(fd) == -1) /* This should never happen. */ throw SysError("closing file descriptor %1%", fd); + fd = -1; } } @@ -822,6 +823,12 @@ void Pipe::create() } +void Pipe::close() +{ + readSide.close(); + writeSide.close(); +} + ////////////////////////////////////////////////////////////////////// @@ -1121,7 +1128,7 @@ void runProgram2(const RunOptions & options) throw SysError("executing '%1%'", options.program); }, processOptions); - out.writeSide = -1; + out.writeSide.close(); std::thread writerThread; @@ -1134,7 +1141,7 @@ void runProgram2(const RunOptions & options) if (source) { - in.readSide = -1; + in.readSide.close(); writerThread = std::thread([&]() { try { std::vector buf(8 * 1024); @@ -1151,7 +1158,7 @@ void runProgram2(const RunOptions & options) } catch (...) { promise.set_exception(std::current_exception()); } - in.writeSide = -1; + in.writeSide.close(); }); } diff --git a/src/libutil/util.hh b/src/libutil/util.hh index ad49c65b3..ef5e5012b 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -188,7 +188,6 @@ public: class AutoCloseFD { int fd; - void close(); public: AutoCloseFD(); AutoCloseFD(int fd); @@ -200,6 +199,7 @@ public: int get() const; explicit operator bool() const; int release(); + void close(); }; @@ -216,6 +216,7 @@ class Pipe public: AutoCloseFD readSide, writeSide; void create(); + void close(); };