tree-wide: make logger a shared_ptr so it actually destructs at the end of the program

It was, of course, just leaked before. Fixing this naturally exposed some bugs.

Change-Id: I013d4631ae424929e28c2705943410592116c965

src/build-remote/build-remote.cc

@@ -60,7 +60,7 @@ static bool allSupportedLocally(Store & store, const std::set<std::string>& 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");

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<Logger> 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());
 }
 
 }

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 <atomic>
 #include <map>
 #include <thread>
 #include <sstream>
@@ -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<Logger> makeProgressBar()
 {
-    return new ProgressBar(shouldANSI());
+    return std::make_shared<ProgressBar>(shouldANSI());
 }
 
 void startProgressBar()
 {
-    logger = makeProgressBar();
+    logger.replace(makeProgressBar());
 }
 
 void stopProgressBar()
 {
-    auto progressBar = dynamic_cast<ProgressBar *>(logger);
+    auto progressBar = dynamic_cast<ProgressBar *>(&**logger);
     if (progressBar) progressBar->stop();
 
 }

src/libmain/progress-bar.hh

@@ -5,7 +5,7 @@
 
 namespace nix {
 
-Logger * makeProgressBar();
+std::shared_ptr<Logger> makeProgressBar();
 
 void startProgressBar();
 

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);

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> logger,
     const StorePath & drvPath,
     const StorePathSet & outputPaths)
 {

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)

src/libstore/daemon.cc

@@ -263,7 +263,7 @@ struct ClientSettings
     }
 };
 
-static void performOp(TunnelLogger * logger, ref<Store> store,
+static void performOp(std::shared_ptr<TunnelLogger> logger, ref<Store> 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<TunnelLogger>(to, clientVersion);
+    auto prevLogger = *nix::logger;
     // FIXME
     if (!recursive)
-        logger = tunnelLogger;
+        prevLogger = nix::logger.replace(tunnelLogger);
 
     unsigned int opCount = 0;
 

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<Logger>(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<Logger> makeSimpleLogger(bool printBuildLogs)
 {
-    return new SimpleLogger(printBuildLogs);
+    return std::make_shared<SimpleLogger>(printBuildLogs);
 }
 
 std::atomic<uint64_t> nextId{0};
 
-Activity::Activity(Logger & logger, Verbosity lvl, ActivityType type,
+Activity::Activity(std::shared_ptr<Logger> 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> pos)
@@ -152,9 +182,9 @@ void to_json(nlohmann::json & json, std::shared_ptr<Pos> pos)
 }
 
 struct JSONLogger : Logger {
-    Logger & prevLogger;
+    std::shared_ptr<Logger> prevLogger;
 
-    JSONLogger(Logger & prevLogger) : prevLogger(prevLogger) { }
+    JSONLogger(std::shared_ptr<Logger> 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<Logger> makeJSONLogger(std::shared_ptr<Logger> prevLogger)
 {
-    return new JSONLogger(prevLogger);
+    return std::make_shared<JSONLogger>(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();
     }

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> logger;
 
     const ActivityId id;
 
-    Activity(Logger & logger, Verbosity lvl, ActivityType type, const std::string & s = "",
+    Activity(std::shared_ptr<Logger> 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> 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<std::shared_ptr<Logger>> inner;
 
-Logger * makeSimpleLogger(bool printBuildLogs = true);
+    explicit LoggerWrapper(std::shared_ptr<Logger> inner) : inner(inner) {}
 
-Logger * makeJSONLogger(Logger & prevLogger);
+    const std::shared_ptr<Logger> operator->() const {
+        return inner.load(std::memory_order_relaxed);
+    }
+
+    const std::shared_ptr<Logger> operator*() const {
+        return inner.load(std::memory_order_relaxed);
+    }
+
+    /**
+     * Replaces the logger with a new one atomically.
+     */
+    const std::shared_ptr<Logger> replace(std::shared_ptr<Logger> newInner) {
+        return inner.exchange(std::move(newInner), std::memory_order_relaxed);
+    }
+};
+
+extern LoggerWrapper& logger;
+
+std::shared_ptr<Logger> makeSimpleLogger(bool printBuildLogs = true);
+
+std::shared_ptr<Logger> makeJSONLogger(std::shared_ptr<Logger> prevLogger);
 
 /**
  * suppress msgs > this

src/libutil/processes.cc

@@ -190,7 +190,7 @@ static int childEntry(void * arg)
 pid_t startProcess(std::function<void()> fun, const ProcessOptions & options)
 {
     std::function<void()> wrapper = [&]() {
-        logger = makeSimpleLogger();
+        logger.replace(makeSimpleLogger());
         try {
 #if __linux__
             if (options.dieWithParent && prctl(PR_SET_PDEATHSIG, SIGKILL) == -1)

tests/unit/libexpr/primops.cc

@@ -27,16 +27,15 @@ namespace nix {
     };
 
     class CaptureLogging {
-        Logger * oldLogger;
-        std::unique_ptr<CaptureLogger> tempLogger;
+        std::shared_ptr<Logger> oldLogger;
+        std::shared_ptr<CaptureLogger> tempLogger;
         public:
-            CaptureLogging() : tempLogger(std::make_unique<CaptureLogger>()) {
-                oldLogger = logger;
-                logger = tempLogger.get();
+            CaptureLogging() : tempLogger(std::make_shared<CaptureLogger>()) {
+                oldLogger = logger.replace(tempLogger);
             }
 
             ~CaptureLogging() {
-                logger = oldLogger;
+                logger.replace(oldLogger);
             }
 
             std::string get() const {