diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index fb90403a0..aa8cd37e1 100644 --- a/src/build-remote/build-remote.cc +++ b/src/build-remote/build-remote.cc @@ -60,7 +60,7 @@ static bool allSupportedLocally(Store & store, const std::set& requ static int main_build_remote(int argc, char * * argv) { { - logger = makeJSONLogger(*logger); + logger.replace(makeJSONLogger(*logger)); /* Ensure we don't get any SSH passphrase or host key popups. */ unsetenv("DISPLAY"); diff --git a/src/libmain/loggers.cc b/src/libmain/loggers.cc index 80080d616..160133469 100644 --- a/src/libmain/loggers.cc +++ b/src/libmain/loggers.cc @@ -20,14 +20,14 @@ LogFormat parseLogFormat(const std::string & logFormatStr) { throw Error("option 'log-format' has an invalid value '%s'", logFormatStr); } -Logger * makeDefaultLogger() { +std::shared_ptr makeDefaultLogger() { switch (defaultLogFormat) { case LogFormat::raw: return makeSimpleLogger(false); case LogFormat::rawWithLogs: return makeSimpleLogger(true); case LogFormat::internalJSON: - return makeJSONLogger(*makeSimpleLogger(true)); + return makeJSONLogger(makeSimpleLogger(true)); case LogFormat::bar: return makeProgressBar(); case LogFormat::barWithLogs: { @@ -50,7 +50,7 @@ void setLogFormat(const LogFormat & logFormat) { } void createDefaultLogger() { - logger = makeDefaultLogger(); + logger.replace(makeDefaultLogger()); } } diff --git a/src/libmain/progress-bar.cc b/src/libmain/progress-bar.cc index d83b09cd4..c477e6f28 100644 --- a/src/libmain/progress-bar.cc +++ b/src/libmain/progress-bar.cc @@ -1,10 +1,10 @@ #include "progress-bar.hh" +#include "file-system.hh" +#include "strings.hh" #include "sync.hh" -#include "store-api.hh" #include "names.hh" #include "terminal.hh" -#include #include #include #include @@ -119,7 +119,18 @@ public: { { auto state(state_.lock()); - if (!state->active) return; + if (!state->active) { + // Unlock immediately so we don't deadlock. + { + auto _ = std::move(state); + } + // Even if the thread is inactive, the handle needs to be + // explicitly joined to not call terminate if it is destructed. + if (updateThread.joinable()) { + updateThread.join(); + } + return; + } state->active = false; writeToStderr("\r\e[K"); updateCV.notify_one(); @@ -531,19 +542,19 @@ public: } }; -Logger * makeProgressBar() +std::shared_ptr makeProgressBar() { - return new ProgressBar(shouldANSI()); + return std::make_shared(shouldANSI()); } void startProgressBar() { - logger = makeProgressBar(); + logger.replace(makeProgressBar()); } void stopProgressBar() { - auto progressBar = dynamic_cast(logger); + auto progressBar = dynamic_cast(&**logger); if (progressBar) progressBar->stop(); } diff --git a/src/libmain/progress-bar.hh b/src/libmain/progress-bar.hh index c3c6e3833..45489fccd 100644 --- a/src/libmain/progress-bar.hh +++ b/src/libmain/progress-bar.hh @@ -5,7 +5,7 @@ namespace nix { -Logger * makeProgressBar(); +std::shared_ptr makeProgressBar(); void startProgressBar(); diff --git a/src/libstore/build/child.cc b/src/libstore/build/child.cc index a82a5eec9..cc3d0596f 100644 --- a/src/libstore/build/child.cc +++ b/src/libstore/build/child.cc @@ -5,7 +5,7 @@ namespace nix { void commonChildInit() { - logger = makeSimpleLogger(); + logger.replace(makeSimpleLogger()); const static std::string pathNullDevice = "/dev/null"; restoreProcessContext(false); diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 97ba994ad..efaf80a81 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -873,9 +873,9 @@ void DerivationGoal::cleanupPostOutputsRegisteredModeNonCheck() { } -void runPostBuildHook( +static void runPostBuildHook( Store & store, - Logger & logger, + std::shared_ptr logger, const StorePath & drvPath, const StorePathSet & outputPaths) { diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 7066f5c93..9e6d2805a 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -2156,7 +2156,7 @@ void LocalDerivationGoal::runChild() /* Execute the program. This should not return. */ if (drv->isBuiltin()) { try { - logger = makeJSONLogger(*logger); + logger.replace(makeJSONLogger(*logger)); BasicDerivation & drv2(*drv); for (auto & e : drv2.env) diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index 4b55668cb..34298b77a 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -263,7 +263,7 @@ struct ClientSettings } }; -static void performOp(TunnelLogger * logger, ref store, +static void performOp(std::shared_ptr logger, ref store, TrustedFlag trusted, RecursiveFlag recursive, WorkerProto::Version clientVersion, Source & from, BufferedSink & to, WorkerProto::Op op) { @@ -1014,11 +1014,11 @@ void processConnection( if (clientVersion < 0x10a) throw Error("the Nix client version is too old"); - auto tunnelLogger = new TunnelLogger(to, clientVersion); - auto prevLogger = nix::logger; + auto tunnelLogger = std::make_shared(to, clientVersion); + auto prevLogger = *nix::logger; // FIXME if (!recursive) - logger = tunnelLogger; + prevLogger = nix::logger.replace(tunnelLogger); unsigned int opCount = 0; diff --git a/src/libutil/logging.cc b/src/libutil/logging.cc index b01bb4dd4..43f884328 100644 --- a/src/libutil/logging.cc +++ b/src/libutil/logging.cc @@ -26,8 +26,6 @@ void setCurActivity(const ActivityId activityId) curActivity = activityId; } -Logger * logger = makeSimpleLogger(true); - void Logger::warn(const std::string & msg) { log(lvlWarn, ANSI_WARNING "warning:" ANSI_NORMAL " " + msg); @@ -108,6 +106,38 @@ public: } }; +// XXX: We have to leak this so that we can not destroy the damned thing on +// shutdown because of destructor ordering. Bad evil horrible. +LoggerWrapper &logger = *(new LoggerWrapper(makeSimpleLogger(true))); + +class NullLogger : public Logger +{ + void log(Verbosity lvl, std::string_view s) override {} + + void logEI(const ErrorInfo & ei) override {} +}; + +Logger * globalNullLogger = new NullLogger(); +Logger * globalSimpleLogger = new SimpleLogger(false); + +[[gnu::destructor(65535)]] +void switchToShutdownLogger() +{ + static auto dontDelete = [](auto d) {}; + + // XXX: Replace the logger with a global logger that is leaked at the end of the process + // This is so that custom loggers with meaningful destructors are guaranteed to actually be destroyed. + // Genuinely I am so sorry for this code. + // + // If there's a tty on the other end, we assume it's probably ok if the log + // format regresses to basic, and we consider it more important that the + // logs get out at all. + // + // FIXME: Replace this madness with explicitly passing around a shared + // pointer to const LoggerWrapper, because holy cow globals are broken. + logger.replace(std::shared_ptr(isatty(STDERR_FILENO) ? globalSimpleLogger : globalNullLogger, dontDelete)); +} + Verbosity verbosity = lvlInfo; void writeToStderr(std::string_view s) @@ -122,18 +152,18 @@ void writeToStderr(std::string_view s) } } -Logger * makeSimpleLogger(bool printBuildLogs) +std::shared_ptr makeSimpleLogger(bool printBuildLogs) { - return new SimpleLogger(printBuildLogs); + return std::make_shared(printBuildLogs); } std::atomic nextId{0}; -Activity::Activity(Logger & logger, Verbosity lvl, ActivityType type, +Activity::Activity(std::shared_ptr logger, Verbosity lvl, ActivityType type, const std::string & s, const Logger::Fields & fields, ActivityId parent) : logger(logger), id(nextId++ + (((uint64_t) getpid()) << 32)) { - logger.startActivity(id, lvl, type, s, fields, parent); + logger->startActivity(id, lvl, type, s, fields, parent); } void to_json(nlohmann::json & json, std::shared_ptr pos) @@ -152,9 +182,9 @@ void to_json(nlohmann::json & json, std::shared_ptr pos) } struct JSONLogger : Logger { - Logger & prevLogger; + std::shared_ptr prevLogger; - JSONLogger(Logger & prevLogger) : prevLogger(prevLogger) { } + JSONLogger(std::shared_ptr prevLogger) : prevLogger(prevLogger) { } bool isVerbose() override { return true; @@ -175,7 +205,7 @@ struct JSONLogger : Logger { void write(const nlohmann::json & json) { - prevLogger.log(lvlError, "@nix " + json.dump(-1, ' ', false, nlohmann::json::error_handler_t::replace)); + prevLogger->log(lvlError, "@nix " + json.dump(-1, ' ', false, nlohmann::json::error_handler_t::replace)); } void log(Verbosity lvl, std::string_view s) override @@ -247,9 +277,9 @@ struct JSONLogger : Logger { } }; -Logger * makeJSONLogger(Logger & prevLogger) +std::shared_ptr makeJSONLogger(std::shared_ptr prevLogger) { - return new JSONLogger(prevLogger); + return std::make_shared(std::move(prevLogger)); } static Logger::Fields getFields(nlohmann::json & json) @@ -325,7 +355,7 @@ bool handleJSONLogMessage(const std::string & msg, Activity::~Activity() { try { - logger.stopActivity(id); + logger->stopActivity(id); } catch (...) { ignoreException(); } diff --git a/src/libutil/logging.hh b/src/libutil/logging.hh index 64be8bc62..7cfdae45a 100644 --- a/src/libutil/logging.hh +++ b/src/libutil/logging.hh @@ -1,7 +1,6 @@ #pragma once ///@file -#include "types.hh" #include "error.hh" #include "config.hh" @@ -132,14 +131,14 @@ void setCurActivity(const ActivityId activityId); struct Activity { - Logger & logger; + std::shared_ptr logger; const ActivityId id; - Activity(Logger & logger, Verbosity lvl, ActivityType type, const std::string & s = "", + Activity(std::shared_ptr logger, Verbosity lvl, ActivityType type, const std::string & s = "", const Logger::Fields & fields = {}, ActivityId parent = getCurActivity()); - Activity(Logger & logger, ActivityType type, + Activity(std::shared_ptr logger, ActivityType type, const Logger::Fields & fields = {}, ActivityId parent = getCurActivity()) : Activity(logger, lvlError, type, "", fields, parent) { }; @@ -163,7 +162,7 @@ struct Activity void result(ResultType type, const Logger::Fields & fields) const { - logger.result(id, type, fields); + logger->result(id, type, fields); } friend class Logger; @@ -176,11 +175,39 @@ struct PushActivity ~PushActivity() { setCurActivity(prevAct); } }; -extern Logger * logger; +/** Wrapper to make replacing the global logger object thread-safe, since we do + * that in various places. + * + * The actual usage of shared_ptr across threads is ok, only replacing it is + * not thread safe. + */ +struct LoggerWrapper +{ + std::atomic> inner; -Logger * makeSimpleLogger(bool printBuildLogs = true); + explicit LoggerWrapper(std::shared_ptr inner) : inner(inner) {} -Logger * makeJSONLogger(Logger & prevLogger); + const std::shared_ptr operator->() const { + return inner.load(std::memory_order_relaxed); + } + + const std::shared_ptr operator*() const { + return inner.load(std::memory_order_relaxed); + } + + /** + * Replaces the logger with a new one atomically. + */ + const std::shared_ptr replace(std::shared_ptr newInner) { + return inner.exchange(std::move(newInner), std::memory_order_relaxed); + } +}; + +extern LoggerWrapper& logger; + +std::shared_ptr makeSimpleLogger(bool printBuildLogs = true); + +std::shared_ptr makeJSONLogger(std::shared_ptr prevLogger); /** * suppress msgs > this diff --git a/src/libutil/processes.cc b/src/libutil/processes.cc index 8639a77f8..ef5fd5531 100644 --- a/src/libutil/processes.cc +++ b/src/libutil/processes.cc @@ -190,7 +190,7 @@ static int childEntry(void * arg) pid_t startProcess(std::function fun, const ProcessOptions & options) { std::function wrapper = [&]() { - logger = makeSimpleLogger(); + logger.replace(makeSimpleLogger()); try { #if __linux__ if (options.dieWithParent && prctl(PR_SET_PDEATHSIG, SIGKILL) == -1) diff --git a/tests/unit/libexpr/primops.cc b/tests/unit/libexpr/primops.cc index bd174a6c0..38d4f282f 100644 --- a/tests/unit/libexpr/primops.cc +++ b/tests/unit/libexpr/primops.cc @@ -27,16 +27,15 @@ namespace nix { }; class CaptureLogging { - Logger * oldLogger; - std::unique_ptr tempLogger; + std::shared_ptr oldLogger; + std::shared_ptr tempLogger; public: - CaptureLogging() : tempLogger(std::make_unique()) { - oldLogger = logger; - logger = tempLogger.get(); + CaptureLogging() : tempLogger(std::make_shared()) { + oldLogger = logger.replace(tempLogger); } ~CaptureLogging() { - logger = oldLogger; + logger.replace(oldLogger); } std::string get() const {