From b13143280c893205594352e4530c65887c0692b6 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Sun, 7 Sep 2025 11:44:15 +0200 Subject: [PATCH] Introduce a "failed" value type In the multithreaded evaluator, it's possible for multiple threads to wait on the same thunk. If evaluation of the thunk results in an exception, the waiting threads shouldn't try to re-force the thunk. Instead, they should rethrow the same exception, without duplicating any work. Therefore, there is now a new value type `tFailed` that stores an std::exception_ptr. If evaluation of a thunk/app results in an exception, `forceValue()` overwrites the value with a `tFailed`. If `forceValue()` encounters a `tFailed`, it rethrows the exception. So you normally never need to check for failed values (since forcing them causes a rethrow). --- src/libexpr-c/nix_api_value.cc | 2 + src/libexpr-c/nix_api_value.h | 3 +- src/libexpr/eval.cc | 16 ++++++-- src/libexpr/include/nix/expr/eval-inline.hh | 13 ++++-- src/libexpr/include/nix/expr/value.hh | 45 ++++++++++++++++++--- src/libexpr/primops.cc | 1 + src/libexpr/print-ambiguous.cc | 3 ++ src/libexpr/print.cc | 9 +++++ src/libexpr/value-to-json.cc | 1 + src/libexpr/value-to-xml.cc | 5 +++ 10 files changed, 85 insertions(+), 13 deletions(-) diff --git a/src/libexpr-c/nix_api_value.cc b/src/libexpr-c/nix_api_value.cc index 093daf2f8..ba0dfea7f 100644 --- a/src/libexpr-c/nix_api_value.cc +++ b/src/libexpr-c/nix_api_value.cc @@ -177,6 +177,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 7cd6ad180..263ca526e 100644 --- a/src/libexpr-c/nix_api_value.h +++ b/src/libexpr-c/nix_api_value.h @@ -32,7 +32,8 @@ typedef enum { NIX_TYPE_ATTRS, NIX_TYPE_LIST, NIX_TYPE_FUNCTION, - NIX_TYPE_EXTERNAL + NIX_TYPE_EXTERNAL, + NIX_TYPE_FAILED, } ValueType; // forward declarations diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index df4e52e5d..545bb6cd9 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -124,6 +124,8 @@ std::string_view showType(ValueType type, bool withArticle) return WA("a", "float"); case nThunk: return WA("a", "thunk"); + case nFailed: + return WA("a", "failure"); } unreachable(); } @@ -2693,8 +2695,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), @@ -2786,8 +2791,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-inline.hh b/src/libexpr/include/nix/expr/eval-inline.hh index 749e51537..558fb748c 100644 --- a/src/libexpr/include/nix/expr/eval-inline.hh +++ b/src/libexpr/include/nix/expr/eval-inline.hh @@ -97,12 +97,19 @@ void EvalState::forceValue(Value & v, const PosIdx pos) else ExprBlackHole::throwInfiniteRecursionError(*this, v); } catch (...) { - v.mkThunk(env, expr); tryFixupBlackHolePos(v, pos); + v.mkFailed(); throw; } - } else if (v.isApp()) - callFunction(*v.app().left, *v.app().right, v, pos); + } else if (v.isApp()) { + try { + callFunction(*v.app().left, *v.app().right, v, pos); + } catch (...) { + v.mkFailed(); + throw; + } + } else if (v.isFailed()) + std::rethrow_exception(v.failed()->ex); } [[gnu::always_inline]] diff --git a/src/libexpr/include/nix/expr/value.hh b/src/libexpr/include/nix/expr/value.hh index 82db1a775..e11d0b528 100644 --- a/src/libexpr/include/nix/expr/value.hh +++ b/src/libexpr/include/nix/expr/value.hh @@ -35,6 +35,7 @@ typedef enum { tBool, tNull, tFloat, + tFailed, tExternal, tPrimOp, tAttrs, @@ -57,6 +58,7 @@ typedef enum { */ typedef enum { nThunk, + nFailed, nInt, nFloat, nBool, @@ -265,6 +267,11 @@ struct ValueBase size_t size; Value * const * elems; }; + + struct Failed : gc + { + std::exception_ptr ex; + }; }; template @@ -291,6 +298,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) \ @@ -595,6 +603,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); @@ -644,6 +657,11 @@ protected: { setUntaggablePayload(path.accessor, path.path); } + + void setStorage(Failed * failed) noexcept + { + setSingleDWordPayload(std::bit_cast(failed)); + } }; /** @@ -866,12 +884,12 @@ public: inline bool isThunk() const { return isa(); - }; + } inline bool isApp() const { return isa(); - }; + } inline bool isBlackhole() const; @@ -879,17 +897,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 @@ -926,6 +949,8 @@ public: return nExternal; case tFloat: return nFloat; + case tFailed: + return nFailed; case tThunk: case tApp: return nThunk; @@ -1048,6 +1073,11 @@ public: setStorage(n); } + inline void mkFailed() noexcept + { + setStorage(new Value::Failed{.ex = std::current_exception()}); + } + bool isList() const noexcept { return isa(); @@ -1151,6 +1181,11 @@ public: { return getStorage().accessor; } + + Failed * failed() const noexcept + { + return getStorage(); + } }; extern ExprBlackHole eBlackHole; diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 9ba417c32..ec6424f8f 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -515,6 +515,7 @@ static void prim_typeOf(EvalState & state, const PosIdx pos, Value ** args, Valu v.mkStringNoCopy("float"); break; case nThunk: + case nFailed: unreachable(); } } diff --git a/src/libexpr/print-ambiguous.cc b/src/libexpr/print-ambiguous.cc index 8b80e2a66..f164fc46c 100644 --- a/src/libexpr/print-ambiguous.cc +++ b/src/libexpr/print-ambiguous.cc @@ -75,6 +75,9 @@ void printAmbiguous( str << "«potential infinite recursion»"; } break; + case nFailed: + str << "«failed»"; + break; case nFunction: if (v.isLambda()) { str << ""; diff --git a/src/libexpr/print.cc b/src/libexpr/print.cc index 5338e365e..27c951432 100644 --- a/src/libexpr/print.cc +++ b/src/libexpr/print.cc @@ -508,6 +508,11 @@ private: } } + void printFailed(Value & v) + { + output << "«failed»"; + } + void printExternal(Value & v) { v.external()->print(output); @@ -583,6 +588,10 @@ private: printThunk(v); break; + case nFailed: + printFailed(v); + break; + case nExternal: printExternal(v); break; diff --git a/src/libexpr/value-to-json.cc b/src/libexpr/value-to-json.cc index 2cd853f60..458733214 100644 --- a/src/libexpr/value-to-json.cc +++ b/src/libexpr/value-to-json.cc @@ -96,6 +96,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 31400e439..74529d5c0 100644 --- a/src/libexpr/value-to-xml.cc +++ b/src/libexpr/value-to-xml.cc @@ -170,6 +170,11 @@ static void printValueAsXML( case nThunk: doc.writeEmptyElement("unevaluated"); + break; + + case nFailed: + doc.writeEmptyElement("failed"); + break; } }