From a9f2aab22612bea940aa79cfb2eb15cc650ff869 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Sat, 5 Oct 2024 00:38:35 +0200 Subject: [PATCH] libstore: extract Worker::goalFinished specifics there's no reason to have the worker set information on goals that the goals themselves return from their entry point. doing this in the goal `work()` function is much cleaner, and a prerequisite to removing more implicit strong shared references to goals that are currently running. Change-Id: Ibb3e953ab8482a6a21ce2ed659d5023a991e7923 --- src/libstore/build/derivation-goal.cc | 2 +- src/libstore/build/derivation-goal.hh | 2 +- .../build/drv-output-substitution-goal.cc | 2 +- .../build/drv-output-substitution-goal.hh | 2 +- src/libstore/build/goal.cc | 18 ++++++++++++++++++ src/libstore/build/goal.hh | 6 ++++-- src/libstore/build/substitution-goal.cc | 2 +- src/libstore/build/substitution-goal.hh | 2 +- src/libstore/build/worker.cc | 7 ------- 9 files changed, 28 insertions(+), 15 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index c6ac39ddf..edee09e13 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -125,7 +125,7 @@ Goal::WorkResult DerivationGoal::timedOut(Error && ex) } -kj::Promise> DerivationGoal::work() noexcept +kj::Promise> DerivationGoal::workImpl() noexcept { return useDerivation ? getDerivation() : haveDerivation(); } diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index 0cacf75fd..b461b7d0d 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -250,7 +250,7 @@ struct DerivationGoal : public Goal WorkResult timedOut(Error && ex); - kj::Promise> work() noexcept override; + kj::Promise> workImpl() noexcept override; /** * Add wanted outputs to an already existing derivation goal. diff --git a/src/libstore/build/drv-output-substitution-goal.cc b/src/libstore/build/drv-output-substitution-goal.cc index 765908aeb..0d6f3fce0 100644 --- a/src/libstore/build/drv-output-substitution-goal.cc +++ b/src/libstore/build/drv-output-substitution-goal.cc @@ -24,7 +24,7 @@ DrvOutputSubstitutionGoal::DrvOutputSubstitutionGoal( } -kj::Promise> DrvOutputSubstitutionGoal::work() noexcept +kj::Promise> DrvOutputSubstitutionGoal::workImpl() noexcept try { trace("init"); diff --git a/src/libstore/build/drv-output-substitution-goal.hh b/src/libstore/build/drv-output-substitution-goal.hh index 86020705e..f959e2a7b 100644 --- a/src/libstore/build/drv-output-substitution-goal.hh +++ b/src/libstore/build/drv-output-substitution-goal.hh @@ -70,7 +70,7 @@ public: kj::Promise> outPathValid() noexcept; kj::Promise> finished() noexcept; - kj::Promise> work() noexcept override; + kj::Promise> workImpl() noexcept override; JobCategory jobCategory() const override { return JobCategory::Substitution; diff --git a/src/libstore/build/goal.cc b/src/libstore/build/goal.cc index 5910c37b2..7fbf43045 100644 --- a/src/libstore/build/goal.cc +++ b/src/libstore/build/goal.cc @@ -1,6 +1,7 @@ #include "goal.hh" #include "async-collect.hh" #include "worker.hh" +#include #include namespace nix { @@ -19,6 +20,23 @@ kj::Promise Goal::waitForAWhile() return worker.aio.provider->getTimer().afterDelay(settings.pollInterval.get() * kj::SECONDS); } +kj::Promise> Goal::work() noexcept +try { + BOOST_OUTCOME_CO_TRY(auto result, co_await workImpl()); + + trace("done"); + assert(!exitCode.has_value()); + exitCode = result.exitCode; + ex = result.ex; + + notify->fulfill(); + cleanup(); + + co_return std::move(result); +} catch (...) { + co_return result::failure(std::current_exception()); +} + kj::Promise> Goal::waitForGoals(kj::Array>> dependencies) noexcept try { diff --git a/src/libstore/build/goal.hh b/src/libstore/build/goal.hh index 03c2cf309..10926fffc 100644 --- a/src/libstore/build/goal.hh +++ b/src/libstore/build/goal.hh @@ -92,7 +92,7 @@ struct Goal */ BuildResult buildResult; - // for use by Worker only. will go away once work() is a promise. + // for use by Worker and Goal only. will go away once work() is a promise. kj::Own> notify; protected: @@ -121,6 +121,8 @@ protected: return waitForGoals(kj::arrOf>>(std::move(goals)...)); } + virtual kj::Promise> workImpl() noexcept = 0; + public: /** @@ -138,7 +140,7 @@ public: trace("goal destroyed"); } - virtual kj::Promise> work() noexcept = 0; + kj::Promise> work() noexcept; virtual void waiteeDone(GoalPtr waitee) { } diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index c0dd95da5..206bc8649 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -46,7 +46,7 @@ Goal::WorkResult PathSubstitutionGoal::done( } -kj::Promise> PathSubstitutionGoal::work() noexcept +kj::Promise> PathSubstitutionGoal::workImpl() noexcept try { trace("init"); diff --git a/src/libstore/build/substitution-goal.hh b/src/libstore/build/substitution-goal.hh index 41e665779..18b4262a4 100644 --- a/src/libstore/build/substitution-goal.hh +++ b/src/libstore/build/substitution-goal.hh @@ -87,7 +87,7 @@ public: ); ~PathSubstitutionGoal(); - kj::Promise> work() noexcept override; + kj::Promise> workImpl() noexcept override; /** * The states. diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index 0e8694a6d..e90f17678 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -195,19 +195,12 @@ static void removeGoal(std::shared_ptr goal, auto & goalMap) void Worker::goalFinished(GoalPtr goal, Goal::WorkResult & f) { - goal->trace("done"); - assert(!goal->exitCode.has_value()); - goal->exitCode = f.exitCode; - goal->ex = f.ex; - permanentFailure |= f.permanentFailure; timedOut |= f.timedOut; hashMismatch |= f.hashMismatch; checkMismatch |= f.checkMismatch; removeGoal(goal); - goal->notify->fulfill(); - goal->cleanup(); } void Worker::removeGoal(GoalPtr goal)