From 4d97e5a38688110a8e4b0c559b020ead9f1688a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Sat, 16 Dec 2023 10:10:49 +0100 Subject: [PATCH] improve infinite recursion errors --- src/nix-eval-jobs.cc | 52 ++++++++++++++++++++++++++++++------------ tests/assets/flake.nix | 18 ++++++++++++++- tests/test_eval.py | 35 ++++++++++++++++++++++++---- 3 files changed, 86 insertions(+), 19 deletions(-) diff --git a/src/nix-eval-jobs.cc b/src/nix-eval-jobs.cc index 7e9cf38..44b88e7 100644 --- a/src/nix-eval-jobs.cc +++ b/src/nix-eval-jobs.cc @@ -104,7 +104,7 @@ struct State { std::exception_ptr exc; }; -void handleBrokenWorkerPipe(Proc &proc) { +void handleBrokenWorkerPipe(Proc &proc, std::string_view msg) { // we already took the process status from Proc, no // need to wait for it again to avoid error messages pid_t pid = proc.pid.release(); @@ -113,27 +113,49 @@ void handleBrokenWorkerPipe(Proc &proc) { int rc = waitpid(pid, &status, WNOHANG); if (rc == 0) { kill(pid, SIGKILL); - throw Error("BUG: worker pipe closed but worker still running?"); + throw Error( + "BUG: while %s, worker pipe got closed but evaluation worker still running?", + msg); } else if (rc == -1) { kill(pid, SIGKILL); - throw Error("BUG: waitpid waiting for worker failed: %s", - strerror(errno)); + throw Error("BUG: while %s, waitpid for evaluation worker failed: %s", + msg, strerror(errno)); } else { if (WIFEXITED(status)) { - throw Error("evaluation worker exited with %d", - WEXITSTATUS(status)); + if (WEXITSTATUS(status) == 1) { + throw Error( + "while %s, evaluation worker exited with exit code 1, " + "(possibly an infinite recursion)", + msg); + } + throw Error("while %s, evaluation worker exited with %d", + msg, WEXITSTATUS(status)); } else if (WIFSIGNALED(status)) { if (WTERMSIG(status) == SIGKILL) { - throw Error("evaluation worker killed by SIGKILL, maybe " - "memory limit reached?"); + throw Error( + "while %s, evaluation worker got killed by SIGKILL, maybe " + "memory limit reached?", + msg); } - throw Error("evaluation worker killed by signal %d (%s)", - WTERMSIG(status), strsignal(WTERMSIG(status))); + throw Error( + "while %s, evaluation worker got killed by signal %d (%s)", + msg, WTERMSIG(status), strsignal(WTERMSIG(status))); } // else ignore WIFSTOPPED and WIFCONTINUED } } } +std::string joinAttrPath(json &attrPath) { + std::string joined; + for (auto& element : attrPath) { + if (!joined.empty()) { + joined += '.'; + } + joined += element.get(); + } + return joined; +} + void collector(Sync &state_, std::condition_variable &wakeup) { try { std::optional> proc_; @@ -151,7 +173,7 @@ void collector(Sync &state_, std::condition_variable &wakeup) { /* Check whether the existing worker process is still there. */ auto s = fromReader->readLine(); if (s.empty()) { - handleBrokenWorkerPipe(*proc.get()); + handleBrokenWorkerPipe(*proc.get(), "checking worker process"); } else if (s == "restart") { proc_ = std::nullopt; fromReader_ = std::nullopt; @@ -176,7 +198,7 @@ void collector(Sync &state_, std::condition_variable &wakeup) { if ((state->todo.empty() && state->active.empty()) || state->exc) { if (tryWriteLine(proc->to.get(), "exit") < 0) { - handleBrokenWorkerPipe(*proc.get()); + handleBrokenWorkerPipe(*proc.get(), "sending exit"); } return; } @@ -191,13 +213,15 @@ void collector(Sync &state_, std::condition_variable &wakeup) { /* Tell the worker to evaluate it. */ if (tryWriteLine(proc->to.get(), "do " + attrPath.dump()) < 0) { - handleBrokenWorkerPipe(*proc.get()); + auto msg = "sending attrPath '" + joinAttrPath(attrPath) + "'"; + handleBrokenWorkerPipe(*proc.get(), msg); } /* Wait for the response. */ auto respString = fromReader->readLine(); if (respString.empty()) { - handleBrokenWorkerPipe(*proc.get()); + auto msg = "reading result for attrPath '" + joinAttrPath(attrPath) + "'"; + handleBrokenWorkerPipe(*proc.get(), msg); } json response; try { diff --git a/tests/assets/flake.nix b/tests/assets/flake.nix index fd0a07f..5a32277 100644 --- a/tests/assets/flake.nix +++ b/tests/assets/flake.nix @@ -8,6 +8,22 @@ { hydraJobs = import ./ci.nix { inherit pkgs; }; - legacyPackages.x86_64-linux.brokenPackage = throw "this is an evaluation error"; + legacyPackages.x86_64-linux = { + brokenPkgs = { + brokenPackage = throw "this is an evaluation error"; + }; + infiniteRecursionPkgs = { + packageWithInfiniteRecursion = + let + recursion = [ recursion ]; + in + derivation { + inherit (pkgs) system; + name = "drvB"; + recursiveAttr = recursion; + builder = ":"; + }; + }; + }; }; } diff --git a/tests/test_eval.py b/tests/test_eval.py index 88e15de..2bc60df 100644 --- a/tests/test_eval.py +++ b/tests/test_eval.py @@ -1,10 +1,10 @@ #!/usr/bin/env python3 -import subprocess import json -from tempfile import TemporaryDirectory +import subprocess from pathlib import Path -from typing import List, Dict, Any +from tempfile import TemporaryDirectory +from typing import Any, Dict, List TEST_ROOT = Path(__file__).parent.resolve() PROJECT_ROOT = TEST_ROOT.parent @@ -77,8 +77,10 @@ def test_eval_error() -> None: "--gc-roots-dir", tempdir, "--meta", + "--workers", + "1", "--flake", - ".#legacyPackages.x86_64-linux", + ".#legacyPackages.x86_64-linux.brokenPkgs", ] res = subprocess.run( cmd, @@ -86,6 +88,31 @@ def test_eval_error() -> None: text=True, stdout=subprocess.PIPE, ) + print(res.stdout) attrs = json.loads(res.stdout) assert attrs["attr"] == "brokenPackage" assert "this is an evaluation error" in attrs["error"] + + +def test_recursion_error() -> None: + with TemporaryDirectory() as tempdir: + cmd = [ + str(BIN), + "--gc-roots-dir", + tempdir, + "--meta", + "--workers", + "1", + "--flake", + ".#legacyPackages.x86_64-linux.infiniteRecursionPkgs", + ] + res = subprocess.run( + cmd, + cwd=TEST_ROOT.joinpath("assets"), + text=True, + stderr=subprocess.PIPE, + ) + assert res.returncode == 1 + print(res.stderr) + assert 'packageWithInfiniteRecursion' in res.stderr + assert "possible infinite recursion" in res.stderr