From ee0c195eba7d16b796fd9883e3fe88c0d64ff0bf Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 1 Oct 2024 15:30:30 -0700 Subject: [PATCH] Split ignoreException to avoid suppressing CTRL-C This splits `ignoreException` into `ignoreExceptionExceptInterrupt` (which ignores all exceptions except `Interrupt`, which indicates a SIGINT/CTRL-C) and `ignoreExceptionInDestructor` (which ignores all exceptions, so that destructors do not throw exceptions). This prevents many cases where Nix ignores CTRL-C entirely. See: https://github.com/NixOS/nix/issues/7245 Upstream-PR: https://github.com/NixOS/nix/pull/11618 Change-Id: Ie7d2467eedbe840d1b9fa2e88a4e88e4ab26a87b --- src/libexpr/eval-cache.cc | 6 +++--- src/libmain/shared.cc | 2 +- src/libstore/build/derivation-goal.cc | 4 ++-- src/libstore/build/hook-instance.cc | 3 ++- src/libstore/build/local-derivation-goal.cc | 9 +++++---- src/libstore/build/substitution-goal.cc | 2 +- src/libstore/filetransfer.cc | 2 +- src/libstore/gc.cc | 4 ++-- src/libstore/local-store.cc | 4 ++-- src/libstore/optimise-store.cc | 2 +- src/libstore/pathlocks.cc | 2 +- src/libstore/pathlocks.hh | 3 ++- src/libstore/remote-fs-accessor.cc | 4 ++-- src/libstore/remote-store.cc | 5 +++-- src/libstore/sqlite.cc | 6 +++--- src/libstore/store-api.cc | 2 +- src/libutil/current-process.cc | 2 +- src/libutil/error.cc | 14 +++++++++++++- src/libutil/error.hh | 17 ++++++++++++++++- src/libutil/file-descriptor.cc | 2 +- src/libutil/file-system.cc | 2 +- src/libutil/logging.cc | 2 +- src/libutil/serialise.cc | 2 +- src/libutil/serialise.hh | 4 ++-- src/libutil/signals.cc | 2 +- src/libutil/thread-pool.cc | 5 ++--- src/nix/develop.cc | 2 +- src/nix/flake.cc | 5 ++++- 28 files changed, 76 insertions(+), 43 deletions(-) diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index 83bfd4fb0..e7336c7e8 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -79,7 +79,7 @@ struct AttrDb state->txn->commit(); state->txn.reset(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } @@ -90,7 +90,7 @@ struct AttrDb try { return fun(); } catch (SQLiteError &) { - ignoreException(); + ignoreExceptionExceptInterrupt(); failed = true; return 0; } @@ -329,7 +329,7 @@ static std::shared_ptr makeAttrDb( try { return std::make_shared(cfg, fingerprint, symbols); } catch (SQLiteError &) { - ignoreException(); + ignoreExceptionExceptInterrupt(); return nullptr; } } diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index 64bd00606..029b457b1 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -395,7 +395,7 @@ RunPager::~RunPager() pid.wait(); } } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 5c0452391..fe2bb0145 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -107,7 +107,7 @@ DerivationGoal::~DerivationGoal() noexcept(false) { /* Careful: we should never ever throw an exception from a destructor. */ - try { closeLogFile(); } catch (...) { ignoreException(); } + try { closeLogFile(); } catch (...) { ignoreExceptionInDestructor(); } } @@ -858,7 +858,7 @@ void replaceValidPath(const Path & storePath, const Path & tmpPath) // attempt to recover movePath(oldPath, storePath); } catch (...) { - ignoreException(); + ignoreExceptionExceptInterrupt(); } throw; } diff --git a/src/libstore/build/hook-instance.cc b/src/libstore/build/hook-instance.cc index f91a904cc..521f34917 100644 --- a/src/libstore/build/hook-instance.cc +++ b/src/libstore/build/hook-instance.cc @@ -1,4 +1,5 @@ #include "child.hh" +#include "error.hh" #include "file-system.hh" #include "globals.hh" #include "hook-instance.hh" @@ -86,7 +87,7 @@ HookInstance::~HookInstance() toHook.reset(); if (pid) pid.kill(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index f3d0bc8b4..5c1fcf1f8 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -1,4 +1,5 @@ #include "local-derivation-goal.hh" +#include "error.hh" #include "indirect-root-store.hh" #include "machines.hh" #include "store-api.hh" @@ -98,9 +99,9 @@ LocalDerivationGoal::~LocalDerivationGoal() noexcept(false) { /* Careful: we should never ever throw an exception from a destructor. */ - try { deleteTmpDir(false); } catch (...) { ignoreException(); } - try { killChild(); } catch (...) { ignoreException(); } - try { stopDaemon(); } catch (...) { ignoreException(); } + try { deleteTmpDir(false); } catch (...) { ignoreExceptionInDestructor(); } + try { killChild(); } catch (...) { ignoreExceptionInDestructor(); } + try { stopDaemon(); } catch (...) { ignoreExceptionInDestructor(); } } @@ -1249,7 +1250,7 @@ void LocalDerivationGoal::startDaemon() NotTrusted, daemon::Recursive); debug("terminated daemon connection"); } catch (SysError &) { - ignoreException(); + ignoreExceptionExceptInterrupt(); } }); diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index 8088bf668..6dddf6e40 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -291,7 +291,7 @@ void PathSubstitutionGoal::cleanup() thr.get(); } } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 10c810e49..6fe1f9a05 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -115,7 +115,7 @@ struct curlFileTransfer : public FileTransfer if (!done) fail(FileTransferError(Interrupted, {}, "download of '%s' was interrupted", request.uri)); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index d5903d01e..99bf80994 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -923,8 +923,8 @@ void LocalStore::autoGC(bool sync) } catch (...) { // FIXME: we could propagate the exception to the - // future, but we don't really care. - ignoreException(); + // future, but we don't really care. (what??) + ignoreExceptionInDestructor(); } }).detach(); diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 1af0f54de..c3248c2c3 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -481,7 +481,7 @@ LocalStore::~LocalStore() unlink(fnTempRoots.c_str()); } } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } @@ -1222,7 +1222,7 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source, try { parseDump(sink, source); } catch (...) { - ignoreException(); + ignoreExceptionExceptInterrupt(); } } }; diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index c60e5a85d..14381b6e0 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -31,7 +31,7 @@ struct MakeReadOnly /* This will make the path read-only. */ if (path != "") canonicaliseTimestampAndPermissions(path); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } }; diff --git a/src/libstore/pathlocks.cc b/src/libstore/pathlocks.cc index ced0f30bb..3225857ec 100644 --- a/src/libstore/pathlocks.cc +++ b/src/libstore/pathlocks.cc @@ -145,7 +145,7 @@ PathLocks::~PathLocks() try { unlock(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libstore/pathlocks.hh b/src/libstore/pathlocks.hh index d06d031b5..feb0f5548 100644 --- a/src/libstore/pathlocks.hh +++ b/src/libstore/pathlocks.hh @@ -1,6 +1,7 @@ #pragma once ///@file +#include "error.hh" #include "file-descriptor.hh" namespace nix { @@ -53,7 +54,7 @@ struct FdLock if (acquired) lockFile(fd, ltNone, false); } catch (SysError &) { - ignoreException(); + ignoreExceptionInDestructor(); } } }; diff --git a/src/libstore/remote-fs-accessor.cc b/src/libstore/remote-fs-accessor.cc index 0689ce74d..59d267873 100644 --- a/src/libstore/remote-fs-accessor.cc +++ b/src/libstore/remote-fs-accessor.cc @@ -29,7 +29,7 @@ ref RemoteFSAccessor::addToCache(std::string_view hashPart, std::str /* FIXME: do this asynchronously. */ writeFile(makeCacheFile(hashPart, "nar"), nar); } catch (...) { - ignoreException(); + ignoreExceptionExceptInterrupt(); } } @@ -41,7 +41,7 @@ ref RemoteFSAccessor::addToCache(std::string_view hashPart, std::str nlohmann::json j = listNar(narAccessor, "", true); writeFile(makeCacheFile(hashPart, "ls"), j.dump()); } catch (...) { - ignoreException(); + ignoreExceptionExceptInterrupt(); } } diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 1f94ca03f..a9f9818be 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -1,3 +1,4 @@ +#include "error.hh" #include "serialise.hh" #include "signals.hh" #include "path-with-outputs.hh" @@ -855,7 +856,7 @@ RemoteStore::Connection::~Connection() try { to.flush(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } @@ -985,7 +986,7 @@ void RemoteStore::ConnectionHandle::withFramedSink(std::function copyPaths( // not be within our control to change that, and we might still want // to at least copy the output paths. if (e.missingFeature == Xp::CaDerivations) - ignoreException(); + ignoreExceptionExceptInterrupt(); else throw; } diff --git a/src/libutil/current-process.cc b/src/libutil/current-process.cc index 33cda211b..3b3e46a9a 100644 --- a/src/libutil/current-process.cc +++ b/src/libutil/current-process.cc @@ -49,7 +49,7 @@ unsigned int getMaxCPU() auto period = cpuMaxParts[1]; if (quota != "max") return std::ceil(std::stoi(quota) / std::stof(period)); - } catch (Error &) { ignoreException(lvlDebug); } + } catch (Error &) { ignoreExceptionInDestructor(lvlDebug); } #endif return 0; diff --git a/src/libutil/error.cc b/src/libutil/error.cc index a7cbfbfd0..027f0b1a5 100644 --- a/src/libutil/error.cc +++ b/src/libutil/error.cc @@ -4,6 +4,7 @@ #include "position.hh" #include "terminal.hh" #include "strings.hh" +#include "signals.hh" #include #include @@ -416,7 +417,7 @@ std::ostream & showErrorInfo(std::ostream & out, const ErrorInfo & einfo, bool s return out; } -void ignoreException(Verbosity lvl) +void ignoreExceptionInDestructor(Verbosity lvl) { /* Make sure no exceptions leave this function. printError() also throws when remote is closed. */ @@ -429,4 +430,15 @@ void ignoreException(Verbosity lvl) } catch (...) { } } +void ignoreExceptionExceptInterrupt(Verbosity lvl) +{ + try { + throw; + } catch (const Interrupted & e) { + throw; + } catch (std::exception & e) { + printMsg(lvl, "error (ignored): %1%", e.what()); + } +} + } diff --git a/src/libutil/error.hh b/src/libutil/error.hh index 73c1ccadd..4eff2c2bc 100644 --- a/src/libutil/error.hh +++ b/src/libutil/error.hh @@ -204,7 +204,22 @@ public: /** * Exception handling in destructors: print an error message, then * ignore the exception. + * + * If you're not in a destructor, you usually want to use `ignoreExceptionExceptInterrupt()`. + * + * This function might also be used in callbacks whose caller may not handle exceptions, + * but ideally we propagate the exception using an exception_ptr in such cases. + * See e.g. `PackBuilderContext` */ -void ignoreException(Verbosity lvl = lvlError); +void ignoreExceptionInDestructor(Verbosity lvl = lvlError); + +/** + * Not destructor-safe. + * Print an error message, then ignore the exception. + * If the exception is an `Interrupted` exception, rethrow it. + * + * This may be used in a few places where Interrupt can't happen, but that's ok. + */ +void ignoreExceptionExceptInterrupt(Verbosity lvl = lvlError); } diff --git a/src/libutil/file-descriptor.cc b/src/libutil/file-descriptor.cc index 8385ea402..cbb2bb539 100644 --- a/src/libutil/file-descriptor.cc +++ b/src/libutil/file-descriptor.cc @@ -146,7 +146,7 @@ AutoCloseFD::~AutoCloseFD() try { close(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index 1d3eba58f..c4ffb1d0c 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -522,7 +522,7 @@ AutoDelete::~AutoDelete() } } } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libutil/logging.cc b/src/libutil/logging.cc index 7d9482814..7609e6e39 100644 --- a/src/libutil/logging.cc +++ b/src/libutil/logging.cc @@ -352,7 +352,7 @@ Activity::~Activity() try { logger.stopActivity(id); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libutil/serialise.cc b/src/libutil/serialise.cc index f509fedff..2f5a11a28 100644 --- a/src/libutil/serialise.cc +++ b/src/libutil/serialise.cc @@ -83,7 +83,7 @@ void BufferedSink::flush() FdSink::~FdSink() { - try { flush(); } catch (...) { ignoreException(); } + try { flush(); } catch (...) { ignoreExceptionInDestructor(); } } diff --git a/src/libutil/serialise.hh b/src/libutil/serialise.hh index 3a9685e0e..08ea9a135 100644 --- a/src/libutil/serialise.hh +++ b/src/libutil/serialise.hh @@ -549,7 +549,7 @@ struct FramedSource : Source } } } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } @@ -595,7 +595,7 @@ struct FramedSink : nix::BufferedSink to << 0; to.flush(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libutil/signals.cc b/src/libutil/signals.cc index 04a697d01..4e9ed0ba1 100644 --- a/src/libutil/signals.cc +++ b/src/libutil/signals.cc @@ -78,7 +78,7 @@ void triggerInterrupt() try { callback(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } } diff --git a/src/libutil/thread-pool.cc b/src/libutil/thread-pool.cc index 0ff83e997..cd380b608 100644 --- a/src/libutil/thread-pool.cc +++ b/src/libutil/thread-pool.cc @@ -109,9 +109,8 @@ void ThreadPool::doWork(bool mainThread) try { std::rethrow_exception(exc); } catch (std::exception & e) { - if (!dynamic_cast(&e) && - !dynamic_cast(&e)) - ignoreException(); + if (!dynamic_cast(&e)) + ignoreExceptionExceptInterrupt(); } catch (...) { } } diff --git a/src/nix/develop.cc b/src/nix/develop.cc index fb144c904..d1615ecdc 100644 --- a/src/nix/develop.cc +++ b/src/nix/develop.cc @@ -639,7 +639,7 @@ struct CmdDevelop : Common, MixEnvironment throw Error("package 'nixpkgs#bashInteractive' does not provide a 'bin/bash'"); } catch (Error &) { - ignoreException(); + ignoreExceptionExceptInterrupt(); } // Override SHELL with the one chosen for this environment. diff --git a/src/nix/flake.cc b/src/nix/flake.cc index 15c393c90..0c704a995 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -16,6 +16,7 @@ #include "eval-cache.hh" #include "markdown.hh" #include "terminal.hh" +#include "signals.hh" #include #include @@ -367,9 +368,11 @@ struct CmdFlakeCheck : FlakeCommand auto reportError = [&](const Error & e) { try { throw e; + } catch (Interrupted & e) { + throw; } catch (Error & e) { if (settings.keepGoing) { - ignoreException(); + ignoreExceptionExceptInterrupt(); hasErrors = true; } else