Merge pull request #6962 from edolstra/unique-ptr-logger

Make 'logger' a std::unique_ptr

src/libexpr-tests/primops.cc

@@ -28,20 +28,15 @@ 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::move(logger);
+                logger = std::make_unique<CaptureLogger>();
             }
 
             ~CaptureLogging() {
-                logger = oldLogger;
-            }
-
-            std::string get() const {
-                return tempLogger->get();
+                logger = std::move(oldLogger);
             }
     };
 
@@ -113,7 +108,7 @@ namespace nix {
         CaptureLogging l;
         auto v = eval("builtins.trace \"test string 123\" 123");
         ASSERT_THAT(v, IsIntEq(123));
-        auto text = l.get();
+        auto text = (dynamic_cast<CaptureLogger *>(logger.get()))->get();
         ASSERT_NE(text.find("test string 123"), std::string::npos);
     }
 

src/libmain/loggers.cc

@@ -6,7 +6,8 @@ namespace nix {
 
 LogFormat defaultLogFormat = LogFormat::raw;
 
-LogFormat parseLogFormat(const std::string & logFormatStr) {
+LogFormat parseLogFormat(const std::string & logFormatStr)
+{
     if (logFormatStr == "raw" || getEnv("NIX_GET_COMPLETIONS"))
         return LogFormat::raw;
     else if (logFormatStr == "raw-with-logs")
@@ -20,7 +21,8 @@ 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);
@@ -40,16 +42,19 @@ Logger * makeDefaultLogger() {
     }
 }
 
-void setLogFormat(const std::string & logFormatStr) {
+void setLogFormat(const std::string & logFormatStr)
+{
     setLogFormat(parseLogFormat(logFormatStr));
 }
 
-void setLogFormat(const LogFormat & logFormat) {
+void setLogFormat(const LogFormat & logFormat)
+{
     defaultLogFormat = logFormat;
     createDefaultLogger();
 }
 
-void createDefaultLogger() {
+void createDefaultLogger()
+{
     logger = makeDefaultLogger();
 }
 

src/libmain/progress-bar.cc

@@ -117,12 +117,14 @@ public:
     {
         {
             auto state(state_.lock());
-            if (!state->active) return;
+            if (state->active) {
                 state->active = false;
                 writeToStderr("\r\e[K");
                 updateCV.notify_one();
                 quitCV.notify_one();
             }
+        }
+        if (updateThread.joinable())
             updateThread.join();
     }
 
@@ -553,9 +555,9 @@ public:
     }
 };
 
-Logger * makeProgressBar()
+std::unique_ptr<Logger> makeProgressBar()
 {
-    return new ProgressBar(isTTY());
+    return std::make_unique<ProgressBar>(isTTY());
 }
 
 void startProgressBar()
@@ -565,9 +567,8 @@ void startProgressBar()
 
 void stopProgressBar()
 {
-    auto progressBar = dynamic_cast<ProgressBar *>(logger);
-    if (progressBar) progressBar->stop();
-
+    if (auto progressBar = dynamic_cast<ProgressBar *>(logger.get()))
+        progressBar->stop();
 }
 
 }

src/libmain/progress-bar.hh

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

src/libstore/daemon.cc

@@ -1041,11 +1041,15 @@ void processConnection(
     conn.protoVersion = protoVersion;
     conn.features = features;
 
-    auto tunnelLogger = new TunnelLogger(conn.to, protoVersion);
-    auto prevLogger = nix::logger;
+    auto tunnelLogger_ = std::make_unique<TunnelLogger>(conn.to, protoVersion);
+    auto tunnelLogger = tunnelLogger_.get();
+    std::unique_ptr<Logger> prevLogger_;
+    auto prevLogger = logger.get();
     // FIXME
-    if (!recursive)
-        logger = tunnelLogger;
+    if (!recursive) {
+        prevLogger_ = std::move(logger);
+        logger = std::move(tunnelLogger_);
+    }
 
     unsigned int opCount = 0;
 

src/libutil/logging.cc

@@ -29,7 +29,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)
 {
@@ -128,9 +128,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};
@@ -262,9 +262,9 @@ struct JSONLogger : Logger {
     }
 };
 
-Logger * makeJSONLogger(Descriptor fd)
+std::unique_ptr<Logger> makeJSONLogger(Descriptor fd)
 {
-    return new JSONLogger(fd);
+    return std::make_unique<JSONLogger>(fd);
 }
 
 static Logger::Fields getFields(nlohmann::json & json)

src/libutil/logging.hh

@@ -180,11 +180,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(Descriptor fd);
+std::unique_ptr<Logger> makeJSONLogger(Descriptor fd);
 
 /**
  * @param source A noun phrase describing the source of the message, e.g. "the builder".

src/libutil/unix/processes.cc

@@ -200,8 +200,15 @@ static int childEntry(void * arg)
 pid_t startProcess(std::function<void()> fun, const ProcessOptions & options)
 {
     ChildWrapperFunction wrapper = [&] {
-        if (!options.allowVfork)
+        if (!options.allowVfork) {
+            /* Set a simple logger, while releasing (not destroying)
+               the parent logger. We don't want to run the parent
+               logger's destructor since that will crash (e.g. when
+               ~ProgressBar() tries to join a thread that doesn't
+               exist. */
+            logger.release();
             logger = makeSimpleLogger();
+        }
         try {
 #if __linux__
             if (options.dieWithParent && prctl(PR_SET_PDEATHSIG, SIGKILL) == -1)

src/nix/main.cc

@@ -388,8 +388,6 @@ void mainWrapped(int argc, char * * argv)
     }
     #endif
 
-    Finally f([] { logger->stop(); });
-
     programPath = argv[0];
     auto programName = std::string(baseNameOf(programPath));
     auto extensionPos = programName.find_last_of(".");