From 5e9db0976158e6990c99fe3cb1b2ec0bd41d7d28 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Tue, 13 Aug 2024 15:34:55 +0200 Subject: [PATCH] libstore: downsize hook pipes don't keep fds open we're not using. currently this does not cause any problems, but it does increase the size of our fd table needlessly and in the future, when we have proper async processing, having builderOut open in the daemon once the hook has been fully started is problematic Change-Id: I6e7fb773b280b042873103638d3e04272ca1e4fc --- src/libstore/build/derivation-goal.cc | 20 ++++++++++---------- src/libstore/build/hook-instance.cc | 26 +++++++++++++++----------- src/libstore/build/hook-instance.hh | 8 ++++---- 3 files changed, 29 insertions(+), 25 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 2781929e7..b7f8005ae 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -822,8 +822,8 @@ int DerivationGoal::getChildStatus() void DerivationGoal::closeReadPipes() { - hook->builderOut.readSide.reset(); - hook->fromHook.readSide.reset(); + hook->builderOut.reset(); + hook->fromHook.reset(); builderOutFD = nullptr; } @@ -1135,7 +1135,7 @@ HookReply DerivationGoal::tryBuildHook(bool inBuildSlot) while (true) { auto s = [&]() { try { - return readLine(worker.hook.instance->fromHook.readSide.get()); + return readLine(worker.hook.instance->fromHook.get()); } catch (Error & e) { e.addTrace({}, "while reading the response from the build hook"); throw; @@ -1171,7 +1171,7 @@ HookReply DerivationGoal::tryBuildHook(bool inBuildSlot) if (e.errNo == EPIPE) { printError( "build hook died unexpectedly: %s", - chomp(drainFD(worker.hook.instance->fromHook.readSide.get()))); + chomp(drainFD(worker.hook.instance->fromHook.get()))); worker.hook.instance.reset(); return rpDecline; } else @@ -1181,7 +1181,7 @@ HookReply DerivationGoal::tryBuildHook(bool inBuildSlot) hook = std::move(worker.hook.instance); try { - machineName = readLine(hook->fromHook.readSide.get()); + machineName = readLine(hook->fromHook.get()); } catch (Error & e) { e.addTrace({}, "while reading the machine name from the build hook"); throw; @@ -1204,15 +1204,15 @@ HookReply DerivationGoal::tryBuildHook(bool inBuildSlot) } hook->sink = FdSink(); - hook->toHook.writeSide.reset(); + hook->toHook.reset(); /* Create the log file and pipe. */ Path logFile = openLogFile(); std::set fds; - fds.insert(hook->fromHook.readSide.get()); - fds.insert(hook->builderOut.readSide.get()); - builderOutFD = &hook->builderOut.readSide; + fds.insert(hook->fromHook.get()); + fds.insert(hook->builderOut.get()); + builderOutFD = &hook->builderOut; worker.childStarted(shared_from_this(), fds, false); return rpAccept; @@ -1309,7 +1309,7 @@ Goal::WorkResult DerivationGoal::handleChildOutput(int fd, std::string_view data return StillAlive{}; } - if (hook && fd == hook->fromHook.readSide.get()) { + if (hook && fd == hook->fromHook.get()) { for (auto c : data) if (c == '\n') { auto json = parseJSONMessage(currentHookLine); diff --git a/src/libstore/build/hook-instance.cc b/src/libstore/build/hook-instance.cc index 9a93646f4..d5da80c74 100644 --- a/src/libstore/build/hook-instance.cc +++ b/src/libstore/build/hook-instance.cc @@ -26,18 +26,21 @@ HookInstance::HookInstance() args.push_back(std::to_string(verbosity)); /* Create a pipe to get the output of the child. */ - fromHook.create(); + Pipe fromHook_; + fromHook_.create(); /* Create the communication pipes. */ - toHook.create(); + Pipe toHook_; + toHook_.create(); /* Create a pipe to get the output of the builder. */ - builderOut.create(); + Pipe builderOut_; + builderOut_.create(); /* Fork the hook. */ pid = startProcess([&]() { - if (dup2(fromHook.writeSide.get(), STDERR_FILENO) == -1) + if (dup2(fromHook_.writeSide.get(), STDERR_FILENO) == -1) throw SysError("cannot pipe standard error into log file"); commonChildInit(); @@ -45,16 +48,16 @@ HookInstance::HookInstance() if (chdir("/") == -1) throw SysError("changing into /"); /* Dup the communication pipes. */ - if (dup2(toHook.readSide.get(), STDIN_FILENO) == -1) + if (dup2(toHook_.readSide.get(), STDIN_FILENO) == -1) throw SysError("dupping to-hook read side"); /* Use fd 4 for the builder's stdout/stderr. */ - if (dup2(builderOut.writeSide.get(), 4) == -1) + if (dup2(builderOut_.writeSide.get(), 4) == -1) throw SysError("dupping builder's stdout/stderr"); /* Hack: pass the read side of that fd to allow build-remote to read SSH error messages. */ - if (dup2(builderOut.readSide.get(), 5) == -1) + if (dup2(builderOut_.readSide.get(), 5) == -1) throw SysError("dupping builder's stdout/stderr"); execv(buildHook.c_str(), stringsToCharPtrs(args).data()); @@ -63,10 +66,11 @@ HookInstance::HookInstance() }); pid.setSeparatePG(true); - fromHook.writeSide.reset(); - toHook.readSide.reset(); + fromHook = std::move(fromHook_.readSide); + toHook = std::move(toHook_.writeSide); + builderOut = std::move(builderOut_.readSide); - sink = FdSink(toHook.writeSide.get()); + sink = FdSink(toHook.get()); std::map settings; globalConfig.getSettings(settings); for (auto & setting : settings) @@ -78,7 +82,7 @@ HookInstance::HookInstance() HookInstance::~HookInstance() { try { - toHook.writeSide.reset(); + toHook.reset(); if (pid) pid.kill(); } catch (...) { ignoreException(); diff --git a/src/libstore/build/hook-instance.hh b/src/libstore/build/hook-instance.hh index 481158296..b8632d299 100644 --- a/src/libstore/build/hook-instance.hh +++ b/src/libstore/build/hook-instance.hh @@ -10,19 +10,19 @@ namespace nix { struct HookInstance { /** - * Pipes for talking to the build hook. + * Pipe for talking to the build hook. */ - Pipe toHook; + AutoCloseFD toHook; /** * Pipe for the hook's standard output/error. */ - Pipe fromHook; + AutoCloseFD fromHook; /** * Pipe for the builder's standard output/error. */ - Pipe builderOut; + AutoCloseFD builderOut; /** * The process ID of the hook.