From 896a123605b825cd0a1548fc40da1a757ca30d25 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Sat, 5 Oct 2024 00:38:35 +0200 Subject: [PATCH] libstore: remove Goal::StillAlive this was a triumph. i'm making a note here: huge success. it's hard to overstate my satisfaction! i'm not even angry. i'm being so sincere ri actually, no. we *are* angry. this was one dumbass odyssey. nobody has asked for this. but not doing it would have locked us into old, broken protocols forever or (possibly worse) forced us to write our own async framework building on the old did-you-mean-continuations in Worker. if we had done that we'd be locked into ever more, and ever more complex, manual state management all over the place. this just could not stand. Change-Id: I43a6de1035febff59d2eff83be9ad52af4659871 --- src/libstore/build/derivation-goal.cc | 28 +++++++++---------- src/libstore/build/derivation-goal.hh | 18 ++++++------ .../build/drv-output-substitution-goal.cc | 8 +++--- src/libstore/build/goal.cc | 15 ++++------ src/libstore/build/goal.hh | 20 +++---------- src/libstore/build/local-derivation-goal.cc | 4 +-- src/libstore/build/local-derivation-goal.hh | 2 +- src/libstore/build/substitution-goal.cc | 4 +-- src/libstore/build/substitution-goal.hh | 2 +- src/libstore/build/worker.cc | 18 ++---------- src/libstore/build/worker.hh | 3 +- 11 files changed, 46 insertions(+), 76 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 60389fdd5..c6ac39ddf 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -118,7 +118,7 @@ void DerivationGoal::killChild() } -Goal::Finished DerivationGoal::timedOut(Error && ex) +Goal::WorkResult DerivationGoal::timedOut(Error && ex) { killChild(); return done(BuildResult::TimedOut, {}, std::move(ex)); @@ -728,7 +728,7 @@ retry: if (!actLock) actLock = std::make_unique(*logger, lvlWarn, actBuildWaiting, fmt("waiting for lock on %s", Magenta(showPaths(lockFiles)))); - (co_await waitForAWhile()).value(); + co_await waitForAWhile(); // we can loop very often, and `co_return co_await` always allocates a new frame goto retry; } @@ -799,7 +799,7 @@ retry: actLock = std::make_unique(*logger, lvlTalkative, actBuildWaiting, fmt("waiting for a machine to build '%s'", Magenta(worker.store.printStorePath(drvPath)))); outputLocks.unlock(); - (co_await waitForAWhile()).value(); + co_await waitForAWhile(); goto retry; } @@ -1331,7 +1331,7 @@ void DerivationGoal::closeLogFile() } -Goal::Finished DerivationGoal::tooMuchLogs() +Goal::WorkResult DerivationGoal::tooMuchLogs() { killChild(); return done( @@ -1380,7 +1380,7 @@ struct DerivationGoal::InputStream final : private kj::AsyncObject } }; -kj::Promise> DerivationGoal::handleBuilderOutput(InputStream & in) noexcept +kj::Promise> DerivationGoal::handleBuilderOutput(InputStream & in) noexcept try { auto buf = kj::heapArray(4096); while (true) { @@ -1413,7 +1413,7 @@ try { co_return std::current_exception(); } -kj::Promise> DerivationGoal::handleHookOutput(InputStream & in) noexcept +kj::Promise> DerivationGoal::handleHookOutput(InputStream & in) noexcept try { auto buf = kj::heapArray(4096); while (true) { @@ -1467,7 +1467,7 @@ try { co_return std::current_exception(); } -kj::Promise> DerivationGoal::handleChildOutput() noexcept +kj::Promise> DerivationGoal::handleChildOutput() noexcept try { assert(builderOutFD); @@ -1483,7 +1483,7 @@ try { handlers = handlers.exclusiveJoin( worker.aio.provider->getTimer() .afterDelay(settings.buildTimeout.get() * kj::SECONDS) - .then([this]() -> Outcome { + .then([this]() -> Outcome { return timedOut( Error("%1% timed out after %2% seconds", name, settings.buildTimeout) ); @@ -1491,7 +1491,7 @@ try { ); } - return handlers.then([this](auto r) -> Outcome { + return handlers.then([this](auto r) -> Outcome { if (!currentLogLine.empty()) flushLine(); return r; }); @@ -1499,7 +1499,7 @@ try { return {std::current_exception()}; } -kj::Promise> DerivationGoal::monitorForSilence() noexcept +kj::Promise> DerivationGoal::monitorForSilence() noexcept { while (true) { const auto stash = lastChildActivity; @@ -1513,13 +1513,13 @@ kj::Promise> DerivationGoal::monitorForSilence() n } } -kj::Promise> +kj::Promise> DerivationGoal::handleChildStreams(InputStream & builderIn, InputStream * hookIn) noexcept { lastChildActivity = worker.aio.provider->getTimer().now(); auto handlers = kj::joinPromisesFailFast([&] { - kj::Vector>> parts{2}; + kj::Vector>> parts{2}; parts.add(handleBuilderOutput(builderIn)); if (hookIn) { @@ -1680,7 +1680,7 @@ SingleDrvOutputs DerivationGoal::assertPathValidity() } -Goal::Finished DerivationGoal::done( +Goal::WorkResult DerivationGoal::done( BuildResult::Status status, SingleDrvOutputs builtOutputs, std::optional ex) @@ -1717,7 +1717,7 @@ Goal::Finished DerivationGoal::done( logError(ex->info()); } - return Finished{ + return WorkResult{ .exitCode = buildResult.success() ? ecSuccess : ecFailed, .result = buildResult, .ex = ex ? std::make_shared(std::move(*ex)) : nullptr, diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index 1e2f9b55c..0cacf75fd 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -18,7 +18,7 @@ struct HookInstance; struct HookReplyBase { struct [[nodiscard]] Accept { - kj::Promise> promise; + kj::Promise> promise; }; struct [[nodiscard]] Decline {}; struct [[nodiscard]] Postpone {}; @@ -248,7 +248,7 @@ struct DerivationGoal : public Goal BuildMode buildMode = bmNormal); virtual ~DerivationGoal() noexcept(false); - Finished timedOut(Error && ex); + WorkResult timedOut(Error && ex); kj::Promise> work() noexcept override; @@ -319,13 +319,13 @@ struct DerivationGoal : public Goal protected: kj::TimePoint lastChildActivity = kj::minValue; - kj::Promise> handleChildOutput() noexcept; - kj::Promise> + kj::Promise> handleChildOutput() noexcept; + kj::Promise> handleChildStreams(InputStream & builderIn, InputStream * hookIn) noexcept; - kj::Promise> handleBuilderOutput(InputStream & in) noexcept; - kj::Promise> handleHookOutput(InputStream & in) noexcept; - kj::Promise> monitorForSilence() noexcept; - Finished tooMuchLogs(); + kj::Promise> handleBuilderOutput(InputStream & in) noexcept; + kj::Promise> handleHookOutput(InputStream & in) noexcept; + kj::Promise> monitorForSilence() noexcept; + WorkResult tooMuchLogs(); void flushLine(); public: @@ -360,7 +360,7 @@ public: void started(); - Finished done( + WorkResult done( BuildResult::Status status, SingleDrvOutputs builtOutputs = {}, std::optional ex = {}); diff --git a/src/libstore/build/drv-output-substitution-goal.cc b/src/libstore/build/drv-output-substitution-goal.cc index 9ffa33d7b..765908aeb 100644 --- a/src/libstore/build/drv-output-substitution-goal.cc +++ b/src/libstore/build/drv-output-substitution-goal.cc @@ -30,7 +30,7 @@ try { /* If the derivation already exists, we’re done */ if (worker.store.queryRealisation(id)) { - co_return Finished{ecSuccess, std::move(buildResult)}; + co_return WorkResult{ecSuccess, std::move(buildResult)}; } subs = settings.useSubstitutes ? getDefaultSubstituters() : std::list>(); @@ -61,7 +61,7 @@ try { /* Hack: don't indicate failure if there were no substituters. In that case the calling derivation should just do a build. */ - co_return Finished{substituterFailed ? ecFailed : ecNoSubstituters, std::move(buildResult)}; + co_return WorkResult{substituterFailed ? ecFailed : ecNoSubstituters, std::move(buildResult)}; } sub = subs.front(); @@ -140,7 +140,7 @@ try { if (nrFailed > 0) { debug("The output path of the derivation output '%s' could not be substituted", id.to_string()); - return {Finished{ + return {WorkResult{ nrNoSubstituters > 0 || nrIncompleteClosure > 0 ? ecIncompleteClosure : ecFailed, std::move(buildResult), }}; @@ -155,7 +155,7 @@ try { kj::Promise> DrvOutputSubstitutionGoal::finished() noexcept try { trace("finished"); - return {Finished{ecSuccess, std::move(buildResult)}}; + return {WorkResult{ecSuccess, std::move(buildResult)}}; } catch (...) { return {std::current_exception()}; } diff --git a/src/libstore/build/goal.cc b/src/libstore/build/goal.cc index cfdb6717c..5910c37b2 100644 --- a/src/libstore/build/goal.cc +++ b/src/libstore/build/goal.cc @@ -11,18 +11,15 @@ void Goal::trace(std::string_view s) debug("%1%: %2%", name, s); } -kj::Promise> Goal::waitForAWhile() -try { +kj::Promise Goal::waitForAWhile() +{ trace("wait for a while"); /* If we are polling goals that are waiting for a lock, then wake up after a few seconds at most. */ - co_await worker.aio.provider->getTimer().afterDelay(settings.pollInterval.get() * kj::SECONDS); - co_return StillAlive{}; -} catch (...) { - co_return std::current_exception(); + return worker.aio.provider->getTimer().afterDelay(settings.pollInterval.get() * kj::SECONDS); } -kj::Promise> +kj::Promise> Goal::waitForGoals(kj::Array>> dependencies) noexcept try { auto left = dependencies.size(); @@ -45,11 +42,11 @@ try { waiteeDone(dep); if (dep->exitCode == ecFailed && !settings.keepGoing) { - co_return result::success(StillAlive{}); + co_return result::success(); } } - co_return result::success(StillAlive{}); + co_return result::success(); } catch (...) { co_return result::failure(std::current_exception()); } diff --git a/src/libstore/build/goal.hh b/src/libstore/build/goal.hh index 17c3d85db..03c2cf309 100644 --- a/src/libstore/build/goal.hh +++ b/src/libstore/build/goal.hh @@ -99,11 +99,7 @@ protected: AsyncSemaphore::Token slotToken; public: - - struct Finished; - - struct [[nodiscard]] StillAlive {}; - struct [[nodiscard]] Finished { + struct [[nodiscard]] WorkResult { ExitCode exitCode; BuildResult result; std::shared_ptr ex; @@ -113,21 +109,13 @@ public: bool checkMismatch = false; }; - struct [[nodiscard]] WorkResult : std::variant< - StillAlive, - Finished> - { - WorkResult() = delete; - using variant::variant; - }; - protected: - kj::Promise> waitForAWhile(); - kj::Promise> + kj::Promise waitForAWhile(); + kj::Promise> waitForGoals(kj::Array>> dependencies) noexcept; template... G> - kj::Promise> + kj::Promise> waitForGoals(std::pair, kj::Promise>... goals) noexcept { return waitForGoals(kj::arrOf>>(std::move(goals)...)); diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 5c1fcf1f8..c8c68f99f 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -214,7 +214,7 @@ retry: if (!actLock) actLock = std::make_unique(*logger, lvlWarn, actBuildWaiting, fmt("waiting for a free build user ID for '%s'", Magenta(worker.store.printStorePath(drvPath)))); - (co_await waitForAWhile()).value(); + co_await waitForAWhile(); // we can loop very often, and `co_return co_await` always allocates a new frame goto retry; } @@ -399,7 +399,7 @@ void LocalDerivationGoal::cleanupPostOutputsRegisteredModeNonCheck() // NOTE this one isn't noexcept because it's called from places that expect // exceptions to signal failure to launch. we should change this some time. -kj::Promise> LocalDerivationGoal::startBuilder() +kj::Promise> LocalDerivationGoal::startBuilder() { if ((buildUser && buildUser->getUIDCount() != 1) #if __linux__ diff --git a/src/libstore/build/local-derivation-goal.hh b/src/libstore/build/local-derivation-goal.hh index 44bcd2ffe..cd6ea2b55 100644 --- a/src/libstore/build/local-derivation-goal.hh +++ b/src/libstore/build/local-derivation-goal.hh @@ -218,7 +218,7 @@ struct LocalDerivationGoal : public DerivationGoal /** * Start building a derivation. */ - kj::Promise> startBuilder(); + kj::Promise> startBuilder(); /** * Fill in the environment for the builder. diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index 6dddf6e40..c0dd95da5 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -32,7 +32,7 @@ PathSubstitutionGoal::~PathSubstitutionGoal() } -Goal::Finished PathSubstitutionGoal::done( +Goal::WorkResult PathSubstitutionGoal::done( ExitCode result, BuildResult::Status status, std::optional errorMsg) @@ -42,7 +42,7 @@ Goal::Finished PathSubstitutionGoal::done( debug(*errorMsg); buildResult.errorMsg = *errorMsg; } - return Finished{result, std::move(buildResult)}; + return WorkResult{result, std::move(buildResult)}; } diff --git a/src/libstore/build/substitution-goal.hh b/src/libstore/build/substitution-goal.hh index dc701bcba..41e665779 100644 --- a/src/libstore/build/substitution-goal.hh +++ b/src/libstore/build/substitution-goal.hh @@ -72,7 +72,7 @@ struct PathSubstitutionGoal : public Goal */ std::optional ca; - Finished done( + WorkResult done( ExitCode result, BuildResult::Status status, std::optional errorMsg = {}); diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index e063ede71..0ca805b4d 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -193,7 +193,7 @@ static void removeGoal(std::shared_ptr goal, auto & goalMap) } -void Worker::goalFinished(GoalPtr goal, Goal::Finished & f) +void Worker::goalFinished(GoalPtr goal, Goal::WorkResult & f) { goal->trace("done"); assert(!goal->exitCode.has_value()); @@ -210,20 +210,6 @@ void Worker::goalFinished(GoalPtr goal, Goal::Finished & f) goal->cleanup(); } -void Worker::handleWorkResult(GoalPtr goal, Goal::WorkResult how) -{ - std::visit( - overloaded{ - [&](Goal::StillAlive) { - childStarted(goal, kj::evalLater([goal] { return goal->work(); })); - }, - [&](Goal::Finished & f) { goalFinished(goal, f); }, - }, - how - ); - updateStatistics(); -} - void Worker::removeGoal(GoalPtr goal) { if (auto drvGoal = std::dynamic_pointer_cast(goal)) @@ -250,7 +236,7 @@ void Worker::childStarted(GoalPtr goal, kj::Promise> pr children.add(promise .then([this, goal](auto result) { if (result.has_value()) { - handleWorkResult(goal, std::move(result.assume_value())); + goalFinished(goal, result.assume_value()); } else { childException = result.assume_error(); } diff --git a/src/libstore/build/worker.hh b/src/libstore/build/worker.hh index cd49fb860..d0bf742c5 100644 --- a/src/libstore/build/worker.hh +++ b/src/libstore/build/worker.hh @@ -139,8 +139,7 @@ private: */ bool checkMismatch = false; - void goalFinished(GoalPtr goal, Goal::Finished & f); - void handleWorkResult(GoalPtr goal, Goal::WorkResult how); + void goalFinished(GoalPtr goal, Goal::WorkResult & f); kj::Own> childFinished;