Ensure that files are parsed/evaluated only once by edolstra · Pull Request #13938 · NixOS/nix

edolstra · 2025-09-08T06:41:45Z

Motivation

Extracted from the multithreaded evaluator.

When doing multithreaded evaluation, we want to ensure that any Nix file is parsed and evaluated only once. The easiest way to do this is to rely on thunks, since those ensure locking in the multithreaded evaluator (not part of this PR). fileEvalCache is now a mapping from SourcePath to a Value *. The value is initially a thunk (pointing to a ExprParseFile helper object) that can be forced to parse and evaluate the file. So a subsequent thread requesting the same file will see a thunk that is possibly locked and wait for it.

The parser cache is gone since it's no longer needed. However, there is a new importResolutionCache that maps SourcePaths to SourcePaths (e.g. /foo to /foo/default.nix). Previously we put multiple entries in fileEvalCache, which was ugly and could result in work duplication.

Context

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

Mic92 · 2025-09-12T09:47:30Z

@edolstra could you rebase?

When doing multithreaded evaluation, we want to ensure that any Nix file is parsed and evaluated only once. The easiest way to do this is to rely on thunks, since those ensure locking in the multithreaded evaluator. `fileEvalCache` is now a mapping from `SourcePath` to a `Value *`. The value is initially a thunk (pointing to a `ExprParseFile` helper object) that can be forced to parse and evaluate the file. So a subsequent thread requesting the same file will see a thunk that is possibly locked and wait for it. The parser cache is gone since it's no longer needed. However, there is a new `importResolutionCache` that maps `SourcePath`s to `SourcePath`s (e.g. `/foo` to `/foo/default.nix`). Previously we put multiple entries in `fileEvalCache`, which was ugly and could result in work duplication.

edolstra · 2025-09-12T15:53:14Z

@Mic92 Done!

xokdvium

Pretty neat refactor. I like this a lot

xokdvium · 2025-09-12T19:34:37Z

src/libexpr/eval.cc

+            vExpr = allocValue();
+            vExpr->mkThunk(&baseEnv, &expr);


Any particular reason this has to be in a callback? Can't we just do this:

Value * vExpr = allocValue(); ExprParseFile expr{*resolvedPath, mustBeTrivial}; vExpr->mkThunk(&baseEnv, &expr);

It avoids an allocation in the case where the fileEvalCache entry already exists.

xokdvium · 2025-09-17T22:42:06Z

src/libexpr/eval.cc

-    if (path != resolvedPath)
-        fileEvalCache.emplace(path, v);
+    Value * vExpr;
+    ExprParseFile expr{*resolvedPath, mustBeTrivial};


I think this is a bug. This expression is allocated on the stack and it the results in a dangling pointer. E.g this leads to a segfault in:

nix eval --expr '(builtins.getFlake "github:nixos/nixpkgs/25.05")' --impure

Ok, maybe there's something else at play here.

xokdvium · 2025-09-17T22:51:05Z

src/libexpr/eval.cc

-    fileParseCache.emplace(resolvedPath, e);
+void EvalState::evalFile(const SourcePath & path, Value & v, bool mustBeTrivial)
+{
+    auto resolvedPath = getConcurrent(*importResolutionCache, path);


This leads to a dangling reference, because this variable is allocated on the stack, but it's later saved to the heap allocated in *vExpr.

This has multiple dangling pointer issues that lead to segfaults in e.g.: nix eval --expr '(builtins.getFlake "github:nixos/nixpkgs/25.05")' --impure This reverts commit ad17572, reversing changes made to d314750.

Copilot

Pull Request Overview

This PR refactors the file evaluation caching mechanism to ensure that Nix files are parsed and evaluated only once, particularly in preparation for multithreaded evaluation. The implementation moves from a parser cache approach to a thunk-based system where files are represented as lazy-evaluated expressions.

Key changes:

Replace separate parser and evaluation caches with a unified thunk-based file evaluation cache
Introduce import resolution caching to map source paths to resolved paths
Simplify hash template specializations by removing explicit hash type parameters

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/libutil/posix-source-accessor.cc	Refactor cache lookup to use new `getConcurrent` utility function
src/libutil/include/nix/util/util.hh	Add `getConcurrent` helper for concurrent flat map lookups
src/libutil/include/nix/util/source-path.hh	Restructure hash implementation with Boost compatibility
src/libutil/include/nix/util/canon-path.hh	Restructure hash implementation with Boost compatibility
src/libfetchers/include/nix/fetchers/filtering-source-accessor.hh	Remove explicit hash type parameter from unordered sets
src/libfetchers/git-utils.cc	Remove explicit hash type parameter from unordered maps
src/libfetchers/filtering-source-accessor.cc	Remove explicit hash type parameter from unordered sets
src/libexpr/include/nix/expr/eval.hh	Replace parser cache with import resolution cache and refactor file evaluation cache
src/libexpr/eval.cc	Implement new thunk-based file evaluation system with `ExprParseFile` helper

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-17T22:56:14Z

src/libexpr/eval.cc

+    ExprParseFile expr{*resolvedPath, mustBeTrivial};
+
+    fileEvalCache->try_emplace_and_cvisit(
+        *resolvedPath,
+        nullptr,
+        [&](auto & i) {
+            vExpr = allocValue();
+            vExpr->mkThunk(&baseEnv, &expr);


The ExprParseFile object is created on the stack but its address is passed to vExpr->mkThunk(). This creates a dangling reference when the stack object goes out of scope. The expression should be allocated on the heap or managed differently to ensure it remains valid for the lifetime of the thunk.

Suggested change

ExprParseFile expr{*resolvedPath, mustBeTrivial};

fileEvalCache->try_emplace_and_cvisit(

*resolvedPath,

nullptr,

[&](auto & i) {

vExpr = allocValue();

vExpr->mkThunk(&baseEnv, &expr);

auto expr = new ExprParseFile{*resolvedPath, mustBeTrivial};

fileEvalCache->try_emplace_and_cvisit(

*resolvedPath,

nullptr,

[&](auto & i) {

vExpr = allocValue();

vExpr->mkThunk(&baseEnv, expr);

Copilot · 2025-09-17T22:56:14Z

src/libexpr/eval.cc

+    SourcePath & path;
+    bool mustBeTrivial;

-    auto resolvedPath = resolveExprPath(path);
-    if ((i = fileEvalCache.find(resolvedPath)) != fileEvalCache.end()) {
-        v = i->second;
-        return;
+    ExprParseFile(SourcePath & path, bool mustBeTrivial)


Storing a reference to SourcePath in ExprParseFile is unsafe when the referenced object's lifetime is not guaranteed. Consider storing by value instead to avoid potential dangling references.

xokdvium · 2025-09-17T22:58:05Z

Revert in #14013.

Revert "Merge pull request #13938 from NixOS/import-thunk"

This reverts commit fd03481.

edolstra added 6 commits September 12, 2025 16:49

Add getConcurrent helper function

7f9b522

CanonPath: Implement boost::hash

8fbf4b9

SourcePath: Implement boost::hash

47c16fc

Remove some unnecessary hash template arguments

4b63ff6

Use concurrent_flat_map_fwd.hpp

8d8f49c

edolstra force-pushed the import-thunk branch from 947a480 to 8d8f49c Compare September 12, 2025 15:51

github-actions bot added the fetching Networking with the outside (non-Nix) world, input locking label Sep 12, 2025

xokdvium approved these changes Sep 12, 2025

View reviewed changes

xokdvium reviewed Sep 12, 2025

View reviewed changes

edolstra merged commit ad17572 into master Sep 15, 2025
29 checks passed

edolstra deleted the import-thunk branch September 15, 2025 17:03

xokdvium reviewed Sep 17, 2025

View reviewed changes

Mic92 requested a review from Copilot September 17, 2025 22:55

Copilot AI reviewed Sep 17, 2025

View reviewed changes

Mic92 added a commit that referenced this pull request Sep 17, 2025

Merge pull request #14013 from xokdvium/revert-13938

9d8c6a6

Revert "Merge pull request #13938 from NixOS/import-thunk"

edolstra added a commit that referenced this pull request Sep 22, 2025

Reapply "Merge pull request #13938 from NixOS/import-thunk"

5f60602

This reverts commit fd03481.

edolstra mentioned this pull request Sep 22, 2025

Ensure that files are parsed/evaluated only once (2nd attempt) #14040

Merged

Uh oh!

Conversation

edolstra commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Context

Uh oh!

Mic92 commented Sep 12, 2025

Uh oh!

edolstra commented Sep 12, 2025

Uh oh!

xokdvium left a comment

Choose a reason for hiding this comment

Uh oh!

xokdvium Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

edolstra Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xokdvium Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

xokdvium Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

xokdvium Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

xokdvium commented Sep 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

edolstra commented Sep 8, 2025 •

edited

Loading