diff options
| -rw-r--r-- | src/libstore/build/derivation-goal.cc | 9 | ||||
| -rw-r--r-- | src/libstore/build/derivation-goal.hh | 8 | ||||
| -rw-r--r-- | src/libstore/build/worker.cc | 47 | ||||
| -rw-r--r-- | src/libstore/build/worker.hh | 2 |
4 files changed, 45 insertions, 21 deletions
diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 5c0452391..d90a8639d 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -130,8 +130,12 @@ kj::Promise<Result<Goal::WorkResult>> DerivationGoal::work() noexcept return useDerivation ? getDerivation() : haveDerivation(); } -void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs) +bool DerivationGoal::addWantedOutputs(const OutputsSpec & outputs) { + if (isDone) { + return false; + } + auto newWanted = wantedOutputs.union_(outputs); switch (needRestart) { case NeedRestartForMoreOutputs::OutputsUnmodifedDontNeed: @@ -148,6 +152,7 @@ void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs) break; }; wantedOutputs = newWanted; + return true; } @@ -1680,6 +1685,8 @@ Goal::Finished DerivationGoal::done( SingleDrvOutputs builtOutputs, std::optional<Error> ex) { + isDone = true; + outputLocks.unlock(); buildResult.status = status; if (ex) diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index 7505409c0..1e2f9b55c 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -74,6 +74,12 @@ struct DerivationGoal : public Goal struct InputStream; /** + * Whether this goal has completed. Completed goals can not be + * asked for more outputs, a new goal must be created instead. + */ + bool isDone = false; + + /** * Whether to use an on-disk .drv file. */ bool useDerivation; @@ -249,7 +255,7 @@ struct DerivationGoal : public Goal /** * Add wanted outputs to an already existing derivation goal. */ - void addWantedOutputs(const OutputsSpec & outputs); + bool addWantedOutputs(const OutputsSpec & outputs); /** * The states. diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index 5be706e42..e063ede71 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -57,24 +57,35 @@ std::pair<std::shared_ptr<G>, kj::Promise<void>> Worker::makeGoalCommon( std::map<ID, CachedGoal<G>> & map, const ID & key, InvocableR<std::unique_ptr<G>> auto create, - std::invocable<G &> auto modify + InvocableR<bool, G &> auto modify ) { auto [it, _inserted] = map.try_emplace(key); - auto & goal_weak = it->second; - auto goal = goal_weak.goal.lock(); - if (!goal) { - goal = create(); - goal->notify = std::move(goal_weak.fulfiller); - goal_weak.goal = goal; - // do not start working immediately, this round of the event loop - // may have more calls to this function lined up that'll also run - // modify(). starting early can then cause the goals to misbehave - childStarted(goal, kj::evalLater([goal] { return goal->work(); })); - } else { - modify(*goal); + // try twice to create the goal. we can only loop if we hit the continue, + // and then we only want to recreate the goal *once*. concurrent accesses + // to the worker are not sound, we want to catch them if at all possible. + for ([[maybe_unused]] auto _attempt : {1, 2}) { + auto & goal_weak = it->second; + auto goal = goal_weak.goal.lock(); + if (!goal) { + goal = create(); + goal->notify = std::move(goal_weak.fulfiller); + goal_weak.goal = goal; + // do not start working immediately. if we are not yet running we + // may create dependencies as though they were toplevel goals, in + // which case the dependencies will not report build errors. when + // we are running we may be called for this same goal more times, + // and then we want to modify rather than recreate when possible. + childStarted(goal, kj::evalLater([goal] { return goal->work(); })); + } else { + if (!modify(*goal)) { + goal_weak = {}; + continue; + } + } + return {goal, goal_weak.promise->addBranch()}; } - return {goal, goal_weak.promise->addBranch()}; + assert(false && "could not make a goal. possible concurrent worker access"); } @@ -94,7 +105,7 @@ std::pair<std::shared_ptr<DerivationGoal>, kj::Promise<void>> Worker::makeDeriva drvPath, wantedOutputs, *this, running, buildMode ); }, - [&](DerivationGoal & g) { g.addWantedOutputs(wantedOutputs); } + [&](DerivationGoal & g) { return g.addWantedOutputs(wantedOutputs); } ); } @@ -118,7 +129,7 @@ std::pair<std::shared_ptr<DerivationGoal>, kj::Promise<void>> Worker::makeBasicD drvPath, drv, wantedOutputs, *this, running, buildMode ); }, - [&](DerivationGoal & g) { g.addWantedOutputs(wantedOutputs); } + [&](DerivationGoal & g) { return g.addWantedOutputs(wantedOutputs); } ); } @@ -132,7 +143,7 @@ Worker::makePathSubstitutionGoal( substitutionGoals, path, [&] { return std::make_unique<PathSubstitutionGoal>(path, *this, running, repair, ca); }, - [&](auto &) {} + [&](auto &) { return true; } ); } @@ -146,7 +157,7 @@ Worker::makeDrvOutputSubstitutionGoal( drvOutputSubstitutionGoals, id, [&] { return std::make_unique<DrvOutputSubstitutionGoal>(id, *this, running, repair, ca); }, - [&](auto &) {} + [&](auto &) { return true; } ); } diff --git a/src/libstore/build/worker.hh b/src/libstore/build/worker.hh index d6cde8384..cd49fb860 100644 --- a/src/libstore/build/worker.hh +++ b/src/libstore/build/worker.hh @@ -241,7 +241,7 @@ private: std::map<ID, CachedGoal<G>> & map, const ID & key, InvocableR<std::unique_ptr<G>> auto create, - std::invocable<G &> auto modify + InvocableR<bool, G &> auto modify ); std::pair<std::shared_ptr<DerivationGoal>, kj::Promise<void>> makeDerivationGoal( const StorePath & drvPath, |