diff --git a/doc/manual/rl-next/c-api-recoverable-errors.md b/doc/manual/rl-next/c-api-recoverable-errors.md new file mode 100644 index 00000000000..462984f529b --- /dev/null +++ b/doc/manual/rl-next/c-api-recoverable-errors.md @@ -0,0 +1,23 @@ +--- +synopsis: "C API: Errors returned from your primops are not treated as recoverable by default" +prs: [15286, 13930] +--- + +Nix 2.34 by default remembers the error in the thunk that triggered it. + +Previously the following sequence of events worked: + +1. Have a thunk that invokes a primop that's defined through the C API +2. The primop returns an error +3. Force the thunk again +4. The primop returns a value +5. The thunk evaluated successfully + +**Resolution** + +C API consumers that rely on this must change their recoverable error calls: + +```diff +-nix_set_err_msg(context, NIX_ERR_*, msg); ++nix_set_err_msg(context, NIX_ERR_RECOVERABLE, msg); +``` diff --git a/src/libexpr-c/nix_api_value.cc b/src/libexpr-c/nix_api_value.cc index 7fd8233adec..c27b5cf1523 100644 --- a/src/libexpr-c/nix_api_value.cc +++ b/src/libexpr-c/nix_api_value.cc @@ -1,4 +1,5 @@ #include "nix/expr/attr-set.hh" +#include "nix/expr/eval-error.hh" #include "nix/util/configuration.hh" #include "nix/expr/eval.hh" #include "nix/store/globals.hh" @@ -107,8 +108,13 @@ static void nix_c_primop_wrapper( f(userdata, &ctx, (EvalState *) &state, external_args.data(), vTmpPtr); if (ctx.last_err_code != NIX_OK) { - /* TODO: Throw different errors depending on the error code */ - state.error("Error from custom function: %s", *ctx.last_err).atPos(pos).debugThrow(); + if (ctx.last_err_code == NIX_ERR_RECOVERABLE) { + state.error("Recoverable error from custom function: %s", *ctx.last_err) + .atPos(pos) + .debugThrow(); + } else { + state.error("Error from custom function: %s", *ctx.last_err).atPos(pos).debugThrow(); + } } if (!vTmp.isValid()) { @@ -194,6 +200,8 @@ ValueType nix_get_type(nix_c_context * context, const nix_value * value) switch (v.type()) { case nThunk: return NIX_TYPE_THUNK; + case nFailed: + return NIX_TYPE_FAILED; case nInt: return NIX_TYPE_INT; case nFloat: diff --git a/src/libexpr-c/nix_api_value.h b/src/libexpr-c/nix_api_value.h index 5bd45da9059..0220bf68e2d 100644 --- a/src/libexpr-c/nix_api_value.h +++ b/src/libexpr-c/nix_api_value.h @@ -100,7 +100,8 @@ typedef enum { /** @brief External value from C++ plugins or C API * @see Externals */ - NIX_TYPE_EXTERNAL + NIX_TYPE_EXTERNAL, + NIX_TYPE_FAILED, } ValueType; // forward declarations diff --git a/src/libexpr-tests/nix_api_expr.cc b/src/libexpr-tests/nix_api_expr.cc index c7e246c727f..56363038d0c 100644 --- a/src/libexpr-tests/nix_api_expr.cc +++ b/src/libexpr-tests/nix_api_expr.cc @@ -517,4 +517,106 @@ TEST_F(nix_api_expr_test, nix_expr_attrset_update) assert_ctx_ok(); } +// The following is a test case for retryable thunks. This is a requirement +// for the current way in which NixOps4 evaluates its deployment expressions. +// An alternative strategy could be implemented, but unwinding the stack may +// be a more efficient way to deal with many suspensions/resumptions, compared +// to e.g. using a thread or coroutine stack for each suspended dependency. +// This test models the essential bits of a deployment tool that uses such +// a strategy. + +// State for the retryable primop - simulates deployment resource availability +struct DeploymentResourceState +{ + bool vm_created = false; +}; + +static void primop_load_resource_input( + void * user_data, nix_c_context * context, EvalState * state, nix_value ** args, nix_value * ret) +{ + assert(context); + assert(state); + auto * resource_state = static_cast(user_data); + + // Get the resource input name argument + std::string input_name; + if (nix_get_string(context, args[0], OBSERVE_STRING(input_name)) != NIX_OK) + return; + + // Only handle "vm_id" input - throw for anything else + if (input_name != "vm_id") { + std::string error_msg = "unknown resource input: " + input_name; + nix_set_err_msg(context, NIX_ERR_NIX_ERROR, error_msg.c_str()); + return; + } + + if (resource_state->vm_created) { + // VM has been created, return the ID + nix_init_string(context, ret, "vm-12345"); + } else { + // VM not created yet, fail with dependency error + nix_set_err_msg(context, NIX_ERR_RECOVERABLE, "VM not yet created"); + } +} + +TEST_F(nix_api_expr_test, nix_expr_thunk_re_evaluation_after_deployment) +{ + // This test demonstrates NixOps4's requirement: a thunk calling a primop should be + // re-evaluable when deployment resources become available that were not available initially. + + DeploymentResourceState resource_state; + + PrimOp * primop = nix_alloc_primop( + ctx, + primop_load_resource_input, + 1, + "loadResourceInput", + nullptr, + "load a deployment resource input", + &resource_state); + assert_ctx_ok(); + + nix_value * primopValue = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_init_primop(ctx, primopValue, primop); + assert_ctx_ok(); + + nix_value * inputName = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_init_string(ctx, inputName, "vm_id"); + assert_ctx_ok(); + + // Create a single thunk by using nix_init_apply instead of nix_value_call + // This creates a lazy application that can be forced multiple times + nix_value * thunk = nix_alloc_value(ctx, state); + assert_ctx_ok(); + nix_init_apply(ctx, thunk, primopValue, inputName); + assert_ctx_ok(); + + // First force: VM not created yet, should fail + nix_value_force(ctx, state, thunk); + ASSERT_EQ(NIX_ERR_NIX_ERROR, nix_err_code(ctx)); + ASSERT_THAT(nix_err_msg(nullptr, ctx, nullptr), testing::HasSubstr("VM not yet created")); + + // Clear the error context for the next attempt + nix_c_context_free(ctx); + ctx = nix_c_context_create(); + + // Simulate deployment process: VM gets created + resource_state.vm_created = true; + + // Second force of the SAME thunk: this is where the "failed" value issue appears + // With failed value caching, this should fail because the thunk is marked as permanently failed + // Without failed value caching (or with retryable failures), this should succeed + nix_value_force(ctx, state, thunk); + + // If we get here without error, the thunk was successfully re-evaluated + assert_ctx_ok(); + + std::string result; + nix_get_string(ctx, thunk, OBSERVE_STRING(result)); + assert_ctx_ok(); + ASSERT_STREQ("vm-12345", result.c_str()); +} + } // namespace nixC diff --git a/src/libexpr-tests/value/print.cc b/src/libexpr-tests/value/print.cc index d226062197d..50e78363e3f 100644 --- a/src/libexpr-tests/value/print.cc +++ b/src/libexpr-tests/value/print.cc @@ -188,6 +188,22 @@ TEST_F(ValuePrintingTests, vBlackhole) test(vBlackhole, "«potential infinite recursion»"); } +TEST_F(ValuePrintingTests, vFailed) +{ + Value v; + try { + throw Error("nope"); + } catch (...) { + v.mkFailed(std::current_exception(), nullptr); + } + + // Historically, a tried and then ignored value (e.g. through tryEval) was + // reverted to the original thunk. + + test(v, "«thunk»"); + test(v, ANSI_MAGENTA "«thunk»" ANSI_NORMAL, PrintOptions{.ansiColors = true}); +} + TEST_F(ValuePrintingTests, depthAttrs) { Value vOne; diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index 4f6c5fe8e78..b2d510ce031 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -11,7 +11,7 @@ namespace nix::eval_cache { CachedEvalError::CachedEvalError(ref cursor, Symbol attr) - : EvalError(cursor->root->state, "cached failure of attribute '%s'", cursor->getAttrPathStr(attr)) + : CloneableError(cursor->root->state, "cached failure of attribute '%s'", cursor->getAttrPathStr(attr)) , cursor(cursor) , attr(attr) { diff --git a/src/libexpr/eval-error.cc b/src/libexpr/eval-error.cc index 5da804e7ac4..a4a9b65318d 100644 --- a/src/libexpr/eval-error.cc +++ b/src/libexpr/eval-error.cc @@ -114,5 +114,6 @@ template class EvalErrorBuilder; template class EvalErrorBuilder; template class EvalErrorBuilder; template class EvalErrorBuilder; +template class EvalErrorBuilder; } // namespace nix diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index ba52e813ba6..ff913bfa51d 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1,4 +1,5 @@ #include "nix/expr/eval.hh" +#include "nix/expr/eval-error.hh" #include "nix/expr/eval-settings.hh" #include "nix/expr/primops.hh" #include "nix/expr/print-options.hh" @@ -31,6 +32,7 @@ #include #include #include +#include #include #include #include @@ -155,6 +157,8 @@ std::string_view showType(ValueType type, bool withArticle) return WA("a", "float"); case nThunk: return WA("a", "thunk"); + case nFailed: + return WA("an", "error"); } unreachable(); } @@ -2178,6 +2182,54 @@ void ExprBlackHole::eval(EvalState & state, [[maybe_unused]] Env & env, Value & // always force this to be separate, otherwise forceValue may inline it and take // a massive perf hit [[gnu::noinline]] +void EvalState::handleEvalExceptionForThunk(Env * env, Expr * expr, Value & v, const PosIdx pos) +{ + if (!env) + tryFixupBlackHolePos(v, pos); + + auto e = std::current_exception(); + Value * recovery = nullptr; + try { + std::rethrow_exception(e); + } catch (const RecoverableEvalError & e) { + recovery = allocValue(); + } catch (...) { + } + if (recovery) { + recovery->mkThunk(env, expr); + } + v.mkFailed(e, recovery); +} + +[[gnu::noinline]] +void EvalState::handleEvalExceptionForApp(Value & v, const Value & savedApp) +{ + auto e = std::current_exception(); + Value * recovery = nullptr; + try { + std::rethrow_exception(e); + } catch (const RecoverableEvalError & e) { + recovery = allocValue(); + } catch (...) { + } + if (recovery) { + *recovery = savedApp; + } + v.mkFailed(e, recovery); +} + +[[gnu::noinline]] +void EvalState::handleEvalFailed(Value & v, const PosIdx pos) +{ + assert(v.isFailed()); + if (auto recoveryValue = v.failed().recoveryValue) { + v = *recoveryValue; + forceValue(v, pos); + } else { + v.failed().rethrow(); + } +} + void EvalState::tryFixupBlackHolePos(Value & v, PosIdx pos) { if (!v.isBlackhole()) @@ -2186,7 +2238,8 @@ void EvalState::tryFixupBlackHolePos(Value & v, PosIdx pos) try { std::rethrow_exception(e); } catch (InfiniteRecursionError & e) { - e.atPos(positions[pos]); + if (!e.hasPos()) + e.atPos(positions[pos]); } catch (...) { } } @@ -2825,8 +2878,11 @@ void EvalState::assertEqValues(Value & v1, Value & v2, const PosIdx pos, std::st } return; - case nThunk: // Must not be left by forceValue - assert(false); + // Cannot be returned by forceValue(). + case nThunk: + case nFailed: + unreachable(); + default: // Note that we pass compiler flags that should make `default:` unreachable. // Also note that this probably ran after `eqValues`, which implements // the same logic more efficiently (without having to unwind stacks), @@ -2920,8 +2976,11 @@ bool EvalState::eqValues(Value & v1, Value & v2, const PosIdx pos, std::string_v // !!! return v1.fpoint() == v2.fpoint(); - case nThunk: // Must not be left by forceValue - assert(false); + // Cannot be returned by forceValue(). + case nThunk: + case nFailed: + unreachable(); + default: // Note that we pass compiler flags that should make `default:` unreachable. error("eqValues: cannot compare %1% with %2%", showType(v1), showType(v2)) .withTrace(pos, errorCtx) diff --git a/src/libexpr/include/nix/expr/eval-cache.hh b/src/libexpr/include/nix/expr/eval-cache.hh index 6d82f8c7e35..165b27420c4 100644 --- a/src/libexpr/include/nix/expr/eval-cache.hh +++ b/src/libexpr/include/nix/expr/eval-cache.hh @@ -14,7 +14,7 @@ namespace nix::eval_cache { struct AttrDb; class AttrCursor; -struct CachedEvalError : EvalError +struct CachedEvalError : CloneableError { const ref cursor; const Symbol attr; diff --git a/src/libexpr/include/nix/expr/eval-error.hh b/src/libexpr/include/nix/expr/eval-error.hh index 06a7790d5e4..dd679810e8a 100644 --- a/src/libexpr/include/nix/expr/eval-error.hh +++ b/src/libexpr/include/nix/expr/eval-error.hh @@ -18,7 +18,7 @@ class EvalErrorBuilder; * * Most subclasses should inherit from `EvalError` instead of this class. */ -class EvalBaseError : public Error +class EvalBaseError : public CloneableError { template friend class EvalErrorBuilder; @@ -26,14 +26,14 @@ public: EvalState & state; EvalBaseError(EvalState & state, ErrorInfo && errorInfo) - : Error(errorInfo) + : CloneableError(errorInfo) , state(state) { } template explicit EvalBaseError(EvalState & state, const std::string & formatString, const Args &... formatArgs) - : Error(formatString, formatArgs...) + : CloneableError(formatString, formatArgs...) , state(state) { } @@ -60,23 +60,31 @@ MakeError(InfiniteRecursionError, EvalError); * Inherits from EvalBaseError (not EvalError) because resource exhaustion * should not be cached. */ -struct StackOverflowError : public EvalBaseError +struct StackOverflowError : public CloneableError { StackOverflowError(EvalState & state) - : EvalBaseError(state, "stack overflow; max-call-depth exceeded") + : CloneableError(state, "stack overflow; max-call-depth exceeded") { } }; MakeError(IFDError, EvalBaseError); -struct InvalidPathError : public EvalError +/** + * An evaluation error which should be retried instead of rethrown. + * + * A RecoverableEvalError is not an EvalError, because we shouldn't cache it in + * the eval cache, as it should be retried anyway. + */ +MakeError(RecoverableEvalError, EvalBaseError); + +struct InvalidPathError : public CloneableError { public: Path path; InvalidPathError(EvalState & state, const Path & path) - : EvalError(state, "path '%s' is not valid", path) + : CloneableError(state, "path '%s' is not valid", path) { } }; diff --git a/src/libexpr/include/nix/expr/eval-gc.hh b/src/libexpr/include/nix/expr/eval-gc.hh index 813c2920d0e..1b1eb7e627f 100644 --- a/src/libexpr/include/nix/expr/eval-gc.hh +++ b/src/libexpr/include/nix/expr/eval-gc.hh @@ -33,6 +33,9 @@ using gc_allocator = std::allocator; struct gc {}; +struct gc_cleanup +{}; + #endif namespace nix { diff --git a/src/libexpr/include/nix/expr/eval-inline.hh b/src/libexpr/include/nix/expr/eval-inline.hh index 561c07f0891..451a5e09e20 100644 --- a/src/libexpr/include/nix/expr/eval-inline.hh +++ b/src/libexpr/include/nix/expr/eval-inline.hh @@ -5,6 +5,7 @@ #include "nix/expr/eval.hh" #include "nix/expr/eval-error.hh" #include "nix/expr/eval-settings.hh" +#include namespace nix { @@ -91,18 +92,25 @@ void EvalState::forceValue(Value & v, const PosIdx pos) Expr * expr = v.thunk().expr; try { v.mkBlackhole(); - // checkInterrupt(); if (env) [[likely]] expr->eval(*this, *env, v); else ExprBlackHole::throwInfiniteRecursionError(*this, v); } catch (...) { - v.mkThunk(env, expr); - tryFixupBlackHolePos(v, pos); + handleEvalExceptionForThunk(env, expr, v, pos); throw; } - } else if (v.isApp()) - callFunction(*v.app().left, *v.app().right, v, pos); + } else if (v.isApp()) { + Value savedApp = v; + try { + callFunction(*v.app().left, *v.app().right, v, pos); + } catch (...) { + handleEvalExceptionForApp(v, savedApp); + throw; + } + } else if (v.isFailed()) { + handleEvalFailed(v, pos); + } } [[gnu::always_inline]] diff --git a/src/libexpr/include/nix/expr/eval.hh b/src/libexpr/include/nix/expr/eval.hh index 29acb4547aa..3e3d76e2c7f 100644 --- a/src/libexpr/include/nix/expr/eval.hh +++ b/src/libexpr/include/nix/expr/eval.hh @@ -652,8 +652,28 @@ public: */ inline void forceValue(Value & v, const PosIdx pos); +private: + + /** + * Internal support function for forceValue + * + * This code is factored out so that it's not in the heavily inlined hot path. + */ + void handleEvalExceptionForThunk(Env * env, Expr * expr, Value & v, const PosIdx pos); + + /** + * Internal support function for forceValue + * + * This code is factored out so that it's not in the heavily inlined hot path. + */ + void handleEvalExceptionForApp(Value & v, const Value & savedApp); + + void handleEvalFailed(Value & v, PosIdx pos); + void tryFixupBlackHolePos(Value & v, PosIdx pos); +public: + /** * Force a value, then recursively force list elements and * attributes. diff --git a/src/libexpr/include/nix/expr/value.hh b/src/libexpr/include/nix/expr/value.hh index 5b317e7acfc..3a478c4bfd2 100644 --- a/src/libexpr/include/nix/expr/value.hh +++ b/src/libexpr/include/nix/expr/value.hh @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -42,6 +43,7 @@ enum InternalType { tBool, tNull, tFloat, + tFailed, tExternal, tPrimOp, tAttrs, @@ -68,6 +70,7 @@ enum InternalType { */ typedef enum { nThunk, + nFailed, nInt, nFloat, nBool, @@ -424,6 +427,36 @@ struct ValueBase size_t size; Value * const * elems; }; + + struct Failed : gc_cleanup + { + std::exception_ptr ex; + /** + * Optional value for recovering `RecoverableEvalError` + * Must be set iff `ex` is an instance of `RecoverableEvalError`. + */ + Value * recoveryValue; + + Failed(std::exception_ptr ex, Value * recoveryValue) + : ex(ex) + , recoveryValue(recoveryValue) + { + } + + [[noreturn]] void rethrow() const + { + try { + std::rethrow_exception(ex); + } catch (BaseError & e) { + /* Rethrow the copy of the exception - not the original one. + Stack tracing mechanisms rely on being able to modify the exceptions + they catch by reference. */ + e.throwClone(); + } catch (...) { + throw; + } + } + }; }; template @@ -450,6 +483,7 @@ struct PayloadTypeToInternalType MACRO(PrimOp *, primOp, tPrimOp) \ MACRO(ValueBase::PrimOpApplicationThunk, primOpApp, tPrimOpApp) \ MACRO(ExternalValueBase *, external, tExternal) \ + MACRO(ValueBase::Failed *, failed, tFailed) \ MACRO(NixFloat, fpoint, tFloat) #define NIX_VALUE_PAYLOAD_TYPE(T, FIELD_NAME, DISCRIMINATOR) \ @@ -754,6 +788,11 @@ protected: path.path = std::bit_cast(payload[1]); } + void getStorage(Failed *& failed) const noexcept + { + failed = std::bit_cast(payload[1]); + } + void setStorage(NixInt integer) noexcept { setSingleDWordPayload(integer.value); @@ -803,6 +842,11 @@ protected: { setUntaggablePayload(path.accessor, path.path); } + + void setStorage(Failed * failed) noexcept + { + setSingleDWordPayload(std::bit_cast(failed)); + } }; /** @@ -1054,12 +1098,12 @@ public: inline bool isThunk() const { return isa(); - }; + } inline bool isApp() const { return isa(); - }; + } inline bool isBlackhole() const; @@ -1067,17 +1111,22 @@ public: inline bool isLambda() const { return isa(); - }; + } inline bool isPrimOp() const { return isa(); - }; + } inline bool isPrimOpApp() const { return isa(); - }; + } + + inline bool isFailed() const + { + return isa(); + } /** * Returns the normal type of a Value. This only returns nThunk if @@ -1098,6 +1147,7 @@ public: t[tBool] = nBool; t[tNull] = nNull; t[tFloat] = nFloat; + t[tFailed] = nFailed; t[tExternal] = nExternal; t[tAttrs] = nAttrs; t[tPrimOp] = nFunction; @@ -1235,6 +1285,11 @@ public: setStorage(n); } + inline void mkFailed(std::exception_ptr e, Value * recovery) noexcept + { + setStorage(new Value::Failed(e, recovery)); + } + bool isList() const noexcept { return isa(); @@ -1349,6 +1404,13 @@ public: { return getStorage().accessor; } + + Failed & failed() const noexcept + { + auto p = getStorage(); + assert(p); + return *p; + } }; extern ExprBlackHole eBlackHole; diff --git a/src/libexpr/include/nix/expr/value/context.hh b/src/libexpr/include/nix/expr/value/context.hh index 054516bc268..9884c270504 100644 --- a/src/libexpr/include/nix/expr/value/context.hh +++ b/src/libexpr/include/nix/expr/value/context.hh @@ -9,14 +9,14 @@ namespace nix { -class BadNixStringContextElem : public Error +class BadNixStringContextElem final : public CloneableError { public: std::string_view raw; template BadNixStringContextElem(std::string_view raw_, const Args &... args) - : Error("") + : CloneableError("") { raw = raw_; auto hf = HintFmt(args...); diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index b7b80be66a7..0cdf0707559 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -562,6 +562,7 @@ static void prim_typeOf(EvalState & state, const PosIdx pos, Value ** args, Valu v.mkStringNoCopy("float"_sds); break; case nThunk: + case nFailed: unreachable(); } } @@ -1312,11 +1313,12 @@ static void prim_warn(EvalState & state, const PosIdx pos, Value ** args, Value state.forceString(*args[0], pos, "while evaluating the first argument; the message passed to builtins.warn"); { - BaseError msg(std::string{msgStr}); - msg.atPos(state.positions[pos]); - auto info = msg.info(); - info.level = lvlWarn; - info.isFromExpr = true; + ErrorInfo info{ + .level = lvlWarn, + .msg = HintFmt(std::string(msgStr)), + .pos = state.positions[pos], + .isFromExpr = true, + }; logWarning(info); } diff --git a/src/libexpr/print-ambiguous.cc b/src/libexpr/print-ambiguous.cc index cb1a1ef2d3a..ed91cad85a4 100644 --- a/src/libexpr/print-ambiguous.cc +++ b/src/libexpr/print-ambiguous.cc @@ -73,6 +73,11 @@ void printAmbiguous(EvalState & state, Value & v, std::ostream & str, std::set"; + break; case nFunction: if (v.isLambda()) { str << ""; diff --git a/src/libexpr/print.cc b/src/libexpr/print.cc index 066426ec864..f2f62a63698 100644 --- a/src/libexpr/print.cc +++ b/src/libexpr/print.cc @@ -509,6 +509,17 @@ class Printer } } + void printFailed() + { + if (options.ansiColors) + output << ANSI_MAGENTA; + // Historically, a tried and then ignored value (e.g. through tryEval) was + // reverted to the original thunk. + output << "«thunk»"; + if (options.ansiColors) + output << ANSI_NORMAL; + } + void printExternal(Value & v) { v.external()->print(output); @@ -590,6 +601,10 @@ class Printer printFunction(v); break; + case nFailed: + printFailed(); + break; + case nThunk: printThunk(v); break; diff --git a/src/libexpr/value-to-json.cc b/src/libexpr/value-to-json.cc index b2cc482c6e3..4fac29a6671 100644 --- a/src/libexpr/value-to-json.cc +++ b/src/libexpr/value-to-json.cc @@ -98,6 +98,7 @@ json printValueAsJSON( break; case nThunk: + case nFailed: case nFunction: state.error("cannot convert %1% to JSON", showType(v)).atPos(v.determinePos(pos)).debugThrow(); } diff --git a/src/libexpr/value-to-xml.cc b/src/libexpr/value-to-xml.cc index 2fe24857ce5..974927c7e67 100644 --- a/src/libexpr/value-to-xml.cc +++ b/src/libexpr/value-to-xml.cc @@ -171,7 +171,11 @@ static void printValueAsXML( break; case nThunk: + // Historically, a tried and then ignored value (e.g. through tryEval) was + // reverted to the original thunk. + case nFailed: doc.writeEmptyElement("unevaluated"); + break; } } diff --git a/src/libfetchers/git-utils.cc b/src/libfetchers/git-utils.cc index e3bce13b215..d440c3c7076 100644 --- a/src/libfetchers/git-utils.cc +++ b/src/libfetchers/git-utils.cc @@ -74,11 +74,11 @@ namespace nix { struct GitSourceAccessor; -struct GitError : public Error +struct GitError final : public CloneableError { template GitError(const git_error & error, Ts &&... args) - : Error("") + : CloneableError("") { auto hf = HintFmt(std::forward(args)...); err.msg = HintFmt("%1%: %2% (libgit2 error code = %3%)", Uncolored(hf.str()), error.message, error.klass); diff --git a/src/libstore/aws-creds.cc b/src/libstore/aws-creds.cc index 9b0ddefdca8..5baac798ced 100644 --- a/src/libstore/aws-creds.cc +++ b/src/libstore/aws-creds.cc @@ -28,7 +28,7 @@ namespace nix { AwsAuthError::AwsAuthError(int errorCode) - : Error("AWS authentication error: '%s' (%d)", aws_error_str(errorCode), errorCode) + : CloneableError("AWS authentication error: '%s' (%d)", aws_error_str(errorCode), errorCode) , errorCode(errorCode) { } diff --git a/src/libstore/build/goal.cc b/src/libstore/build/goal.cc index d64b869e9e1..ef3501b0091 100644 --- a/src/libstore/build/goal.cc +++ b/src/libstore/build/goal.cc @@ -1,11 +1,11 @@ #include "nix/store/build/goal.hh" #include "nix/store/build/worker.hh" -#include "nix/store/globals.hh" +#include "nix/store/worker-settings.hh" namespace nix { TimedOut::TimedOut(time_t maxDuration) - : BuildError(BuildResult::Failure::TimedOut, "timed out after %1% seconds", maxDuration) + : CloneableError(BuildResult::Failure::TimedOut, "timed out after %1% seconds", maxDuration) , maxDuration(maxDuration) { } diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index b8a9a9aa86e..8e055c14177 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -60,12 +60,12 @@ namespace { using curlSList = std::unique_ptr<::curl_slist, decltype([](::curl_slist * list) { ::curl_slist_free_all(list); })>; using curlMulti = std::unique_ptr<::CURLM, decltype([](::CURLM * multi) { ::curl_multi_cleanup(multi); })>; -struct curlMultiError : Error +struct curlMultiError final : CloneableError { ::CURLMcode code; curlMultiError(::CURLMcode code) - : Error{"unexpected curl multi error: %s", ::curl_multi_strerror(code)} + : CloneableError{"unexpected curl multi error: %s", ::curl_multi_strerror(code)} { assert(code != CURLM_OK); } @@ -1212,7 +1212,7 @@ void FileTransfer::download( template FileTransferError::FileTransferError( FileTransfer::Error error, std::optional response, const Args &... args) - : Error(args...) + : CloneableError(args...) , error(error) , response(response) { diff --git a/src/libstore/include/nix/store/aws-creds.hh b/src/libstore/include/nix/store/aws-creds.hh index 30f6592a06f..0751757cb01 100644 --- a/src/libstore/include/nix/store/aws-creds.hh +++ b/src/libstore/include/nix/store/aws-creds.hh @@ -34,12 +34,13 @@ struct AwsCredentials } }; -class AwsAuthError : public Error +class AwsAuthError final : public CloneableError { std::optional errorCode; public: - using Error::Error; + using CloneableError::CloneableError; + AwsAuthError(int errorCode); std::optional getErrorCode() const diff --git a/src/libstore/include/nix/store/build-result.hh b/src/libstore/include/nix/store/build-result.hh index ce63d099960..c664e6e5b6f 100644 --- a/src/libstore/include/nix/store/build-result.hh +++ b/src/libstore/include/nix/store/build-result.hh @@ -58,7 +58,7 @@ enum struct BuildResultFailureStatus : uint8_t { * This is both an exception type (inherits from Error) and serves as * the failure variant in BuildResult::inner. */ -struct BuildError : public Error +struct BuildError : public CloneableError { using Status = BuildResultFailureStatus; using enum Status; @@ -80,7 +80,7 @@ public: */ template BuildError(Status status, const Args &... args) - : Error(args...) + : CloneableError(args...) , status{status} { } @@ -97,7 +97,7 @@ public: * Also used for deserialization. */ BuildError(Args args) - : Error(std::move(args.msg)) + : CloneableError(std::move(args.msg)) , status{args.status} , isNonDeterministic{args.isNonDeterministic} @@ -108,7 +108,7 @@ public: * Default constructor for deserialization. */ BuildError() - : Error("") + : CloneableError("") { } diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh index a56cbe3c252..3b8f305a70f 100644 --- a/src/libstore/include/nix/store/build/derivation-builder.hh +++ b/src/libstore/include/nix/store/build/derivation-builder.hh @@ -20,14 +20,14 @@ namespace nix { * Denotes a build failure that stemmed from the builder exiting with a * failing exist status. */ -struct BuilderFailureError : BuildError +struct BuilderFailureError final : CloneableError { int builderStatus; std::string extraMsgAfter; BuilderFailureError(BuildResult::Failure::Status status, int builderStatus, std::string extraMsgAfter) - : BuildError{ + : CloneableError{ status, /* No message for now, because the caller will make for us, with extra context */ diff --git a/src/libstore/include/nix/store/build/goal.hh b/src/libstore/include/nix/store/build/goal.hh index 6a92d282898..263647fc126 100644 --- a/src/libstore/include/nix/store/build/goal.hh +++ b/src/libstore/include/nix/store/build/goal.hh @@ -10,7 +10,7 @@ namespace nix { -struct TimedOut : BuildError +struct TimedOut final : CloneableError { time_t maxDuration; diff --git a/src/libstore/include/nix/store/builtins/buildenv.hh b/src/libstore/include/nix/store/builtins/buildenv.hh index c152ab00af5..136a1ab5a77 100644 --- a/src/libstore/include/nix/store/builtins/buildenv.hh +++ b/src/libstore/include/nix/store/builtins/buildenv.hh @@ -22,7 +22,7 @@ struct Package } }; -class BuildEnvFileConflictError : public Error +class BuildEnvFileConflictError final : public CloneableError { public: const Path fileA; @@ -30,7 +30,7 @@ public: int priority; BuildEnvFileConflictError(const Path fileA, const Path fileB, int priority) - : Error( + : CloneableError( "Unable to build profile. There is a conflict for the following files:\n" "\n" " %1%\n" diff --git a/src/libstore/include/nix/store/filetransfer.hh b/src/libstore/include/nix/store/filetransfer.hh index 48c1b5a2f47..e34c3692a27 100644 --- a/src/libstore/include/nix/store/filetransfer.hh +++ b/src/libstore/include/nix/store/filetransfer.hh @@ -403,7 +403,7 @@ ref getFileTransfer(); */ ref makeFileTransfer(const FileTransferSettings & settings = fileTransferSettings); -class FileTransferError : public Error +class FileTransferError final : public CloneableError { public: FileTransfer::Error error; diff --git a/src/libstore/include/nix/store/realisation.hh b/src/libstore/include/nix/store/realisation.hh index 55597acb7c0..670f8ce499e 100644 --- a/src/libstore/include/nix/store/realisation.hh +++ b/src/libstore/include/nix/store/realisation.hh @@ -146,7 +146,7 @@ struct RealisedPath auto operator<=>(const RealisedPath &) const = default; }; -class MissingRealisation : public Error +class MissingRealisation final : public CloneableError { public: MissingRealisation(DrvOutput & outputId) @@ -155,7 +155,7 @@ public: } MissingRealisation(std::string_view drv, OutputName outputName) - : Error( + : CloneableError( "cannot operate on output '%s' of the " "unbuilt derivation '%s'", outputName, diff --git a/src/libstore/include/nix/store/sqlite.hh b/src/libstore/include/nix/store/sqlite.hh index 83839d3852c..be14fde1938 100644 --- a/src/libstore/include/nix/store/sqlite.hh +++ b/src/libstore/include/nix/store/sqlite.hh @@ -166,7 +166,7 @@ struct SQLiteTxn ~SQLiteTxn(); }; -struct SQLiteError : Error +struct SQLiteError : CloneableError { std::string path; std::string errMsg; diff --git a/src/libstore/sqlite.cc b/src/libstore/sqlite.cc index 498e2deda0c..16561f3b2b1 100644 --- a/src/libstore/sqlite.cc +++ b/src/libstore/sqlite.cc @@ -17,7 +17,7 @@ namespace nix { SQLiteError::SQLiteError( const char * path, const char * errMsg, int errNo, int extendedErrNo, int offset, HintFmt && hf) - : Error("") + : CloneableError("") , path(path) , errMsg(errMsg) , errNo(errNo) diff --git a/src/libstore/ssh.cc b/src/libstore/ssh.cc index 1a99083669c..a5014b2c89b 100644 --- a/src/libstore/ssh.cc +++ b/src/libstore/ssh.cc @@ -18,11 +18,11 @@ static std::string parsePublicHostKey(std::string_view host, std::string_view ss } } -class InvalidSSHAuthority : public Error +class InvalidSSHAuthority final : public CloneableError { public: InvalidSSHAuthority(const ParsedURL::Authority & authority, std::string_view reason) - : Error("invalid SSH authority: '%s': %s", authority.to_string(), reason) + : CloneableError("invalid SSH authority: '%s': %s", authority.to_string(), reason) { } }; diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 504063da097..981da9cf2d8 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -55,10 +55,10 @@ namespace nix { -struct NotDeterministic : BuildError +struct NotDeterministic final : CloneableError { NotDeterministic(auto &&... args) - : BuildError(BuildResult::Failure::NotDeterministic, args...) + : CloneableError(BuildResult::Failure::NotDeterministic, args...) { isNonDeterministic = true; } diff --git a/src/libutil-c/nix_api_util.h b/src/libutil-c/nix_api_util.h index d301e5743cf..66ea1752221 100644 --- a/src/libutil-c/nix_api_util.h +++ b/src/libutil-c/nix_api_util.h @@ -109,6 +109,13 @@ enum nix_err { */ NIX_ERR_NIX_ERROR = -4, + /** + * @brief A recoverable error occurred. + * + * This is used primarily by C API *consumers* to communicate that a failed + * primop call should be retried on the next evaluation attempt. + */ + NIX_ERR_RECOVERABLE = -5, }; typedef enum nix_err nix_err; diff --git a/src/libutil/error.cc b/src/libutil/error.cc index c42cbd5a13e..75344dab30f 100644 --- a/src/libutil/error.cc +++ b/src/libutil/error.cc @@ -41,6 +41,11 @@ const std::string & BaseError::calcWhat() const } } +bool BaseError::hasPos() const +{ + return err.pos.get() && *err.pos.get(); +} + std::optional ErrorInfo::programName = std::nullopt; std::ostream & operator<<(std::ostream & os, const HintFmt & hf) diff --git a/src/libutil/experimental-features.cc b/src/libutil/experimental-features.cc index 20d1a082a66..727365833fe 100644 --- a/src/libutil/experimental-features.cc +++ b/src/libutil/experimental-features.cc @@ -378,7 +378,7 @@ std::set parseFeatures(const StringSet & rawFeatures) } MissingExperimentalFeature::MissingExperimentalFeature(ExperimentalFeature feature, std::string reason) - : Error( + : CloneableError( "experimental Nix feature '%1%' is disabled%2%; add '--extra-experimental-features %1%' to enable it", showExperimentalFeature(feature), Uncolored(optionalBracket(" (", reason, ")"))) diff --git a/src/libutil/include/nix/util/error.hh b/src/libutil/include/nix/util/error.hh index d9babf34d1c..ca54523b666 100644 --- a/src/libutil/include/nix/util/error.hh +++ b/src/libutil/include/nix/util/error.hh @@ -191,6 +191,8 @@ public: err.pos = pos; } + bool hasPos() const; + void pushTrace(Trace trace) { err.traces.push_front(trace); @@ -227,13 +229,31 @@ public: { return err; }; + + [[noreturn]] virtual void throwClone() const = 0; +}; + +template +class CloneableError : public Base +{ +public: + using Base::Base; + + /** + * Rethrow a copy of this exception. Useful when the exception can get + * modified when appending traces. + */ + [[noreturn]] void throwClone() const override + { + throw Derived(static_cast(*this)); + } }; -#define MakeError(newClass, superClass) \ - class newClass : public superClass \ - { \ - public: \ - using superClass::superClass; \ +#define MakeError(newClass, superClass) \ + class newClass : public CloneableError \ + { \ + public: \ + using CloneableError::CloneableError; \ } MakeError(Error, BaseError); @@ -245,7 +265,7 @@ MakeError(UnimplementedError, Error); * std::error_code. Use when you want to catch and check an error condition like * no_such_file_or_directory (ENOENT) without ifdefs. */ -class SystemError : public Error +class SystemError : public CloneableError { std::error_code errorCode; std::string errorDetails; @@ -265,7 +285,7 @@ protected: */ template SystemError(Disambig, std::error_code errorCode, std::string_view errorDetails, Args &&... args) - : Error("") + : CloneableError("") , errorCode(errorCode) , errorDetails(errorDetails) { @@ -311,7 +331,7 @@ public: * support is too WIP to justify the code churn, but if it is finished * then a better identifier becomes moe worth it. */ -class SysError : public SystemError +class SysError final : public CloneableError { public: int errNo; @@ -322,7 +342,7 @@ public: */ template SysError(int errNo, Args &&... args) - : SystemError( + : CloneableError( Disambig{}, std::make_error_code(static_cast(errNo)), strerror(errNo), @@ -392,7 +412,7 @@ namespace windows { * Unless you need to catch a specific error number, don't catch this in * portable code. Catch `SystemError` instead. */ -class WinError : public SystemError +class WinError : public CloneableError { public: DWORD lastError; @@ -404,7 +424,7 @@ public: */ template WinError(DWORD lastError, Args &&... args) - : SystemError( + : CloneableError( Disambig{}, std::error_code(lastError, std::system_category()), renderError(lastError), diff --git a/src/libutil/include/nix/util/experimental-features.hh b/src/libutil/include/nix/util/experimental-features.hh index aca14bfbb41..1a4c9b6b574 100644 --- a/src/libutil/include/nix/util/experimental-features.hh +++ b/src/libutil/include/nix/util/experimental-features.hh @@ -80,7 +80,7 @@ std::set parseFeatures(const StringSet &); * An experimental feature was required for some (experimental) * operation, but was not enabled. */ -class MissingExperimentalFeature : public Error +class MissingExperimentalFeature final : public CloneableError { public: /** diff --git a/src/libutil/include/nix/util/processes.hh b/src/libutil/include/nix/util/processes.hh index 44e8c2952d3..3b9cff22ceb 100644 --- a/src/libutil/include/nix/util/processes.hh +++ b/src/libutil/include/nix/util/processes.hh @@ -133,14 +133,14 @@ std::pair runProgram(RunOptions && options); void runProgram2(const RunOptions & options); -class ExecError : public Error +class ExecError final : public CloneableError { public: int status; template ExecError(int status, const Args &... args) - : Error(args...) + : CloneableError(args...) , status(status) { } diff --git a/src/libutil/include/nix/util/source-accessor.hh b/src/libutil/include/nix/util/source-accessor.hh index e1de9ece7f3..3a72f800528 100644 --- a/src/libutil/include/nix/util/source-accessor.hh +++ b/src/libutil/include/nix/util/source-accessor.hh @@ -231,19 +231,19 @@ ref makeEmptySourceAccessor(); */ MakeError(RestrictedPathError, Error); -struct SymlinkNotAllowed : public Error +struct SymlinkNotAllowed final : public CloneableError { CanonPath path; SymlinkNotAllowed(CanonPath path) - : Error("relative path '%s' points to a symlink, which is not allowed", path.rel()) + : CloneableError("relative path '%s' points to a symlink, which is not allowed", path.rel()) , path(std::move(path)) { } template SymlinkNotAllowed(CanonPath path, const std::string & fs, Args &&... args) - : Error(fs, std::forward(args)...) + : CloneableError(fs, std::forward(args)...) , path(std::move(path)) { } diff --git a/tests/functional/lang/eval-fail-memoised-error-trace-not-mutated.err.exp b/tests/functional/lang/eval-fail-memoised-error-trace-not-mutated.err.exp new file mode 100644 index 00000000000..f8807b6bd9a --- /dev/null +++ b/tests/functional/lang/eval-fail-memoised-error-trace-not-mutated.err.exp @@ -0,0 +1,27 @@ +error: + … while calling the 'seq' builtin + at /pwd/lang/eval-fail-memoised-error-trace-not-mutated.nix:10:1: + 9| # a fresh instance of the exceptions to avoid trace mutation. + 10| builtins.seq (builtins.tryEval b) (builtins.seq (builtins.tryEval c) d) + | ^ + 11| + + … while calling the 'seq' builtin + at /pwd/lang/eval-fail-memoised-error-trace-not-mutated.nix:10:36: + 9| # a fresh instance of the exceptions to avoid trace mutation. + 10| builtins.seq (builtins.tryEval b) (builtins.seq (builtins.tryEval c) d) + | ^ + 11| + + … forcing d + + … forcing b + + … while calling the 'throw' builtin + at /pwd/lang/eval-fail-memoised-error-trace-not-mutated.nix:2:7: + 1| let + 2| a = throw "nope"; + | ^ + 3| b = builtins.addErrorContext "forcing b" a; + + error: nope diff --git a/tests/functional/lang/eval-fail-memoised-error-trace-not-mutated.nix b/tests/functional/lang/eval-fail-memoised-error-trace-not-mutated.nix new file mode 100644 index 00000000000..03db62843a3 --- /dev/null +++ b/tests/functional/lang/eval-fail-memoised-error-trace-not-mutated.nix @@ -0,0 +1,10 @@ +let + a = throw "nope"; + b = builtins.addErrorContext "forcing b" a; + c = builtins.addErrorContext "forcing c" a; + d = builtins.addErrorContext "forcing d" a; +in +# Since nix 2.34 errors are memoised. Trying to eval a failed thunk includes +# the trace from when it was first forced. When forcing a failed value it gets +# a fresh instance of the exceptions to avoid trace mutation. +builtins.seq (builtins.tryEval b) (builtins.seq (builtins.tryEval c) d) diff --git a/tests/functional/lang/eval-okay-tryeval-failed-thunk-reeval.err.exp b/tests/functional/lang/eval-okay-tryeval-failed-thunk-reeval.err.exp new file mode 100644 index 00000000000..ba0da519dcb --- /dev/null +++ b/tests/functional/lang/eval-okay-tryeval-failed-thunk-reeval.err.exp @@ -0,0 +1 @@ +trace: throwing diff --git a/tests/functional/lang/eval-okay-tryeval-failed-thunk-reeval.exp b/tests/functional/lang/eval-okay-tryeval-failed-thunk-reeval.exp new file mode 100644 index 00000000000..be54b4b4e39 --- /dev/null +++ b/tests/functional/lang/eval-okay-tryeval-failed-thunk-reeval.exp @@ -0,0 +1 @@ +"done" diff --git a/tests/functional/lang/eval-okay-tryeval-failed-thunk-reeval.nix b/tests/functional/lang/eval-okay-tryeval-failed-thunk-reeval.nix new file mode 100644 index 00000000000..21298619907 --- /dev/null +++ b/tests/functional/lang/eval-okay-tryeval-failed-thunk-reeval.nix @@ -0,0 +1,7 @@ +# Since Nix 2.34, errors are memoized +let + # This attribute value will only be evaluated once. + foo = builtins.trace "throwing" throw "nope"; +in +# Trigger and catch the error twice. +builtins.seq (builtins.tryEval foo).success builtins.seq (builtins.tryEval foo).success "done"