fix the logger leaks

Passing around the loggers as raw pointers or references everywhere is really
error prone, so they were just leaked all over the place. Introduce unique
ownership instead so that they are properly freed when no longer needed (for
example, when a different logger is created or the program is terminated).

Of course, actually exercising the destructors uncovered a bug in ProgressBar,
where creating it in inactive state (on a non-TTY stderr) resulted in the
update thread starting anyway, violating the invariant required by the
destructor. This had to be fixed too.

Change-Id: I372cb710aaca492c2adcd8241f1a8e57221b5d16

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 = makeJSONLogger(std::move(logger));
 
         /* Ensure we don't get any SSH passphrase or host key popups. */
         unsetenv("DISPLAY");

src/libmain/loggers.cc

@@ -24,14 +24,14 @@ LogFormat parseLogFormat(const std::string & logFormatStr) {
     throw Error("option 'log-format' has an invalid value '%s'", logFormatStr);
 }
 
-Logger * makeDefaultLogger() {
+std::unique_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: {

src/libmain/progress-bar.cc

@@ -44,17 +44,20 @@ static std::string_view storePathToName(std::string_view path)
 ProgressBar::ProgressBar(bool isTTY)
     : isTTY(isTTY)
 {
-    state_.lock()->active = isTTY;
-    updateThread = std::thread([&]() {
-        auto state(state_.lock());
-        auto nextWakeup = A_LONG_TIME;
-        while (state->active) {
-            if (!state->haveUpdate)
-                state.wait_for(updateCV, nextWakeup);
-            nextWakeup = draw(*state, {});
-            state.wait_for(quitCV, std::chrono::milliseconds(50));
-        }
-    });
+    if ((state_.lock()->active = isTTY)) {
+        // Only start the update thread when the logger is set to active.
+        // Otherwise, the destructor will std::terminate trying to destroy a joinable thread.
+        updateThread = std::thread([&]() {
+            auto state(state_.lock());
+            auto nextWakeup = A_LONG_TIME;
+            while (state->active) {
+                if (!state->haveUpdate)
+                    state.wait_for(updateCV, nextWakeup);
+                nextWakeup = draw(*state, {});
+                state.wait_for(quitCV, std::chrono::milliseconds(50));
+            }
+        });
+    }
 }
 
 ProgressBar::~ProgressBar()
@@ -554,9 +557,9 @@ void ProgressBar::setPrintMultiline(bool printMultiline)
     this->printMultiline = printMultiline;
 }
 
-Logger * makeProgressBar()
+std::unique_ptr<Logger> makeProgressBar()
 {
-    return new ProgressBar(shouldANSI());
+    return std::make_unique<ProgressBar>(shouldANSI());
 }
 
 void startProgressBar()
@@ -566,7 +569,7 @@ void startProgressBar()
 
 void stopProgressBar()
 {
-    auto progressBar = dynamic_cast<ProgressBar *>(logger);
+    auto progressBar = dynamic_cast<ProgressBar *>(logger.get());
     if (progressBar) progressBar->stop();
 
 }

src/libmain/progress-bar.hh

@@ -111,7 +111,7 @@ struct ProgressBar : public Logger
     void setPrintMultiline(bool printMultiline) override;
 };
 
-Logger * makeProgressBar();
+std::unique_ptr<Logger> makeProgressBar();
 
 void startProgressBar();
 

src/libstore/build/local-derivation-goal.cc

@@ -2636,7 +2636,7 @@ void LocalDerivationGoal::runChild()
         /* Execute the program.  This should not return. */
         if (drv->isBuiltin()) {
             try {
-                logger = makeJSONLogger(*logger);
+                logger = makeJSONLogger(std::move(logger));
 
                 BasicDerivation & drv2(*drv);
                 for (auto & e : drv2.env)

src/libstore/daemon.cc

@@ -991,11 +991,13 @@ void processConnection(
     if (clientVersion < MIN_SUPPORTED_WORKER_PROTO_VERSION)
         throw Error("the Nix client version is too old");
 
-    auto tunnelLogger = new TunnelLogger(to, clientVersion);
-    auto prevLogger = nix::logger;
+    std::unique_ptr<Logger> savedPrevLogger;
+    auto prevLogger = nix::logger.get();
+    auto ownedTunnelLogger = std::make_unique<TunnelLogger>(to, clientVersion);
+    auto tunnelLogger = ownedTunnelLogger.get();
     // FIXME
     if (!recursive)
-        logger = tunnelLogger;
+        savedPrevLogger = std::exchange(logger, std::move(ownedTunnelLogger));
 
     unsigned int opCount = 0;
 

src/libutil/logging.cc

@@ -27,7 +27,7 @@ void setCurActivity(const ActivityId activityId)
     curActivity = activityId;
 }
 
-Logger * logger = makeSimpleLogger(true);
+std::unique_ptr<Logger> logger = makeSimpleLogger(true);
 
 void Logger::warn(const std::string & msg)
 {
@@ -129,9 +129,9 @@ void writeToStderr(std::string_view s)
     }
 }
 
-Logger * makeSimpleLogger(bool printBuildLogs)
+std::unique_ptr<Logger> makeSimpleLogger(bool printBuildLogs)
 {
-    return new SimpleLogger(printBuildLogs);
+    return std::make_unique<SimpleLogger>(printBuildLogs);
 }
 
 std::atomic<uint64_t> nextId{0};
@@ -159,9 +159,9 @@ void to_json(nlohmann::json & json, std::shared_ptr<Pos> pos)
 }
 
 struct JSONLogger : Logger {
-    Logger & prevLogger;
+    std::unique_ptr<Logger> prevLogger;
 
-    JSONLogger(Logger & prevLogger) : prevLogger(prevLogger) { }
+    JSONLogger(std::unique_ptr<Logger> prevLogger) : prevLogger(std::move(prevLogger)) { }
 
     bool isVerbose() override {
         return true;
@@ -182,7 +182,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
@@ -254,9 +254,9 @@ struct JSONLogger : Logger {
     }
 };
 
-Logger * makeJSONLogger(Logger & prevLogger)
+std::unique_ptr<Logger> makeJSONLogger(std::unique_ptr<Logger> prevLogger)
 {
-    return new JSONLogger(prevLogger);
+    return std::make_unique<JSONLogger>(std::move(prevLogger));
 }
 
 static Logger::Fields getFields(nlohmann::json & json)

src/libutil/logging.hh

@@ -227,11 +227,11 @@ struct PushActivity
     ~PushActivity() { setCurActivity(prevAct); }
 };
 
-extern Logger * logger;
+extern std::unique_ptr<Logger> logger;
 
-Logger * makeSimpleLogger(bool printBuildLogs = true);
+std::unique_ptr<Logger> makeSimpleLogger(bool printBuildLogs = true);
 
-Logger * makeJSONLogger(Logger & prevLogger);
+std::unique_ptr<Logger> makeJSONLogger(std::unique_ptr<Logger> prevLogger);
 
 /**
  * suppress msgs > this

tests/unit/libexpr/primops.cc

@@ -27,20 +27,17 @@ namespace nix {
     };
 
     class CaptureLogging {
-        Logger * oldLogger;
-        std::unique_ptr<CaptureLogger> tempLogger;
+        std::unique_ptr<Logger> oldLogger;
         public:
-            CaptureLogging() : tempLogger(std::make_unique<CaptureLogger>()) {
-                oldLogger = logger;
-                logger = tempLogger.get();
-            }
+            CaptureLogging() : oldLogger(std::exchange(logger, std::make_unique<CaptureLogger>())) {}
 
             ~CaptureLogging() {
-                logger = oldLogger;
+                logger = std::move(oldLogger);
             }
 
             std::string get() const {
-                return tempLogger->get();
+                auto captureLogger = dynamic_cast<CaptureLogger *>(logger.get());
+                return captureLogger ? captureLogger->get() : "";
             }
     };
 

tests/unit/libmain/progress-bar.cc

@@ -24,7 +24,7 @@ namespace nix
         initGC();
 
         startProgressBar();
-        ASSERT_NE(dynamic_cast<ProgressBar *>(logger), nullptr);
+        ASSERT_NE(dynamic_cast<ProgressBar *>(logger.get()), nullptr);
         ProgressBar & progressBar = dynamic_cast<ProgressBar &>(*logger);
 
         Activity act(