diff options
58 files changed, 449 insertions, 453 deletions
diff --git a/.editorconfig b/.editorconfig index bcee9cfce..bbc2a4908 100644 --- a/.editorconfig +++ b/.editorconfig @@ -29,3 +29,7 @@ trim_trailing_whitespace = false indent_style = space indent_size = 2 max_line_length = 0 + +[meson.build] +indent_style = space +indent_size = 2 diff --git a/doc/internal-api/doxygen.cfg.in b/doc/internal-api/doxygen.cfg.in index 73fba6948..662fb4333 100644 --- a/doc/internal-api/doxygen.cfg.in +++ b/doc/internal-api/doxygen.cfg.in @@ -33,32 +33,7 @@ GENERATE_LATEX = NO # spaces. See also FILE_PATTERNS and EXTENSION_MAPPING # Note: If this tag is empty the current directory is searched. -# FIXME Make this list more maintainable somehow. We could maybe generate this -# in the Makefile, but we would need to change how `.in` files are preprocessed -# so they can expand variables despite configure variables. - -INPUT = \ - src/libcmd \ - src/libexpr \ - src/libexpr/flake \ - tests/unit/libexpr \ - tests/unit/libexpr/value \ - tests/unit/libexpr/test \ - tests/unit/libexpr/test/value \ - src/libexpr/value \ - src/libfetchers \ - src/libmain \ - src/libstore \ - src/libstore/build \ - src/libstore/builtins \ - tests/unit/libstore \ - tests/unit/libstore/test \ - src/libutil \ - tests/unit/libutil \ - tests/unit/libutil/test \ - src/nix \ - src/nix-env \ - src/nix-store +INPUT = @INPUT_PATHS@ # If the MACRO_EXPANSION tag is set to YES, doxygen will expand all macro names # in the source code. If set to NO, only conditional compilation will be @@ -97,3 +72,15 @@ EXPAND_AS_DEFINED = \ DECLARE_WORKER_SERIALISER \ DECLARE_SERVE_SERIALISER \ LENGTH_PREFIXED_PROTO_HELPER + +# The STRIP_FROM_PATH tag can be used to strip a user-defined part of the path. +# Stripping is only done if one of the specified strings matches the left-hand +# part of the path. The tag can be used to show relative paths in the file list. +# If left blank the directory from which doxygen is run is used as the path to +# strip. +# +# Note that you can specify absolute paths here, but also relative paths, which +# will be relative from the directory where doxygen is started. +# This tag requires that the tag FULL_PATH_NAMES is set to YES. + +STRIP_FROM_PATH = "@PROJECT_SOURCE_ROOT@" diff --git a/doc/internal-api/meson.build b/doc/internal-api/meson.build index faa30f194..af93b6943 100644 --- a/doc/internal-api/meson.build +++ b/doc/internal-api/meson.build @@ -1,3 +1,35 @@ +internal_api_sources = [ + 'src/libcmd', + 'src/libexpr', + 'src/libexpr/flake', + 'tests/unit/libexpr', + 'tests/unit/libexpr/value', + 'tests/unit/libexpr/test', + 'tests/unit/libexpr/test/value', + 'src/libexpr/value', + 'src/libfetchers', + 'src/libmain', + 'src/libstore', + 'src/libstore/build', + 'src/libstore/builtins', + 'tests/unit/libstore', + 'tests/unit/libstore/test', + 'src/libutil', + 'tests/unit/libutil', + 'tests/unit/libutil/test', + 'src/nix', + 'src/nix-env', + 'src/nix-store', +] + +# We feed Doxygen absolute paths so it can be invoked from any working directory. +internal_api_sources_absolute = [] +foreach src : internal_api_sources + internal_api_sources_absolute += '"' + (meson.project_source_root() / src) + '"' +endforeach + +internal_api_sources_oneline = ' \\\n '.join(internal_api_sources_absolute) + doxygen_cfg = configure_file( input : 'doxygen.cfg.in', output : 'doxygen.cfg', @@ -5,22 +37,16 @@ doxygen_cfg = configure_file( 'PACKAGE_VERSION': meson.project_version(), 'RAPIDCHECK_HEADERS': rapidcheck_meson.get_variable('includedir'), 'docdir' : meson.current_build_dir(), + 'INPUT_PATHS' : internal_api_sources_oneline, + 'PROJECT_SOURCE_ROOT' : meson.project_source_root(), }, ) internal_api_docs = custom_target( 'internal-api-docs', command : [ - bash, - # Meson can you please just give us a `workdir` argument to custom targets... - '-c', - # We have to prefix the doxygen_cfg path with the project build root - # because of the cd in front. - 'cd @0@ && @1@ @2@/@INPUT0@'.format( - meson.project_source_root(), - doxygen.full_path(), - meson.project_build_root(), - ), + doxygen.full_path(), + '@INPUT0@', ], input : [ doxygen_cfg, diff --git a/doc/manual/meson.build b/doc/manual/meson.build index 38aad55b5..35d94740c 100644 --- a/doc/manual/meson.build +++ b/doc/manual/meson.build @@ -126,6 +126,7 @@ manual = custom_target( 'manual', 'markdown', ], + install : true, install_dir : [ datadir / 'doc/nix', false, diff --git a/doc/manual/rl-next/ctrl-c-improved.md b/doc/manual/rl-next/ctrl-c-improved.md new file mode 100644 index 000000000..c27a0edbb --- /dev/null +++ b/doc/manual/rl-next/ctrl-c-improved.md @@ -0,0 +1,13 @@ +--- +synopsis: Ctrl-C stops Nix commands much more reliably and responsively +issues: [7245, fj#393] +cls: [2016] +prs: [11618] +category: Fixes +credits: [roberth, 9999years] +--- + +CTRL-C will now stop Nix commands much more reliably and responsively. While +there are still some cases where a Nix command can be slow or unresponsive +following a `SIGINT` (please report these as issues!), the vast majority of +signals will now cause the Nix command to quit quickly and consistently. diff --git a/meson.build b/meson.build index ea2050b6b..2cf86e985 100644 --- a/meson.build +++ b/meson.build @@ -50,10 +50,9 @@ project('lix', 'cpp', 'rust', meson_version : '>=1.4.0', version : run_command('bash', '-c', 'echo -n $(jq -r .version < ./version.json)$VERSION_SUFFIX', check : true).stdout().strip(), default_options : [ - 'cpp_std=c++2a', + 'cpp_std=c++23', 'rust_std=2021', - # TODO(Qyriad): increase the warning level - 'warning_level=1', + 'warning_level=2', 'debug=true', 'optimization=2', 'errorlogs=true', # Please print logs for tests that fail @@ -485,6 +484,7 @@ add_project_arguments( # TODO(Qyriad): Yes this is how the autoconf+Make system did it. # It would be nice for our headers to be idempotent instead. '-include', 'config.h', + '-Wno-unused-parameter', '-Wno-deprecated-declarations', '-Wimplicit-fallthrough', '-Werror=switch', @@ -493,12 +493,6 @@ add_project_arguments( '-Wdeprecated-copy', '-Wignored-qualifiers', '-Werror=suggest-override', - # Enable assertions in libstdc++ by default. Harmless on libc++. Benchmarked - # at ~1% overhead in `nix search`. - # - # FIXME: remove when we get meson 1.4.0 which will default this to on for us: - # https://mesonbuild.com/Release-notes-for-1-4-0.html#ndebug-setting-now-controls-c-stdlib-assertions - '-D_GLIBCXX_ASSERTIONS=1', language : 'cpp', ) diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index 83bfd4fb0..e7336c7e8 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -79,7 +79,7 @@ struct AttrDb state->txn->commit(); state->txn.reset(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } @@ -90,7 +90,7 @@ struct AttrDb try { return fun(); } catch (SQLiteError &) { - ignoreException(); + ignoreExceptionExceptInterrupt(); failed = true; return 0; } @@ -329,7 +329,7 @@ static std::shared_ptr<AttrDb> makeAttrDb( try { return std::make_shared<AttrDb>(cfg, fingerprint, symbols); } catch (SQLiteError &) { - ignoreException(); + ignoreExceptionExceptInterrupt(); return nullptr; } } diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index 68da254e2..4b659b71a 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -21,6 +21,14 @@ std::ostream & operator <<(std::ostream & str, const SymbolStr & symbol) return printIdentifier(str, s); } +AttrName::AttrName(Symbol s) : symbol(s) +{ +} + +AttrName::AttrName(std::unique_ptr<Expr> e) : expr(std::move(e)) +{ +} + void Expr::show(const SymbolTable & symbols, std::ostream & str) const { abort(); diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index d16281c39..4e857b321 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -30,8 +30,8 @@ struct AttrName { Symbol symbol; std::unique_ptr<Expr> expr; - AttrName(Symbol s) : symbol(s) {}; - AttrName(std::unique_ptr<Expr> e) : expr(std::move(e)) {}; + AttrName(Symbol s); + AttrName(std::unique_ptr<Expr> e); }; typedef std::vector<AttrName> AttrPath; diff --git a/src/libexpr/parser/state.hh b/src/libexpr/parser/state.hh index 1d57e2f5f..3b9b90b94 100644 --- a/src/libexpr/parser/state.hh +++ b/src/libexpr/parser/state.hh @@ -9,7 +9,7 @@ namespace nix::parser { struct StringToken { std::string_view s; - bool hasIndentation; + bool hasIndentation = false; operator std::string_view() const { return s; } }; diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index 64bd00606..029b457b1 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -395,7 +395,7 @@ RunPager::~RunPager() pid.wait(); } } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libstore/build-result.hh b/src/libstore/build-result.hh index 9634fb944..846c6c9b9 100644 --- a/src/libstore/build-result.hh +++ b/src/libstore/build-result.hh @@ -47,7 +47,7 @@ struct BuildResult * @todo This should be an entire ErrorInfo object, not just a * string, for richer information. */ - std::string errorMsg; + std::string errorMsg = {}; std::string toString() const { auto strStatus = [&]() { @@ -90,7 +90,7 @@ struct BuildResult * For derivations, a mapping from the names of the wanted outputs * to actual paths. */ - SingleDrvOutputs builtOutputs; + SingleDrvOutputs builtOutputs = {}; /** * The start/stop times of the build (or one of the rounds, if it diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 5c0452391..7f72efa6a 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -107,7 +107,7 @@ DerivationGoal::~DerivationGoal() noexcept(false) { /* Careful: we should never ever throw an exception from a destructor. */ - try { closeLogFile(); } catch (...) { ignoreException(); } + try { closeLogFile(); } catch (...) { ignoreExceptionInDestructor(); } } @@ -118,20 +118,24 @@ void DerivationGoal::killChild() } -Goal::Finished DerivationGoal::timedOut(Error && ex) +Goal::WorkResult DerivationGoal::timedOut(Error && ex) { killChild(); return done(BuildResult::TimedOut, {}, std::move(ex)); } -kj::Promise<Result<Goal::WorkResult>> DerivationGoal::work() noexcept +kj::Promise<Result<Goal::WorkResult>> DerivationGoal::workImpl() 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; } @@ -262,7 +267,7 @@ try { /* We are first going to try to create the invalid output paths through substitutes. If that doesn't work, we'll build them. */ - kj::Vector<std::pair<GoalPtr, kj::Promise<void>>> dependencies; + kj::Vector<std::pair<GoalPtr, kj::Promise<Result<WorkResult>>>> dependencies; if (settings.useSubstitutes) { if (parsedDrv->substitutesAllowed()) { for (auto & [outputName, status] : initialOutputs) { @@ -371,7 +376,7 @@ try { produced using a substitute. So we have to build instead. */ kj::Promise<Result<Goal::WorkResult>> DerivationGoal::gaveUpOnSubstitution() noexcept try { - kj::Vector<std::pair<GoalPtr, kj::Promise<void>>> dependencies; + kj::Vector<std::pair<GoalPtr, kj::Promise<Result<WorkResult>>>> dependencies; /* At this point we are building all outputs, so if more are wanted there is no need to restart. */ @@ -477,7 +482,7 @@ try { } /* Check each path (slow!). */ - kj::Vector<std::pair<GoalPtr, kj::Promise<void>>> dependencies; + kj::Vector<std::pair<GoalPtr, kj::Promise<Result<WorkResult>>>> dependencies; for (auto & i : outputClosure) { if (worker.pathContentsGood(i)) continue; printError( @@ -723,7 +728,7 @@ retry: if (!actLock) actLock = std::make_unique<Activity>(*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; } @@ -794,7 +799,7 @@ retry: actLock = std::make_unique<Activity>(*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; } @@ -858,7 +863,7 @@ void replaceValidPath(const Path & storePath, const Path & tmpPath) // attempt to recover movePath(oldPath, storePath); } catch (...) { - ignoreException(); + ignoreExceptionExceptInterrupt(); } throw; } @@ -1326,7 +1331,7 @@ void DerivationGoal::closeLogFile() } -Goal::Finished DerivationGoal::tooMuchLogs() +Goal::WorkResult DerivationGoal::tooMuchLogs() { killChild(); return done( @@ -1375,7 +1380,7 @@ struct DerivationGoal::InputStream final : private kj::AsyncObject } }; -kj::Promise<Outcome<void, Goal::Finished>> DerivationGoal::handleBuilderOutput(InputStream & in) noexcept +kj::Promise<Outcome<void, Goal::WorkResult>> DerivationGoal::handleBuilderOutput(InputStream & in) noexcept try { auto buf = kj::heapArray<char>(4096); while (true) { @@ -1408,7 +1413,7 @@ try { co_return std::current_exception(); } -kj::Promise<Outcome<void, Goal::Finished>> DerivationGoal::handleHookOutput(InputStream & in) noexcept +kj::Promise<Outcome<void, Goal::WorkResult>> DerivationGoal::handleHookOutput(InputStream & in) noexcept try { auto buf = kj::heapArray<char>(4096); while (true) { @@ -1462,7 +1467,7 @@ try { co_return std::current_exception(); } -kj::Promise<Outcome<void, Goal::Finished>> DerivationGoal::handleChildOutput() noexcept +kj::Promise<Outcome<void, Goal::WorkResult>> DerivationGoal::handleChildOutput() noexcept try { assert(builderOutFD); @@ -1478,7 +1483,7 @@ try { handlers = handlers.exclusiveJoin( worker.aio.provider->getTimer() .afterDelay(settings.buildTimeout.get() * kj::SECONDS) - .then([this]() -> Outcome<void, Finished> { + .then([this]() -> Outcome<void, WorkResult> { return timedOut( Error("%1% timed out after %2% seconds", name, settings.buildTimeout) ); @@ -1486,7 +1491,7 @@ try { ); } - return handlers.then([this](auto r) -> Outcome<void, Finished> { + return handlers.then([this](auto r) -> Outcome<void, WorkResult> { if (!currentLogLine.empty()) flushLine(); return r; }); @@ -1494,7 +1499,7 @@ try { return {std::current_exception()}; } -kj::Promise<Outcome<void, Goal::Finished>> DerivationGoal::monitorForSilence() noexcept +kj::Promise<Outcome<void, Goal::WorkResult>> DerivationGoal::monitorForSilence() noexcept { while (true) { const auto stash = lastChildActivity; @@ -1508,13 +1513,13 @@ kj::Promise<Outcome<void, Goal::Finished>> DerivationGoal::monitorForSilence() n } } -kj::Promise<Outcome<void, Goal::Finished>> +kj::Promise<Outcome<void, Goal::WorkResult>> DerivationGoal::handleChildStreams(InputStream & builderIn, InputStream * hookIn) noexcept { lastChildActivity = worker.aio.provider->getTimer().now(); auto handlers = kj::joinPromisesFailFast([&] { - kj::Vector<kj::Promise<Outcome<void, Finished>>> parts{2}; + kj::Vector<kj::Promise<Outcome<void, WorkResult>>> parts{2}; parts.add(handleBuilderOutput(builderIn)); if (hookIn) { @@ -1675,11 +1680,13 @@ SingleDrvOutputs DerivationGoal::assertPathValidity() } -Goal::Finished DerivationGoal::done( +Goal::WorkResult DerivationGoal::done( BuildResult::Status status, SingleDrvOutputs builtOutputs, std::optional<Error> ex) { + isDone = true; + outputLocks.unlock(); buildResult.status = status; if (ex) @@ -1710,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<Error>(std::move(*ex)) : nullptr, diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index 7505409c0..6dd58afd2 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<Outcome<void, Goal::Finished>> promise; + kj::Promise<Outcome<void, Goal::WorkResult>> promise; }; struct [[nodiscard]] Decline {}; struct [[nodiscard]] Postpone {}; @@ -63,7 +63,7 @@ struct InitialOutputStatus { struct InitialOutput { bool wanted; Hash outputHash; - std::optional<InitialOutputStatus> known; + std::optional<InitialOutputStatus> known = {}; }; /** @@ -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; @@ -179,6 +185,11 @@ struct DerivationGoal : public Goal std::map<std::string, InitialOutput> initialOutputs; /** + * Build result. + */ + BuildResult buildResult; + + /** * File descriptor for the log file. */ AutoCloseFD fdLogFile; @@ -242,14 +253,14 @@ struct DerivationGoal : public Goal BuildMode buildMode = bmNormal); virtual ~DerivationGoal() noexcept(false); - Finished timedOut(Error && ex); + WorkResult timedOut(Error && ex); - kj::Promise<Result<WorkResult>> work() noexcept override; + kj::Promise<Result<WorkResult>> workImpl() noexcept override; /** * Add wanted outputs to an already existing derivation goal. */ - void addWantedOutputs(const OutputsSpec & outputs); + bool addWantedOutputs(const OutputsSpec & outputs); /** * The states. @@ -313,13 +324,13 @@ struct DerivationGoal : public Goal protected: kj::TimePoint lastChildActivity = kj::minValue; - kj::Promise<Outcome<void, Finished>> handleChildOutput() noexcept; - kj::Promise<Outcome<void, Finished>> + kj::Promise<Outcome<void, WorkResult>> handleChildOutput() noexcept; + kj::Promise<Outcome<void, WorkResult>> handleChildStreams(InputStream & builderIn, InputStream * hookIn) noexcept; - kj::Promise<Outcome<void, Finished>> handleBuilderOutput(InputStream & in) noexcept; - kj::Promise<Outcome<void, Finished>> handleHookOutput(InputStream & in) noexcept; - kj::Promise<Outcome<void, Finished>> monitorForSilence() noexcept; - Finished tooMuchLogs(); + kj::Promise<Outcome<void, WorkResult>> handleBuilderOutput(InputStream & in) noexcept; + kj::Promise<Outcome<void, WorkResult>> handleHookOutput(InputStream & in) noexcept; + kj::Promise<Outcome<void, WorkResult>> monitorForSilence() noexcept; + WorkResult tooMuchLogs(); void flushLine(); public: @@ -354,7 +365,7 @@ public: void started(); - Finished done( + WorkResult done( BuildResult::Status status, SingleDrvOutputs builtOutputs = {}, std::optional<Error> ex = {}); diff --git a/src/libstore/build/drv-output-substitution-goal.cc b/src/libstore/build/drv-output-substitution-goal.cc index 9ffa33d7b..f04beb884 100644 --- a/src/libstore/build/drv-output-substitution-goal.cc +++ b/src/libstore/build/drv-output-substitution-goal.cc @@ -24,13 +24,13 @@ DrvOutputSubstitutionGoal::DrvOutputSubstitutionGoal( } -kj::Promise<Result<Goal::WorkResult>> DrvOutputSubstitutionGoal::work() noexcept +kj::Promise<Result<Goal::WorkResult>> DrvOutputSubstitutionGoal::workImpl() noexcept try { trace("init"); /* If the derivation already exists, we’re done */ if (worker.store.queryRealisation(id)) { - co_return Finished{ecSuccess, std::move(buildResult)}; + co_return WorkResult{ecSuccess}; } subs = settings.useSubstitutes ? getDefaultSubstituters() : std::list<ref<Store>>(); @@ -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}; } sub = subs.front(); @@ -103,7 +103,7 @@ try { co_return co_await tryNext(); } - kj::Vector<std::pair<GoalPtr, kj::Promise<void>>> dependencies; + kj::Vector<std::pair<GoalPtr, kj::Promise<Result<WorkResult>>>> dependencies; for (const auto & [depId, depPath] : outputInfo->dependentRealisations) { if (depId != id) { if (auto localOutputInfo = worker.store.queryRealisation(depId); @@ -140,9 +140,8 @@ 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 +154,7 @@ try { kj::Promise<Result<Goal::WorkResult>> DrvOutputSubstitutionGoal::finished() noexcept try { trace("finished"); - return {Finished{ecSuccess, std::move(buildResult)}}; + return {WorkResult{ecSuccess}}; } catch (...) { return {std::current_exception()}; } 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<Result<WorkResult>> outPathValid() noexcept; kj::Promise<Result<WorkResult>> finished() noexcept; - kj::Promise<Result<WorkResult>> work() noexcept override; + kj::Promise<Result<WorkResult>> workImpl() noexcept override; JobCategory jobCategory() const override { return JobCategory::Substitution; diff --git a/src/libstore/build/entry-points.cc b/src/libstore/build/entry-points.cc index 27c341295..808179a4d 100644 --- a/src/libstore/build/entry-points.cc +++ b/src/libstore/build/entry-points.cc @@ -25,14 +25,14 @@ void Store::buildPaths(const std::vector<DerivedPath> & reqs, BuildMode buildMod StringSet failed; std::shared_ptr<Error> ex; - for (auto & i : goals) { - if (i->ex) { + for (auto & [i, result] : goals) { + if (result.ex) { if (ex) - logError(i->ex->info()); + logError(result.ex->info()); else - ex = i->ex; + ex = result.ex; } - if (i->exitCode != Goal::ecSuccess) { + if (result.exitCode != Goal::ecSuccess) { if (auto i2 = dynamic_cast<DerivationGoal *>(i.get())) failed.insert(printStorePath(i2->drvPath)); else if (auto i2 = dynamic_cast<PathSubstitutionGoal *>(i.get())) @@ -72,7 +72,7 @@ std::vector<KeyedBuildResult> Store::buildPathsWithResults( std::vector<KeyedBuildResult> results; for (auto & [req, goalPtr] : state) - results.emplace_back(goalPtr->buildResult.restrictTo(req)); + results.emplace_back(goals[goalPtr].result.restrictTo(req)); return results; } @@ -89,8 +89,8 @@ BuildResult Store::buildDerivation(const StorePath & drvPath, const BasicDerivat goals.emplace(gf.makeBasicDerivationGoal(drvPath, drv, OutputsSpec::All{}, buildMode)); return goals; }); - auto goal = *goals.begin(); - return goal->buildResult.restrictTo(DerivedPath::Built { + auto [goal, result] = *goals.begin(); + return result.result.restrictTo(DerivedPath::Built { .drvPath = makeConstantStorePathRef(drvPath), .outputs = OutputsSpec::All {}, }); @@ -116,12 +116,12 @@ void Store::ensurePath(const StorePath & path) goals.emplace(gf.makePathSubstitutionGoal(path)); return goals; }); - auto goal = *goals.begin(); + auto [goal, result] = *goals.begin(); - if (goal->exitCode != Goal::ecSuccess) { - if (goal->ex) { - goal->ex->withExitStatus(worker.failingExitStatus()); - throw std::move(*goal->ex); + if (result.exitCode != Goal::ecSuccess) { + if (result.ex) { + result.ex->withExitStatus(worker.failingExitStatus()); + throw std::move(*result.ex); } else throw Error(worker.failingExitStatus(), "path '%s' does not exist and cannot be created", printStorePath(path)); } @@ -138,9 +138,9 @@ void Store::repairPath(const StorePath & path) goals.emplace(gf.makePathSubstitutionGoal(path, Repair)); return goals; }); - auto goal = *goals.begin(); + auto [goal, result] = *goals.begin(); - if (goal->exitCode != Goal::ecSuccess) { + if (result.exitCode != Goal::ecSuccess) { /* Since substituting the path didn't work, if we have a valid deriver, then rebuild the deriver. */ auto info = queryPathInfo(path); diff --git a/src/libstore/build/goal.cc b/src/libstore/build/goal.cc index cfdb6717c..02b22b8ad 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 <boost/outcome/try.hpp> #include <kj/time.h> namespace nix { @@ -11,45 +12,60 @@ void Goal::trace(std::string_view s) debug("%1%: %2%", name, s); } -kj::Promise<Result<Goal::WorkResult>> Goal::waitForAWhile() -try { +kj::Promise<void> 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{}; + return worker.aio.provider->getTimer().afterDelay(settings.pollInterval.get() * kj::SECONDS); +} + +kj::Promise<Result<Goal::WorkResult>> Goal::work() noexcept +try { + BOOST_OUTCOME_CO_TRY(auto result, co_await workImpl()); + + trace("done"); + + cleanup(); + + co_return std::move(result); } catch (...) { - co_return std::current_exception(); + co_return result::failure(std::current_exception()); } -kj::Promise<Result<Goal::WorkResult>> -Goal::waitForGoals(kj::Array<std::pair<GoalPtr, kj::Promise<void>>> dependencies) noexcept +kj::Promise<Result<void>> +Goal::waitForGoals(kj::Array<std::pair<GoalPtr, kj::Promise<Result<WorkResult>>>> dependencies) noexcept try { auto left = dependencies.size(); for (auto & [dep, p] : dependencies) { - p = p.then([this, dep, &left] { + p = p.then([this, dep, &left](auto _result) -> Result<WorkResult> { + BOOST_OUTCOME_TRY(auto result, _result); + left--; trace(fmt("waitee '%s' done; %d left", dep->name, left)); - if (dep->exitCode != Goal::ecSuccess) ++nrFailed; - if (dep->exitCode == Goal::ecNoSubstituters) ++nrNoSubstituters; - if (dep->exitCode == Goal::ecIncompleteClosure) ++nrIncompleteClosure; + if (result.exitCode != Goal::ecSuccess) ++nrFailed; + if (result.exitCode == Goal::ecNoSubstituters) ++nrNoSubstituters; + if (result.exitCode == Goal::ecIncompleteClosure) ++nrIncompleteClosure; + + return std::move(result); }).eagerlyEvaluate(nullptr); } auto collectDeps = asyncCollect(std::move(dependencies)); while (auto item = co_await collectDeps.next()) { - auto & dep = *item; + auto & [dep, _result] = *item; + BOOST_OUTCOME_CO_TRY(auto result, _result); waiteeDone(dep); - if (dep->exitCode == ecFailed && !settings.keepGoing) { - co_return result::success(StillAlive{}); + if (result.exitCode == ecFailed && !settings.keepGoing) { + 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..29540dcd3 100644 --- a/src/libstore/build/goal.hh +++ b/src/libstore/build/goal.hh @@ -82,64 +82,37 @@ struct Goal */ std::string name; - /** - * Whether the goal is finished. - */ - std::optional<ExitCode> exitCode; - - /** - * Build result. - */ - BuildResult buildResult; - - // for use by Worker only. will go away once work() is a promise. - kj::Own<kj::PromiseFulfiller<void>> notify; - protected: AsyncSemaphore::Token slotToken; public: - - struct Finished; - - struct [[nodiscard]] StillAlive {}; - struct [[nodiscard]] Finished { + struct [[nodiscard]] WorkResult { ExitCode exitCode; - BuildResult result; - std::shared_ptr<Error> ex; + BuildResult result = {}; + std::shared_ptr<Error> ex = {}; bool permanentFailure = false; bool timedOut = false; bool hashMismatch = false; bool checkMismatch = false; }; - struct [[nodiscard]] WorkResult : std::variant< - StillAlive, - Finished> - { - WorkResult() = delete; - using variant::variant; - }; - protected: - kj::Promise<Result<WorkResult>> waitForAWhile(); - kj::Promise<Result<WorkResult>> - waitForGoals(kj::Array<std::pair<GoalPtr, kj::Promise<void>>> dependencies) noexcept; + kj::Promise<void> waitForAWhile(); + kj::Promise<Result<void>> + waitForGoals(kj::Array<std::pair<GoalPtr, kj::Promise<Result<WorkResult>>>> dependencies) noexcept; template<std::derived_from<Goal>... G> - kj::Promise<Result<Goal::WorkResult>> - waitForGoals(std::pair<std::shared_ptr<G>, kj::Promise<void>>... goals) noexcept + kj::Promise<Result<void>> + waitForGoals(std::pair<std::shared_ptr<G>, kj::Promise<Result<WorkResult>>>... goals) noexcept { - return waitForGoals(kj::arrOf<std::pair<GoalPtr, kj::Promise<void>>>(std::move(goals)...)); + return waitForGoals( + kj::arrOf<std::pair<GoalPtr, kj::Promise<Result<WorkResult>>>>(std::move(goals)...) + ); } -public: - - /** - * Exception containing an error message, if any. - */ - std::shared_ptr<Error> ex; + virtual kj::Promise<Result<WorkResult>> workImpl() noexcept = 0; +public: explicit Goal(Worker & worker, bool isDependency) : worker(worker) , isDependency(isDependency) @@ -150,7 +123,7 @@ public: trace("goal destroyed"); } - virtual kj::Promise<Result<WorkResult>> work() noexcept = 0; + kj::Promise<Result<WorkResult>> work() noexcept; virtual void waiteeDone(GoalPtr waitee) { } diff --git a/src/libstore/build/hook-instance.cc b/src/libstore/build/hook-instance.cc index f91a904cc..521f34917 100644 --- a/src/libstore/build/hook-instance.cc +++ b/src/libstore/build/hook-instance.cc @@ -1,4 +1,5 @@ #include "child.hh" +#include "error.hh" #include "file-system.hh" #include "globals.hh" #include "hook-instance.hh" @@ -86,7 +87,7 @@ HookInstance::~HookInstance() toHook.reset(); if (pid) pid.kill(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index f3d0bc8b4..c8c68f99f 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -1,4 +1,5 @@ #include "local-derivation-goal.hh" +#include "error.hh" #include "indirect-root-store.hh" #include "machines.hh" #include "store-api.hh" @@ -98,9 +99,9 @@ LocalDerivationGoal::~LocalDerivationGoal() noexcept(false) { /* Careful: we should never ever throw an exception from a destructor. */ - try { deleteTmpDir(false); } catch (...) { ignoreException(); } - try { killChild(); } catch (...) { ignoreException(); } - try { stopDaemon(); } catch (...) { ignoreException(); } + try { deleteTmpDir(false); } catch (...) { ignoreExceptionInDestructor(); } + try { killChild(); } catch (...) { ignoreExceptionInDestructor(); } + try { stopDaemon(); } catch (...) { ignoreExceptionInDestructor(); } } @@ -213,7 +214,7 @@ retry: if (!actLock) actLock = std::make_unique<Activity>(*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; } @@ -398,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<Outcome<void, Goal::Finished>> LocalDerivationGoal::startBuilder() +kj::Promise<Outcome<void, Goal::WorkResult>> LocalDerivationGoal::startBuilder() { if ((buildUser && buildUser->getUIDCount() != 1) #if __linux__ @@ -1249,7 +1250,7 @@ void LocalDerivationGoal::startDaemon() NotTrusted, daemon::Recursive); debug("terminated daemon connection"); } catch (SysError &) { - ignoreException(); + ignoreExceptionExceptInterrupt(); } }); 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<Outcome<void, Finished>> startBuilder(); + kj::Promise<Outcome<void, WorkResult>> 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 8088bf668..e0ca23a86 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -32,21 +32,21 @@ PathSubstitutionGoal::~PathSubstitutionGoal() } -Goal::Finished PathSubstitutionGoal::done( +Goal::WorkResult PathSubstitutionGoal::done( ExitCode result, BuildResult::Status status, std::optional<std::string> errorMsg) { - buildResult.status = status; + BuildResult buildResult{.status = status}; if (errorMsg) { debug(*errorMsg); buildResult.errorMsg = *errorMsg; } - return Finished{result, std::move(buildResult)}; + return WorkResult{result, std::move(buildResult)}; } -kj::Promise<Result<Goal::WorkResult>> PathSubstitutionGoal::work() noexcept +kj::Promise<Result<Goal::WorkResult>> PathSubstitutionGoal::workImpl() noexcept try { trace("init"); @@ -157,7 +157,7 @@ try { /* To maintain the closure invariant, we first have to realise the paths referenced by this one. */ - kj::Vector<std::pair<GoalPtr, kj::Promise<void>>> dependencies; + kj::Vector<std::pair<GoalPtr, kj::Promise<Result<WorkResult>>>> dependencies; for (auto & i : info->references) if (i != storePath) /* ignore self-references */ dependencies.add(worker.goalFactory().makePathSubstitutionGoal(i)); @@ -291,7 +291,7 @@ void PathSubstitutionGoal::cleanup() thr.get(); } } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libstore/build/substitution-goal.hh b/src/libstore/build/substitution-goal.hh index dc701bcba..18b4262a4 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<ContentAddress> ca; - Finished done( + WorkResult done( ExitCode result, BuildResult::Status status, std::optional<std::string> errorMsg = {}); @@ -87,7 +87,7 @@ public: ); ~PathSubstitutionGoal(); - kj::Promise<Result<WorkResult>> work() noexcept override; + kj::Promise<Result<WorkResult>> workImpl() noexcept override; /** * The states. diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index 5be706e42..d1c1abdf8 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -1,3 +1,4 @@ +#include "async-collect.hh" #include "charptr-cast.hh" #include "worker.hh" #include "finally.hh" @@ -6,6 +7,8 @@ #include "local-derivation-goal.hh" #include "signals.hh" #include "hook-instance.hh" // IWYU pragma: keep +#include <boost/outcome/try.hpp> +#include <kj/vector.h> namespace nix { @@ -43,9 +46,12 @@ Worker::~Worker() goals that refer to this worker should be gone. (Otherwise we are in trouble, since goals may call childTerminated() etc. in their destructors). */ - topGoals.clear(); children.clear(); + derivationGoals.clear(); + drvOutputSubstitutionGoals.clear(); + substitutionGoals.clear(); + assert(expectedSubstitutions == 0); assert(expectedDownloadSize == 0); assert(expectedNarSize == 0); @@ -53,32 +59,63 @@ Worker::~Worker() template<typename ID, std::derived_from<Goal> G> -std::pair<std::shared_ptr<G>, kj::Promise<void>> Worker::makeGoalCommon( +std::pair<std::shared_ptr<G>, kj::Promise<Result<Goal::WorkResult>>> 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 & cachedGoal = it->second; + auto & goal = cachedGoal.goal; + if (!goal) { + goal = create(); + // 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. + auto removeWhenDone = [goal, &map, it] { + // c++ lambda coroutine capture semantics are *so* fucked up. + return [](auto goal, auto & map, auto it) -> kj::Promise<Result<Goal::WorkResult>> { + auto result = co_await goal->work(); + // a concurrent call to makeGoalCommon may have reset our + // cached goal and replaced it with a new instance. don't + // remove the goal in this case, otherwise we will crash. + if (goal == it->second.goal) { + map.erase(it); + } + co_return result; + }(goal, map, it); + }; + cachedGoal.promise = kj::evalLater(std::move(removeWhenDone)).fork(); + children.add(cachedGoal.promise.addBranch().then([this](auto _result) { + if (_result.has_value()) { + auto & result = _result.value(); + permanentFailure |= result.permanentFailure; + timedOut |= result.timedOut; + hashMismatch |= result.hashMismatch; + checkMismatch |= result.checkMismatch; + } + })); + } else { + if (!modify(*goal)) { + cachedGoal = {}; + continue; + } + } + return {goal, cachedGoal.promise.addBranch()}; } - return {goal, goal_weak.promise->addBranch()}; + assert(false && "could not make a goal. possible concurrent worker access"); } -std::pair<std::shared_ptr<DerivationGoal>, kj::Promise<void>> Worker::makeDerivationGoal( +std::pair<std::shared_ptr<DerivationGoal>, kj::Promise<Result<Goal::WorkResult>>> Worker::makeDerivationGoal( const StorePath & drvPath, const OutputsSpec & wantedOutputs, BuildMode buildMode ) { @@ -94,12 +131,12 @@ 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); } ); } -std::pair<std::shared_ptr<DerivationGoal>, kj::Promise<void>> Worker::makeBasicDerivationGoal( +std::pair<std::shared_ptr<DerivationGoal>, kj::Promise<Result<Goal::WorkResult>>> Worker::makeBasicDerivationGoal( const StorePath & drvPath, const BasicDerivation & drv, const OutputsSpec & wantedOutputs, @@ -118,12 +155,12 @@ 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); } ); } -std::pair<std::shared_ptr<PathSubstitutionGoal>, kj::Promise<void>> +std::pair<std::shared_ptr<PathSubstitutionGoal>, kj::Promise<Result<Goal::WorkResult>>> Worker::makePathSubstitutionGoal( const StorePath & path, RepairFlag repair, std::optional<ContentAddress> ca ) @@ -132,12 +169,12 @@ Worker::makePathSubstitutionGoal( substitutionGoals, path, [&] { return std::make_unique<PathSubstitutionGoal>(path, *this, running, repair, ca); }, - [&](auto &) {} + [&](auto &) { return true; } ); } -std::pair<std::shared_ptr<DrvOutputSubstitutionGoal>, kj::Promise<void>> +std::pair<std::shared_ptr<DrvOutputSubstitutionGoal>, kj::Promise<Result<Goal::WorkResult>>> Worker::makeDrvOutputSubstitutionGoal( const DrvOutput & id, RepairFlag repair, std::optional<ContentAddress> ca ) @@ -146,119 +183,27 @@ Worker::makeDrvOutputSubstitutionGoal( drvOutputSubstitutionGoals, id, [&] { return std::make_unique<DrvOutputSubstitutionGoal>(id, *this, running, repair, ca); }, - [&](auto &) {} + [&](auto &) { return true; } ); } -std::pair<GoalPtr, kj::Promise<void>> Worker::makeGoal(const DerivedPath & req, BuildMode buildMode) +std::pair<GoalPtr, kj::Promise<Result<Goal::WorkResult>>> Worker::makeGoal(const DerivedPath & req, BuildMode buildMode) { return std::visit(overloaded { - [&](const DerivedPath::Built & bfd) -> std::pair<GoalPtr, kj::Promise<void>> { + [&](const DerivedPath::Built & bfd) -> std::pair<GoalPtr, kj::Promise<Result<Goal::WorkResult>>> { if (auto bop = std::get_if<DerivedPath::Opaque>(&*bfd.drvPath)) return makeDerivationGoal(bop->path, bfd.outputs, buildMode); else throw UnimplementedError("Building dynamic derivations in one shot is not yet implemented."); }, - [&](const DerivedPath::Opaque & bo) -> std::pair<GoalPtr, kj::Promise<void>> { + [&](const DerivedPath::Opaque & bo) -> std::pair<GoalPtr, kj::Promise<Result<Goal::WorkResult>>> { return makePathSubstitutionGoal(bo.path, buildMode == bmRepair ? Repair : NoRepair); }, }, req.raw()); } - -template<typename G> -static void removeGoal(std::shared_ptr<G> goal, auto & goalMap) -{ - /* !!! inefficient */ - for (auto i = goalMap.begin(); - i != goalMap.end(); ) - if (i->second.goal.lock() == goal) { - auto j = i; ++j; - goalMap.erase(i); - i = j; - } - else ++i; -} - - -void Worker::goalFinished(GoalPtr goal, Goal::Finished & 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::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<DerivationGoal>(goal)) - nix::removeGoal(drvGoal, derivationGoals); - else if (auto subGoal = std::dynamic_pointer_cast<PathSubstitutionGoal>(goal)) - nix::removeGoal(subGoal, substitutionGoals); - else if (auto subGoal = std::dynamic_pointer_cast<DrvOutputSubstitutionGoal>(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 - (unless keepGoing was set). */ - if (goal->exitCode == Goal::ecFailed && !settings.keepGoing) - topGoals.clear(); - } -} - - -void Worker::childStarted(GoalPtr goal, kj::Promise<Result<Goal::WorkResult>> promise) -{ - children.add(promise - .then([this, goal](auto result) { - if (result.has_value()) { - handleWorkResult(goal, std::move(result.assume_value())); - } else { - childException = result.assume_error(); - } - }) - .attach(Finally{[this, goal] { - childTerminated(goal); - }})); -} - - -void Worker::childTerminated(GoalPtr goal) -{ - if (childFinished) { - childFinished->fulfill(); - } -} - - -kj::Promise<Result<void>> Worker::updateStatistics() +kj::Promise<Result<Worker::Results>> Worker::updateStatistics() try { while (true) { statisticsUpdateInhibitor = co_await statisticsUpdateSignal.acquire(); @@ -284,52 +229,54 @@ try { co_return result::failure(std::current_exception()); } -std::vector<GoalPtr> Worker::run(std::function<Targets (GoalFactory &)> req) +Worker::Results Worker::run(std::function<Targets (GoalFactory &)> req) { - auto _topGoals = req(goalFactory()); + auto topGoals = req(goalFactory()); assert(!running); running = true; Finally const _stop([&] { running = false; }); - topGoals.clear(); - for (auto & [goal, _promise] : _topGoals) { - topGoals.insert(goal); - } + auto onInterrupt = kj::newPromiseAndCrossThreadFulfiller<Result<Results>>(); + auto interruptCallback = createInterruptCallback([&] { + return result::failure(std::make_exception_ptr(makeInterrupted())); + }); - auto promise = runImpl().exclusiveJoin(updateStatistics()); + auto promise = runImpl(std::move(topGoals)) + .exclusiveJoin(updateStatistics()) + .exclusiveJoin(std::move(onInterrupt.promise)); // TODO GC interface? - if (auto localStore = dynamic_cast<LocalStore *>(&store); localStore && settings.minFree != 0) { + if (auto localStore = dynamic_cast<LocalStore *>(&store); localStore && settings.minFree != 0u) { // Periodically wake up to see if we need to run the garbage collector. promise = promise.exclusiveJoin(boopGC(*localStore)); } - promise.wait(aio.waitScope).value(); - - std::vector<GoalPtr> results; - for (auto & [i, _p] : _topGoals) { - results.push_back(i); - } - return results; + return promise.wait(aio.waitScope).value(); } -kj::Promise<Result<void>> Worker::runImpl() +kj::Promise<Result<Worker::Results>> Worker::runImpl(Targets topGoals) try { debug("entered goal loop"); - while (1) { - - checkInterrupt(); + kj::Vector<Targets::value_type> promises(topGoals.size()); + for (auto & gp : topGoals) { + promises.add(std::move(gp)); + } - if (topGoals.empty()) break; + Results results; - /* Wait for input. */ - if (!children.isEmpty()) - (co_await waitForInput()).value(); + auto collect = AsyncCollect(promises.releaseAsArray()); + while (auto done = co_await collect.next()) { + // propagate goal exceptions outward + BOOST_OUTCOME_CO_TRY(auto result, done->second); + results.emplace(done->first, result); - if (childException) { - std::rethrow_exception(childException); + /* If a top-level goal failed, then kill all other goals + (unless keepGoing was set). */ + if (result.exitCode == Goal::ecFailed && !settings.keepGoing) { + children.clear(); + break; } } @@ -338,12 +285,12 @@ try { --keep-going *is* set, then they must all be finished now. */ assert(!settings.keepGoing || children.isEmpty()); - co_return result::success(); + co_return std::move(results); } catch (...) { co_return result::failure(std::current_exception()); } -kj::Promise<Result<void>> Worker::boopGC(LocalStore & localStore) +kj::Promise<Result<Worker::Results>> Worker::boopGC(LocalStore & localStore) try { while (true) { co_await aio.provider->getTimer().afterDelay(10 * kj::SECONDS); @@ -353,18 +300,6 @@ try { co_return result::failure(std::current_exception()); } -kj::Promise<Result<void>> Worker::waitForInput() -try { - printMsg(lvlVomit, "waiting for children"); - - auto pair = kj::newPromiseAndFulfiller<void>(); - this->childFinished = kj::mv(pair.fulfiller); - co_await pair.promise; - co_return result::success(); -} catch (...) { - co_return result::failure(std::current_exception()); -} - unsigned int Worker::failingExitStatus() { diff --git a/src/libstore/build/worker.hh b/src/libstore/build/worker.hh index d6cde8384..1a913ca16 100644 --- a/src/libstore/build/worker.hh +++ b/src/libstore/build/worker.hh @@ -30,10 +30,12 @@ struct HookInstance; class GoalFactory { public: - virtual std::pair<std::shared_ptr<DerivationGoal>, kj::Promise<void>> makeDerivationGoal( + virtual std::pair<std::shared_ptr<DerivationGoal>, kj::Promise<Result<Goal::WorkResult>>> + makeDerivationGoal( const StorePath & drvPath, const OutputsSpec & wantedOutputs, BuildMode buildMode = bmNormal ) = 0; - virtual std::pair<std::shared_ptr<DerivationGoal>, kj::Promise<void>> makeBasicDerivationGoal( + virtual std::pair<std::shared_ptr<DerivationGoal>, kj::Promise<Result<Goal::WorkResult>>> + makeBasicDerivationGoal( const StorePath & drvPath, const BasicDerivation & drv, const OutputsSpec & wantedOutputs, @@ -43,13 +45,13 @@ public: /** * @ref SubstitutionGoal "substitution goal" */ - virtual std::pair<std::shared_ptr<PathSubstitutionGoal>, kj::Promise<void>> + virtual std::pair<std::shared_ptr<PathSubstitutionGoal>, kj::Promise<Result<Goal::WorkResult>>> makePathSubstitutionGoal( const StorePath & storePath, RepairFlag repair = NoRepair, std::optional<ContentAddress> ca = std::nullopt ) = 0; - virtual std::pair<std::shared_ptr<DrvOutputSubstitutionGoal>, kj::Promise<void>> + virtual std::pair<std::shared_ptr<DrvOutputSubstitutionGoal>, kj::Promise<Result<Goal::WorkResult>>> makeDrvOutputSubstitutionGoal( const DrvOutput & id, RepairFlag repair = NoRepair, @@ -62,7 +64,7 @@ public: * It will be a `DerivationGoal` for a `DerivedPath::Built` or * a `SubstitutionGoal` for a `DerivedPath::Opaque`. */ - virtual std::pair<GoalPtr, kj::Promise<void>> + virtual std::pair<GoalPtr, kj::Promise<Result<Goal::WorkResult>>> makeGoal(const DerivedPath & req, BuildMode buildMode = bmNormal) = 0; }; @@ -82,28 +84,19 @@ protected: */ class Worker : public WorkerBase { +public: + using Targets = std::map<GoalPtr, kj::Promise<Result<Goal::WorkResult>>>; + using Results = std::map<GoalPtr, Goal::WorkResult>; + private: bool running = false; - /** - * The top-level goals of the worker. - */ - Goals topGoals; - template<typename G> struct CachedGoal { - std::weak_ptr<G> goal; - kj::Own<kj::ForkedPromise<void>> promise; - kj::Own<kj::PromiseFulfiller<void>> fulfiller; - - CachedGoal() - { - auto pf = kj::newPromiseAndFulfiller<void>(); - promise = kj::heap(pf.promise.fork()); - fulfiller = std::move(pf.fulfiller); - } + std::shared_ptr<G> goal; + kj::ForkedPromise<Result<Goal::WorkResult>> promise{nullptr}; }; /** * Maps used to prevent multiple instantiations of a goal for the @@ -139,35 +132,10 @@ private: */ bool checkMismatch = false; - void goalFinished(GoalPtr goal, Goal::Finished & f); - void handleWorkResult(GoalPtr goal, Goal::WorkResult how); - - kj::Own<kj::PromiseFulfiller<void>> childFinished; - - /** - * Wait for input to become available. - */ - kj::Promise<Result<void>> waitForInput(); - - /** - * Remove a dead goal. - */ - void removeGoal(GoalPtr goal); - - /** - * Registers a running child process. - */ - void childStarted(GoalPtr goal, kj::Promise<Result<Goal::WorkResult>> promise); - - /** - * Unregisters a running child process. - */ - void childTerminated(GoalPtr goal); - /** * Pass current stats counters to the logger for progress bar updates. */ - kj::Promise<Result<void>> updateStatistics(); + kj::Promise<Result<Results>> updateStatistics(); AsyncSemaphore statisticsUpdateSignal{1}; std::optional<AsyncSemaphore::Token> statisticsUpdateInhibitor; @@ -180,8 +148,8 @@ private: statisticsUpdateInhibitor = {}; } - kj::Promise<Result<void>> runImpl(); - kj::Promise<Result<void>> boopGC(LocalStore & localStore); + kj::Promise<Result<Results>> runImpl(Targets topGoals); + kj::Promise<Result<Results>> boopGC(LocalStore & localStore); public: @@ -196,7 +164,6 @@ public: private: kj::TaskSet children; - std::exception_ptr childException; public: struct HookState { @@ -237,29 +204,29 @@ public: */ private: template<typename ID, std::derived_from<Goal> G> - std::pair<std::shared_ptr<G>, kj::Promise<void>> makeGoalCommon( + std::pair<std::shared_ptr<G>, kj::Promise<Result<Goal::WorkResult>>> 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 ); - std::pair<std::shared_ptr<DerivationGoal>, kj::Promise<void>> makeDerivationGoal( + std::pair<std::shared_ptr<DerivationGoal>, kj::Promise<Result<Goal::WorkResult>>> makeDerivationGoal( const StorePath & drvPath, const OutputsSpec & wantedOutputs, BuildMode buildMode = bmNormal) override; - std::pair<std::shared_ptr<DerivationGoal>, kj::Promise<void>> makeBasicDerivationGoal( + std::pair<std::shared_ptr<DerivationGoal>, kj::Promise<Result<Goal::WorkResult>>> makeBasicDerivationGoal( const StorePath & drvPath, const BasicDerivation & drv, const OutputsSpec & wantedOutputs, BuildMode buildMode = bmNormal) override; /** * @ref SubstitutionGoal "substitution goal" */ - std::pair<std::shared_ptr<PathSubstitutionGoal>, kj::Promise<void>> + std::pair<std::shared_ptr<PathSubstitutionGoal>, kj::Promise<Result<Goal::WorkResult>>> makePathSubstitutionGoal( const StorePath & storePath, RepairFlag repair = NoRepair, std::optional<ContentAddress> ca = std::nullopt ) override; - std::pair<std::shared_ptr<DrvOutputSubstitutionGoal>, kj::Promise<void>> + std::pair<std::shared_ptr<DrvOutputSubstitutionGoal>, kj::Promise<Result<Goal::WorkResult>>> makeDrvOutputSubstitutionGoal( const DrvOutput & id, RepairFlag repair = NoRepair, @@ -272,16 +239,14 @@ private: * It will be a `DerivationGoal` for a `DerivedPath::Built` or * a `SubstitutionGoal` for a `DerivedPath::Opaque`. */ - std::pair<GoalPtr, kj::Promise<void>> + std::pair<GoalPtr, kj::Promise<Result<Goal::WorkResult>>> makeGoal(const DerivedPath & req, BuildMode buildMode = bmNormal) override; public: - using Targets = std::map<GoalPtr, kj::Promise<void>>; - /** * Loop until the specified top-level goals have finished. */ - std::vector<GoalPtr> run(std::function<Targets (GoalFactory &)> req); + Results run(std::function<Targets (GoalFactory &)> req); /*** * The exit status in case of failure. diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 10c810e49..34b92148e 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -6,6 +6,7 @@ #include "signals.hh" #include "compression.hh" #include "strings.hh" +#include <cstddef> #if ENABLE_S3 #include <aws/core/client/ClientConfiguration.h> @@ -115,7 +116,7 @@ struct curlFileTransfer : public FileTransfer if (!done) fail(FileTransferError(Interrupted, {}, "download of '%s' was interrupted", request.uri)); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } @@ -784,8 +785,10 @@ struct curlFileTransfer : public FileTransfer size_t read(char * data, size_t len) override { - auto readPartial = [this](char * data, size_t len) { + auto readPartial = [this](char * data, size_t len) -> size_t { const auto available = std::min(len, buffered.size()); + if (available == 0u) return 0u; + memcpy(data, buffered.data(), available); buffered.remove_prefix(available); return available; diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index d5903d01e..99bf80994 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -923,8 +923,8 @@ void LocalStore::autoGC(bool sync) } catch (...) { // FIXME: we could propagate the exception to the - // future, but we don't really care. - ignoreException(); + // future, but we don't really care. (what??) + ignoreExceptionInDestructor(); } }).detach(); diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 1af0f54de..c3248c2c3 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -481,7 +481,7 @@ LocalStore::~LocalStore() unlink(fnTempRoots.c_str()); } } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } @@ -1222,7 +1222,7 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source, try { parseDump(sink, source); } catch (...) { - ignoreException(); + ignoreExceptionExceptInterrupt(); } } }; diff --git a/src/libstore/nar-accessor.cc b/src/libstore/nar-accessor.cc index 7600de6e7..f228004a9 100644 --- a/src/libstore/nar-accessor.cc +++ b/src/libstore/nar-accessor.cc @@ -20,10 +20,10 @@ struct NarMember file in the NAR. */ uint64_t start = 0, size = 0; - std::string target; + std::string target = {}; /* If this is a directory, all the children of the directory. */ - std::map<std::string, NarMember> children; + std::map<std::string, NarMember> children = {}; }; struct NarAccessor : public FSAccessor diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index c60e5a85d..14381b6e0 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -31,7 +31,7 @@ struct MakeReadOnly /* This will make the path read-only. */ if (path != "") canonicaliseTimestampAndPermissions(path); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } }; diff --git a/src/libstore/path-with-outputs.hh b/src/libstore/path-with-outputs.hh index 57e03252d..8e2da1908 100644 --- a/src/libstore/path-with-outputs.hh +++ b/src/libstore/path-with-outputs.hh @@ -17,7 +17,7 @@ namespace nix { struct StorePathWithOutputs { StorePath path; - std::set<std::string> outputs; + std::set<std::string> outputs = {}; std::string to_string(const Store & store) const; diff --git a/src/libstore/pathlocks.cc b/src/libstore/pathlocks.cc index ced0f30bb..3225857ec 100644 --- a/src/libstore/pathlocks.cc +++ b/src/libstore/pathlocks.cc @@ -145,7 +145,7 @@ PathLocks::~PathLocks() try { unlock(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libstore/pathlocks.hh b/src/libstore/pathlocks.hh index d06d031b5..feb0f5548 100644 --- a/src/libstore/pathlocks.hh +++ b/src/libstore/pathlocks.hh @@ -1,6 +1,7 @@ #pragma once ///@file +#include "error.hh" #include "file-descriptor.hh" namespace nix { @@ -53,7 +54,7 @@ struct FdLock if (acquired) lockFile(fd, ltNone, false); } catch (SysError &) { - ignoreException(); + ignoreExceptionInDestructor(); } } }; diff --git a/src/libstore/realisation.hh b/src/libstore/realisation.hh index f2b228fa0..baeb7a2c9 100644 --- a/src/libstore/realisation.hh +++ b/src/libstore/realisation.hh @@ -50,7 +50,7 @@ struct Realisation { DrvOutput id; StorePath outPath; - StringSet signatures; + StringSet signatures = {}; /** * The realisations that are required for the current one to be valid. @@ -58,7 +58,7 @@ struct Realisation { * When importing this realisation, the store will first check that all its * dependencies exist, and map to the correct output path */ - std::map<DrvOutput, StorePath> dependentRealisations; + std::map<DrvOutput, StorePath> dependentRealisations = {}; nlohmann::json toJSON() const; static Realisation fromJSON(const nlohmann::json& json, const std::string& whence); diff --git a/src/libstore/remote-fs-accessor.cc b/src/libstore/remote-fs-accessor.cc index 0689ce74d..59d267873 100644 --- a/src/libstore/remote-fs-accessor.cc +++ b/src/libstore/remote-fs-accessor.cc @@ -29,7 +29,7 @@ ref<FSAccessor> RemoteFSAccessor::addToCache(std::string_view hashPart, std::str /* FIXME: do this asynchronously. */ writeFile(makeCacheFile(hashPart, "nar"), nar); } catch (...) { - ignoreException(); + ignoreExceptionExceptInterrupt(); } } @@ -41,7 +41,7 @@ ref<FSAccessor> RemoteFSAccessor::addToCache(std::string_view hashPart, std::str nlohmann::json j = listNar(narAccessor, "", true); writeFile(makeCacheFile(hashPart, "ls"), j.dump()); } catch (...) { - ignoreException(); + ignoreExceptionExceptInterrupt(); } } diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 1f94ca03f..a9f9818be 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -1,3 +1,4 @@ +#include "error.hh" #include "serialise.hh" #include "signals.hh" #include "path-with-outputs.hh" @@ -855,7 +856,7 @@ RemoteStore::Connection::~Connection() try { to.flush(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } @@ -985,7 +986,7 @@ void RemoteStore::ConnectionHandle::withFramedSink(std::function<void(Sink & sin try { std::rethrow_exception(ex); } catch (...) { - ignoreException(); + ignoreExceptionExceptInterrupt(); } } } diff --git a/src/libstore/sqlite.cc b/src/libstore/sqlite.cc index 8d0bfcb11..7aa0b6629 100644 --- a/src/libstore/sqlite.cc +++ b/src/libstore/sqlite.cc @@ -85,7 +85,7 @@ SQLite::~SQLite() if (db && sqlite3_close(db) != SQLITE_OK) SQLiteError::throw_(db, "closing database"); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } @@ -124,7 +124,7 @@ SQLiteStmt::~SQLiteStmt() if (stmt && sqlite3_finalize(stmt) != SQLITE_OK) SQLiteError::throw_(db, "finalizing statement '%s'", sql); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } @@ -248,7 +248,7 @@ SQLiteTxn::~SQLiteTxn() if (active && sqlite3_exec(db, "rollback;", 0, 0, 0) != SQLITE_OK) SQLiteError::throw_(db, "aborting transaction"); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 50d392779..18f80eef8 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -829,7 +829,7 @@ StorePathSet Store::queryValidPaths(const StorePathSet & paths, SubstituteFlag m { size_t left; StorePathSet valid; - std::exception_ptr exc; + std::exception_ptr exc = {}; }; Sync<State> state_(State{paths.size(), StorePathSet()}); @@ -1163,7 +1163,7 @@ std::map<StorePath, StorePath> copyPaths( // not be within our control to change that, and we might still want // to at least copy the output paths. if (e.missingFeature == Xp::CaDerivations) - ignoreException(); + ignoreExceptionExceptInterrupt(); else throw; } diff --git a/src/libutil/current-process.cc b/src/libutil/current-process.cc index 33cda211b..3b3e46a9a 100644 --- a/src/libutil/current-process.cc +++ b/src/libutil/current-process.cc @@ -49,7 +49,7 @@ unsigned int getMaxCPU() auto period = cpuMaxParts[1]; if (quota != "max") return std::ceil(std::stoi(quota) / std::stof(period)); - } catch (Error &) { ignoreException(lvlDebug); } + } catch (Error &) { ignoreExceptionInDestructor(lvlDebug); } #endif return 0; diff --git a/src/libutil/error.cc b/src/libutil/error.cc index a7cbfbfd0..f57e3ef7d 100644 --- a/src/libutil/error.cc +++ b/src/libutil/error.cc @@ -4,6 +4,7 @@ #include "position.hh" #include "terminal.hh" #include "strings.hh" +#include "signals.hh" #include <iostream> #include <optional> @@ -132,7 +133,7 @@ static std::string indent(std::string_view indentFirst, std::string_view indentR /** * A development aid for finding missing positions, to improve error messages. Example use: * - * NIX_DEVELOPER_SHOW_UNKNOWN_LOCATIONS=1 _NIX_TEST_ACCEPT=1 make tests/lang.sh.test + * NIX_DEVELOPER_SHOW_UNKNOWN_LOCATIONS=1 _NIX_TEST_ACCEPT=1 just test --suite installcheck -v functional-lang * git diff -U20 tests * */ @@ -416,7 +417,7 @@ std::ostream & showErrorInfo(std::ostream & out, const ErrorInfo & einfo, bool s return out; } -void ignoreException(Verbosity lvl) +void ignoreExceptionInDestructor(Verbosity lvl) { /* Make sure no exceptions leave this function. printError() also throws when remote is closed. */ @@ -429,4 +430,15 @@ void ignoreException(Verbosity lvl) } catch (...) { } } +void ignoreExceptionExceptInterrupt(Verbosity lvl) +{ + try { + throw; + } catch (const Interrupted & e) { + throw; + } catch (std::exception & e) { + printMsg(lvl, "error (ignored): %1%", e.what()); + } +} + } diff --git a/src/libutil/error.hh b/src/libutil/error.hh index 73c1ccadd..885a2b218 100644 --- a/src/libutil/error.hh +++ b/src/libutil/error.hh @@ -70,17 +70,17 @@ inline bool operator<=(const Trace& lhs, const Trace& rhs); inline bool operator>=(const Trace& lhs, const Trace& rhs); struct ErrorInfo { - Verbosity level; + Verbosity level = Verbosity::lvlError; HintFmt msg; std::shared_ptr<Pos> pos; - std::list<Trace> traces; + std::list<Trace> traces = {}; /** * Exit status. */ unsigned int status = 1; - Suggestions suggestions; + Suggestions suggestions = {}; static std::optional<std::string> programName; }; @@ -204,7 +204,22 @@ public: /** * Exception handling in destructors: print an error message, then * ignore the exception. + * + * If you're not in a destructor, you usually want to use `ignoreExceptionExceptInterrupt()`. + * + * This function might also be used in callbacks whose caller may not handle exceptions, + * but ideally we propagate the exception using an exception_ptr in such cases. + * See e.g. `PackBuilderContext` + */ +void ignoreExceptionInDestructor(Verbosity lvl = lvlError); + +/** + * Not destructor-safe. + * Print an error message, then ignore the exception. + * If the exception is an `Interrupted` exception, rethrow it. + * + * This may be used in a few places where Interrupt can't happen, but that's ok. */ -void ignoreException(Verbosity lvl = lvlError); +void ignoreExceptionExceptInterrupt(Verbosity lvl = lvlError); } diff --git a/src/libutil/file-descriptor.cc b/src/libutil/file-descriptor.cc index 8385ea402..cbb2bb539 100644 --- a/src/libutil/file-descriptor.cc +++ b/src/libutil/file-descriptor.cc @@ -146,7 +146,7 @@ AutoCloseFD::~AutoCloseFD() try { close(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index 1d3eba58f..c4ffb1d0c 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -522,7 +522,7 @@ AutoDelete::~AutoDelete() } } } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libutil/logging.cc b/src/libutil/logging.cc index 7d9482814..7609e6e39 100644 --- a/src/libutil/logging.cc +++ b/src/libutil/logging.cc @@ -352,7 +352,7 @@ Activity::~Activity() try { logger.stopActivity(id); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libutil/processes.hh b/src/libutil/processes.hh index dc09a9ba4..dd6e2978e 100644 --- a/src/libutil/processes.hh +++ b/src/libutil/processes.hh @@ -78,11 +78,11 @@ struct RunOptions { Path program; bool searchPath = true; - Strings args; - std::optional<uid_t> uid; - std::optional<uid_t> gid; - std::optional<Path> chdir; - std::optional<std::map<std::string, std::string>> environment; + Strings args = {}; + std::optional<uid_t> uid = {}; + std::optional<uid_t> gid = {}; + std::optional<Path> chdir = {}; + std::optional<std::map<std::string, std::string>> environment = {}; bool captureStdout = false; bool mergeStderrToStdout = false; bool isInteractive = false; diff --git a/src/libutil/serialise.cc b/src/libutil/serialise.cc index f509fedff..2f5a11a28 100644 --- a/src/libutil/serialise.cc +++ b/src/libutil/serialise.cc @@ -83,7 +83,7 @@ void BufferedSink::flush() FdSink::~FdSink() { - try { flush(); } catch (...) { ignoreException(); } + try { flush(); } catch (...) { ignoreExceptionInDestructor(); } } diff --git a/src/libutil/serialise.hh b/src/libutil/serialise.hh index 3a9685e0e..08ea9a135 100644 --- a/src/libutil/serialise.hh +++ b/src/libutil/serialise.hh @@ -549,7 +549,7 @@ struct FramedSource : Source } } } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } @@ -595,7 +595,7 @@ struct FramedSink : nix::BufferedSink to << 0; to.flush(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libutil/signals.cc b/src/libutil/signals.cc index 04a697d01..dac2964ae 100644 --- a/src/libutil/signals.cc +++ b/src/libutil/signals.cc @@ -12,13 +12,18 @@ std::atomic<bool> _isInterrupted = false; thread_local std::function<bool()> interruptCheck; +Interrupted makeInterrupted() +{ + return Interrupted("interrupted by the user"); +} + void _interrupted() { /* Block user interrupts while an exception is being handled. Throwing an exception while another exception is being handled kills the program! */ if (!std::uncaught_exceptions()) { - throw Interrupted("interrupted by the user"); + throw makeInterrupted(); } } @@ -78,7 +83,7 @@ void triggerInterrupt() try { callback(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } } diff --git a/src/libutil/signals.hh b/src/libutil/signals.hh index 02f8d2ca3..538ff94b4 100644 --- a/src/libutil/signals.hh +++ b/src/libutil/signals.hh @@ -16,10 +16,13 @@ namespace nix { /* User interruption. */ +class Interrupted; + extern std::atomic<bool> _isInterrupted; extern thread_local std::function<bool()> interruptCheck; +Interrupted makeInterrupted(); void _interrupted(); void inline checkInterrupt() diff --git a/src/libutil/thread-pool.cc b/src/libutil/thread-pool.cc index 0ff83e997..cd380b608 100644 --- a/src/libutil/thread-pool.cc +++ b/src/libutil/thread-pool.cc @@ -109,9 +109,8 @@ void ThreadPool::doWork(bool mainThread) try { std::rethrow_exception(exc); } catch (std::exception & e) { - if (!dynamic_cast<Interrupted*>(&e) && - !dynamic_cast<ThreadPoolShutDown*>(&e)) - ignoreException(); + if (!dynamic_cast<ThreadPoolShutDown*>(&e)) + ignoreExceptionExceptInterrupt(); } catch (...) { } } diff --git a/src/nix/develop.cc b/src/nix/develop.cc index fb144c904..d1615ecdc 100644 --- a/src/nix/develop.cc +++ b/src/nix/develop.cc @@ -639,7 +639,7 @@ struct CmdDevelop : Common, MixEnvironment throw Error("package 'nixpkgs#bashInteractive' does not provide a 'bin/bash'"); } catch (Error &) { - ignoreException(); + ignoreExceptionExceptInterrupt(); } // Override SHELL with the one chosen for this environment. diff --git a/src/nix/flake.cc b/src/nix/flake.cc index 15c393c90..0c704a995 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -16,6 +16,7 @@ #include "eval-cache.hh" #include "markdown.hh" #include "terminal.hh" +#include "signals.hh" #include <limits> #include <nlohmann/json.hpp> @@ -367,9 +368,11 @@ struct CmdFlakeCheck : FlakeCommand auto reportError = [&](const Error & e) { try { throw e; + } catch (Interrupted & e) { + throw; } catch (Error & e) { if (settings.keepGoing) { - ignoreException(); + ignoreExceptionExceptInterrupt(); hasErrors = true; } else diff --git a/tests/functional/build.sh b/tests/functional/build.sh index a14f6e3c2..58fba83aa 100644 --- a/tests/functional/build.sh +++ b/tests/functional/build.sh @@ -144,8 +144,8 @@ test "$(<<<"$out" grep -E '^error:' | wc -l)" = 1 # --keep-going and FOD out="$(nix build -f fod-failing.nix -L 2>&1)" && status=0 || status=$? test "$status" = 1 -# one "hash mismatch" error, one "build of ... failed" -test "$(<<<"$out" grep -E '^error:' | wc -l)" = 2 +# at least one "hash mismatch" error, one "build of ... failed" +test "$(<<<"$out" grep -E '^error:' | wc -l)" -ge 2 <<<"$out" grepQuiet -E "hash mismatch in fixed-output derivation '.*-x.\\.drv'" <<<"$out" grepQuiet -E "likely URL: " <<<"$out" grepQuiet -E "error: build of '.*-x[1-4]\\.drv\\^out', '.*-x[1-4]\\.drv\\^out', '.*-x[1-4]\\.drv\\^out', '.*-x[1-4]\\.drv\\^out' failed" diff --git a/tests/functional/lang.sh b/tests/functional/lang.sh index 59db10340..ca54fb88d 100755 --- a/tests/functional/lang.sh +++ b/tests/functional/lang.sh @@ -155,7 +155,7 @@ else echo '' echo 'You can rerun this test with:' echo '' - echo ' _NIX_TEST_ACCEPT=1 make tests/functional/lang.sh.test' + echo ' _NIX_TEST_ACCEPT=1 just test --suite installcheck -v functional-lang' echo '' echo 'to regenerate the files containing the expected output,' echo 'and then view the git diff to decide whether a change is' diff --git a/tests/unit/libstore/filetransfer.cc b/tests/unit/libstore/filetransfer.cc index 71e7392fc..fd4d326f0 100644 --- a/tests/unit/libstore/filetransfer.cc +++ b/tests/unit/libstore/filetransfer.cc @@ -150,6 +150,14 @@ TEST(FileTransfer, exceptionAbortsDownload) } } +TEST(FileTransfer, exceptionAbortsRead) +{ + auto [port, srv] = serveHTTP("200 ok", "content-length: 0\r\n", [] { return ""; }); + auto ft = makeFileTransfer(); + char buf[10] = ""; + ASSERT_THROW(ft->download(FileTransferRequest(fmt("http://[::1]:%d/index", port)))->read(buf, 10), EndOfFile); +} + TEST(FileTransfer, NOT_ON_DARWIN(reportsSetupErrors)) { auto [port, srv] = serveHTTP("404 not found", "", [] { return ""; }); diff --git a/tests/unit/libutil/compression.cc b/tests/unit/libutil/compression.cc index 8d181f53d..6dd8c96df 100644 --- a/tests/unit/libutil/compression.cc +++ b/tests/unit/libutil/compression.cc @@ -1,4 +1,5 @@ #include "compression.hh" +#include <cstddef> #include <gtest/gtest.h> namespace nix { @@ -147,7 +148,7 @@ TEST_P(PerTypeNonNullCompressionTest, truncatedValidInput) /* n.b. This also tests zero-length input, which is also invalid. * As of the writing of this comment, it returns empty output, but is * allowed to throw a compression error instead. */ - for (int i = 0; i < compressed.length(); ++i) { + for (size_t i = 0u; i < compressed.length(); ++i) { auto newCompressed = compressed.substr(compressed.length() - i); try { decompress(method, newCompressed); diff --git a/tests/unit/meson.build b/tests/unit/meson.build index c831ba379..9db619c5d 100644 --- a/tests/unit/meson.build +++ b/tests/unit/meson.build @@ -12,7 +12,11 @@ # It's only ~200 lines; better to just refactor the tests themselves which we'll want to do anyway. default_test_env = { - 'ASAN_OPTIONS': 'detect_leaks=0:halt_on_error=1:abort_on_error=1:print_summary=1:dump_instruction_bytes=1' + 'ASAN_OPTIONS': 'detect_leaks=0:halt_on_error=1:abort_on_error=1:print_summary=1:dump_instruction_bytes=1', + # Prevents loading global configuration file in /etc/nix/nix.conf in tests 😱 + 'NIX_CONF_DIR': '/var/empty', + # Prevent loading user configuration files in tests + 'NIX_USER_CONF_FILES': '', } libutil_test_support_sources = files( |