From 3df619339c012eac62379191ebe0891fdc93a916 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 30 Sep 2024 11:49:53 +0200 Subject: [PATCH] Split ignoreException for destructors or interrupt-safe --- src/libexpr/eval-cache.cc | 6 +- src/libmain/shared.cc | 2 +- src/libstore/build/derivation-goal.cc | 4 +- src/libstore/build/substitution-goal.cc | 2 +- src/libstore/filetransfer.cc | 2 +- src/libstore/gc.cc | 4 +- src/libstore/local-store.cc | 174 +++++++++--------- src/libstore/optimise-store.cc | 2 +- src/libstore/pathlocks.cc | 2 +- src/libstore/remote-fs-accessor.cc | 4 +- src/libstore/sqlite.cc | 6 +- src/libstore/store-api.cc | 2 +- src/libstore/unix/build/hook-instance.cc | 2 +- .../unix/build/local-derivation-goal.cc | 8 +- src/libstore/worker-protocol-connection.cc | 2 +- src/libutil/current-process.cc | 2 +- src/libutil/file-descriptor.cc | 3 +- src/libutil/file-system.cc | 3 +- src/libutil/logging.cc | 2 +- src/libutil/serialise.cc | 3 +- src/libutil/serialise.hh | 4 +- src/libutil/thread-pool.cc | 5 +- src/libutil/unix/signals.cc | 2 +- src/libutil/util.cc | 14 +- src/libutil/util.hh | 23 ++- src/nix/develop.cc | 2 +- src/nix/flake.cc | 4 +- 27 files changed, 164 insertions(+), 125 deletions(-) diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index c407cc89a..ea3319f99 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -101,7 +101,7 @@ struct AttrDb state->txn->commit(); state->txn.reset(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } @@ -112,7 +112,7 @@ struct AttrDb try { return fun(); } catch (SQLiteError &) { - ignoreException(); + ignoreExceptionExceptInterrupt(); failed = true; return 0; } @@ -351,7 +351,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 a224f8d92..50f90bfb3 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -416,7 +416,7 @@ RunPager::~RunPager() } #endif } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index b809e3ffe..34ed16a38 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -90,7 +90,7 @@ DerivationGoal::~DerivationGoal() { /* Careful: we should never ever throw an exception from a destructor. */ - try { closeLogFile(); } catch (...) { ignoreException(); } + try { closeLogFile(); } catch (...) { ignoreExceptionInDestructor(); } } @@ -814,7 +814,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/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index a26eea820..315500719 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -294,7 +294,7 @@ void PathSubstitutionGoal::cleanup() outPipe.close(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 58c52acc0..154ec6007 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -139,7 +139,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 91cf76366..73195794a 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -958,8 +958,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 a8c27b92f..394ee2cc1 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -522,7 +522,7 @@ LocalStore::~LocalStore() unlink(fnTempRoots.c_str()); } } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } @@ -1096,108 +1096,114 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source, if (checkSigs && pathInfoIsUntrusted(info)) throw Error("cannot add path '%s' because it lacks a signature by a trusted key", printStorePath(info.path)); - /* In case we are not interested in reading the NAR: discard it. */ - bool narRead = false; - Finally cleanup = [&]() { - if (!narRead) { - NullFileSystemObjectSink sink; - try { - parseDump(sink, source); - } catch (...) { - ignoreException(); + { + /* In case we are not interested in reading the NAR: discard it. */ + bool narRead = false; + Finally cleanup = [&]() { + if (!narRead) { + NullFileSystemObjectSink sink; + try { + parseDump(sink, source); + } catch (...) { + // TODO: should Interrupted be handled here? + ignoreExceptionInDestructor(); + } } - } - }; + }; - addTempRoot(info.path); - - if (repair || !isValidPath(info.path)) { - - PathLocks outputLock; - - auto realPath = Store::toRealPath(info.path); - - /* Lock the output path. But don't lock if we're being called - from a build hook (whose parent process already acquired a - lock on this path). */ - if (!locksHeld.count(printStorePath(info.path))) - outputLock.lockPaths({realPath}); + addTempRoot(info.path); if (repair || !isValidPath(info.path)) { - deletePath(realPath); + PathLocks outputLock; - /* While restoring the path from the NAR, compute the hash - of the NAR. */ - HashSink hashSink(HashAlgorithm::SHA256); + auto realPath = Store::toRealPath(info.path); - TeeSource wrapperSource { source, hashSink }; + /* Lock the output path. But don't lock if we're being called + from a build hook (whose parent process already acquired a + lock on this path). */ + if (!locksHeld.count(printStorePath(info.path))) + outputLock.lockPaths({realPath}); - narRead = true; - restorePath(realPath, wrapperSource, settings.fsyncStorePaths); + if (repair || !isValidPath(info.path)) { - auto hashResult = hashSink.finish(); + deletePath(realPath); - if (hashResult.first != info.narHash) - throw Error("hash mismatch importing path '%s';\n specified: %s\n got: %s", - printStorePath(info.path), info.narHash.to_string(HashFormat::Nix32, true), hashResult.first.to_string(HashFormat::Nix32, true)); + /* While restoring the path from the NAR, compute the hash + of the NAR. */ + HashSink hashSink(HashAlgorithm::SHA256); - if (hashResult.second != info.narSize) - throw Error("size mismatch importing path '%s';\n specified: %s\n got: %s", - printStorePath(info.path), info.narSize, hashResult.second); + TeeSource wrapperSource { source, hashSink }; - if (info.ca) { - auto & specified = *info.ca; - auto actualHash = ({ - auto accessor = getFSAccessor(false); - CanonPath path { printStorePath(info.path) }; - Hash h { HashAlgorithm::SHA256 }; // throwaway def to appease C++ - auto fim = specified.method.getFileIngestionMethod(); - switch (fim) { - case FileIngestionMethod::Flat: - case FileIngestionMethod::NixArchive: - { - HashModuloSink caSink { - specified.hash.algo, - std::string { info.path.hashPart() }, + narRead = true; + restorePath(realPath, wrapperSource, settings.fsyncStorePaths); + + auto hashResult = hashSink.finish(); + + if (hashResult.first != info.narHash) + throw Error("hash mismatch importing path '%s';\n specified: %s\n got: %s", + printStorePath(info.path), info.narHash.to_string(HashFormat::Nix32, true), hashResult.first.to_string(HashFormat::Nix32, true)); + + if (hashResult.second != info.narSize) + throw Error("size mismatch importing path '%s';\n specified: %s\n got: %s", + printStorePath(info.path), info.narSize, hashResult.second); + + if (info.ca) { + auto & specified = *info.ca; + auto actualHash = ({ + auto accessor = getFSAccessor(false); + CanonPath path { printStorePath(info.path) }; + Hash h { HashAlgorithm::SHA256 }; // throwaway def to appease C++ + auto fim = specified.method.getFileIngestionMethod(); + switch (fim) { + case FileIngestionMethod::Flat: + case FileIngestionMethod::NixArchive: + { + HashModuloSink caSink { + specified.hash.algo, + std::string { info.path.hashPart() }, + }; + dumpPath({accessor, path}, caSink, (FileSerialisationMethod) fim); + h = caSink.finish().first; + break; + } + case FileIngestionMethod::Git: + h = git::dumpHash(specified.hash.algo, {accessor, path}).hash; + break; + } + ContentAddress { + .method = specified.method, + .hash = std::move(h), }; - dumpPath({accessor, path}, caSink, (FileSerialisationMethod) fim); - h = caSink.finish().first; - break; + }); + if (specified.hash != actualHash.hash) { + throw Error("ca hash mismatch importing path '%s';\n specified: %s\n got: %s", + printStorePath(info.path), + specified.hash.to_string(HashFormat::Nix32, true), + actualHash.hash.to_string(HashFormat::Nix32, true)); } - case FileIngestionMethod::Git: - h = git::dumpHash(specified.hash.algo, {accessor, path}).hash; - break; - } - ContentAddress { - .method = specified.method, - .hash = std::move(h), - }; - }); - if (specified.hash != actualHash.hash) { - throw Error("ca hash mismatch importing path '%s';\n specified: %s\n got: %s", - printStorePath(info.path), - specified.hash.to_string(HashFormat::Nix32, true), - actualHash.hash.to_string(HashFormat::Nix32, true)); } + + autoGC(); + + canonicalisePathMetaData(realPath); + + optimisePath(realPath, repair); // FIXME: combine with hashPath() + + if (settings.fsyncStorePaths) { + recursiveSync(realPath); + syncParent(realPath); + } + + registerValidPath(info); } - autoGC(); - - canonicalisePathMetaData(realPath); - - optimisePath(realPath, repair); // FIXME: combine with hashPath() - - if (settings.fsyncStorePaths) { - recursiveSync(realPath); - syncParent(realPath); - } - - registerValidPath(info); + outputLock.setDeletion(true); } - - outputLock.setDeletion(true); } + + // In case `cleanup` ignored an `Interrupted` exception + checkInterrupt(); } diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index 9d903f218..aeff24c64 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -35,7 +35,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 37793db5b..c855e797f 100644 --- a/src/libstore/pathlocks.cc +++ b/src/libstore/pathlocks.cc @@ -27,7 +27,7 @@ PathLocks::~PathLocks() try { unlock(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libstore/remote-fs-accessor.cc b/src/libstore/remote-fs-accessor.cc index 20f1d826c..7e360b5fe 100644 --- a/src/libstore/remote-fs-accessor.cc +++ b/src/libstore/remote-fs-accessor.cc @@ -30,7 +30,7 @@ ref RemoteFSAccessor::addToCache(std::string_view hashPart, std: /* FIXME: do this asynchronously. */ writeFile(makeCacheFile(hashPart, "nar"), nar); } catch (...) { - ignoreException(); + ignoreExceptionExceptInterrupt(); } } @@ -42,7 +42,7 @@ ref RemoteFSAccessor::addToCache(std::string_view hashPart, std: nlohmann::json j = listNar(narAccessor, CanonPath::root, true); writeFile(makeCacheFile(hashPart, "ls"), j.dump()); } catch (...) { - ignoreException(); + ignoreExceptionExceptInterrupt(); } } diff --git a/src/libstore/sqlite.cc b/src/libstore/sqlite.cc index 3175c1978..f02e472fd 100644 --- a/src/libstore/sqlite.cc +++ b/src/libstore/sqlite.cc @@ -86,7 +86,7 @@ SQLite::~SQLite() if (db && sqlite3_close(db) != SQLITE_OK) SQLiteError::throw_(db, "closing database"); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } @@ -125,7 +125,7 @@ SQLiteStmt::~SQLiteStmt() if (stmt && sqlite3_finalize(stmt) != SQLITE_OK) SQLiteError::throw_(db, "finalizing statement '%s'", sql); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } @@ -240,7 +240,7 @@ SQLiteTxn::~SQLiteTxn() if (active && sqlite3_exec(db, "rollback;", 0, 0, 0) != SQLITE_OK) SQLiteError::throw_(db, "aborting transaction"); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 426a69ae2..8109ea322 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -1055,7 +1055,7 @@ std::map 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/libstore/unix/build/hook-instance.cc b/src/libstore/unix/build/hook-instance.cc index 4f8492fe9..79eb25a91 100644 --- a/src/libstore/unix/build/hook-instance.cc +++ b/src/libstore/unix/build/hook-instance.cc @@ -91,7 +91,7 @@ HookInstance::~HookInstance() toHook.writeSide = -1; if (pid != -1) pid.kill(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index 08b973cd1..732988df3 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -109,9 +109,9 @@ LocalDerivationGoal::~LocalDerivationGoal() { /* 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(); } } @@ -1531,7 +1531,7 @@ void LocalDerivationGoal::startDaemon() NotTrusted, daemon::Recursive); debug("terminated daemon connection"); } catch (SystemError &) { - ignoreException(); + ignoreExceptionExceptInterrupt(); } }); diff --git a/src/libstore/worker-protocol-connection.cc b/src/libstore/worker-protocol-connection.cc index ae434c7f0..6585df4be 100644 --- a/src/libstore/worker-protocol-connection.cc +++ b/src/libstore/worker-protocol-connection.cc @@ -12,7 +12,7 @@ WorkerProto::BasicClientConnection::~BasicClientConnection() try { to.flush(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libutil/current-process.cc b/src/libutil/current-process.cc index ed1c1ca6c..ac01f441e 100644 --- a/src/libutil/current-process.cc +++ b/src/libutil/current-process.cc @@ -45,7 +45,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/file-descriptor.cc b/src/libutil/file-descriptor.cc index 4c5daf398..3d8d70fdb 100644 --- a/src/libutil/file-descriptor.cc +++ b/src/libutil/file-descriptor.cc @@ -2,6 +2,7 @@ #include "signals.hh" #include "finally.hh" #include "serialise.hh" +#include "util.hh" #include #include @@ -65,7 +66,7 @@ AutoCloseFD::~AutoCloseFD() try { close(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index edcacb50a..224b78b23 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -5,6 +5,7 @@ #include "signals.hh" #include "finally.hh" #include "serialise.hh" +#include "util.hh" #include #include @@ -517,7 +518,7 @@ AutoDelete::~AutoDelete() } } } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libutil/logging.cc b/src/libutil/logging.cc index 29427f2f6..3ef71a716 100644 --- a/src/libutil/logging.cc +++ b/src/libutil/logging.cc @@ -346,7 +346,7 @@ Activity::~Activity() try { logger.stopActivity(id); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libutil/serialise.cc b/src/libutil/serialise.cc index 4aa5ae385..168d2ed32 100644 --- a/src/libutil/serialise.cc +++ b/src/libutil/serialise.cc @@ -1,5 +1,6 @@ #include "serialise.hh" #include "signals.hh" +#include "util.hh" #include #include @@ -52,7 +53,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 4bb1a3e4b..d9e34e1e0 100644 --- a/src/libutil/serialise.hh +++ b/src/libutil/serialise.hh @@ -503,7 +503,7 @@ struct FramedSource : Source } } } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } @@ -550,7 +550,7 @@ struct FramedSink : nix::BufferedSink to << 0; to.flush(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } diff --git a/src/libutil/thread-pool.cc b/src/libutil/thread-pool.cc index 0f6349642..0355e1f07 100644 --- a/src/libutil/thread-pool.cc +++ b/src/libutil/thread-pool.cc @@ -111,9 +111,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/libutil/unix/signals.cc b/src/libutil/unix/signals.cc index 7e30687d8..d0608dace 100644 --- a/src/libutil/unix/signals.cc +++ b/src/libutil/unix/signals.cc @@ -91,7 +91,7 @@ void unix::triggerInterrupt() try { callback(); } catch (...) { - ignoreException(); + ignoreExceptionInDestructor(); } } } diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 0d728e2d5..ed5c7e4f1 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -1,6 +1,7 @@ #include "util.hh" #include "fmt.hh" #include "file-path.hh" +#include "signals.hh" #include #include @@ -182,7 +183,7 @@ std::string shellEscape(const std::string_view s) } -void ignoreException(Verbosity lvl) +void ignoreExceptionInDestructor(Verbosity lvl) { /* Make sure no exceptions leave this function. printError() also throws when remote is closed. */ @@ -195,6 +196,17 @@ 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()); + } +} + constexpr char base64Chars[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"; diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 75a4af76e..0fb6ff837 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -156,9 +156,26 @@ std::string toLower(std::string s); std::string shellEscape(const std::string_view s); -/* Exception handling in destructors: print an error message, then - ignore the exception. */ -void ignoreException(Verbosity lvl = lvlError); +/** + * 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 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/nix/develop.cc b/src/nix/develop.cc index 04672e2ad..c7a733025 100644 --- a/src/nix/develop.cc +++ b/src/nix/develop.cc @@ -672,7 +672,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 8e109b327..640a80aed 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -372,9 +372,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