From a448a39fb0cbab638dbd4bd51c45a31876dcb1a8 Mon Sep 17 00:00:00 2001
From: Qyriad <qyriad@qyriad.me>
Date: Wed, 26 Jun 2024 15:25:36 -0600
Subject: [PATCH] repl: respect --print-build-logs & fix memory leak

243c0f18d[1] allowed the logger's progress bar to display during repl
builds, but startProgressBar() re-creates the entire logger from
scratch, discarding the value of printBuildLogs (and leaking the
previous logger). In this commit, we instead make the update thread
stopping solely conditional on quitCV, unset active, and manually
reset activities and stored values in the progress bar's state, so stuff
from multiple builds don't leak over into following ones.

[1]: 243c0f18dae2a08ea0e46f7ff33277c63f7506d7

Change-Id: Ie734d1f638d45759d232805d7e3c2005f7dea483
---
 src/libcmd/repl.cc          |  7 ++--
 src/libmain/progress-bar.cc | 66 ++++++++++++++++++++++++++++++-------
 src/libmain/progress-bar.hh | 16 +++++++++
 3 files changed, 75 insertions(+), 14 deletions(-)

diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc
index 28341259c..d8d7ad3c0 100644
--- a/src/libcmd/repl.cc
+++ b/src/libcmd/repl.cc
@@ -300,7 +300,7 @@ ReplExitStatus NixRepl::mainLoop()
 
     /* Stop the progress bar because it interferes with the display of
        the repl. */
-    stopProgressBar();
+    deactivateProgressBar();
 
     std::string input;
 
@@ -684,11 +684,12 @@ ProcessLineResult NixRepl::processLine(std::string line)
             // TODO: this only shows a progress bar for explicitly initiated builds,
             // not eval-time fetching or builds performed for IFD.
             // But we can't just show it everywhere, since that would erase partial output from evaluation.
-            startProgressBar();
+            reactivateProgressBar();
             Finally stopLogger([&]() {
-                stopProgressBar();
+                deactivateProgressBar();
             });
 
+
             state->store->buildPaths({
                 DerivedPath::Built {
                     .drvPath = makeConstantStorePathRef(drvPath),
diff --git a/src/libmain/progress-bar.cc b/src/libmain/progress-bar.cc
index 8c8b60099..7c52a2bb7 100644
--- a/src/libmain/progress-bar.cc
+++ b/src/libmain/progress-bar.cc
@@ -44,20 +44,30 @@ static std::string_view storePathToName(std::string_view path)
 ProgressBar::ProgressBar(bool isTTY)
     : isTTY(isTTY)
 {
-    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;
+    state_.lock()->active = isTTY;
+    updateThread = std::thread([&]() {
+        auto state(state_.lock());
+        auto nextWakeup = A_LONG_TIME;
+
+        auto shouldQuit = [&]() -> bool {
+            auto const status = state.wait_for(quitCV, std::chrono::milliseconds(50));
+            return status != std::cv_status::timeout;
+        };
+
+        while (!shouldQuit()) {
             while (state->active) {
-                if (!state->haveUpdate)
+                if (!state->haveUpdate) {
                     state.wait_for(updateCV, nextWakeup);
-                nextWakeup = draw(*state, {});
-                state.wait_for(quitCV, std::chrono::milliseconds(50));
+                }
+
+                nextWakeup = draw(*state, std::nullopt);
+
+                // So we don't spin this loop too much while we're active but
+                // without updates.
+                shouldQuit();
             }
-        });
-    }
+        }
+    });
 }
 
 ProgressBar::~ProgressBar()
@@ -557,6 +567,24 @@ void ProgressBar::setPrintMultiline(bool printMultiline)
     this->printMultiline = printMultiline;
 }
 
+void ProgressBar::deactivateBar()
+{
+    auto state(state_.lock());
+    bool const prevPaused = state->paused;
+    bool const prevHaveUpdate = state->haveUpdate;
+    *state = ProgressBar::State{
+        .active = false,
+        .paused = prevPaused,
+        .haveUpdate = prevHaveUpdate,
+        // Default constructors for the rest.
+    };
+}
+
+void ProgressBar::reactivateBar()
+{
+    state_.lock()-> active = true;
+}
+
 Logger * makeProgressBar()
 {
     return new ProgressBar(shouldANSI());
@@ -574,4 +602,20 @@ void stopProgressBar()
 
 }
 
+void deactivateProgressBar()
+{
+    auto progressBar = dynamic_cast<ProgressBar *>(logger);
+    if (progressBar) {
+        progressBar->deactivateBar();
+    }
+}
+
+void reactivateProgressBar()
+{
+    auto progressBar = dynamic_cast<ProgressBar *>(logger);
+    if (progressBar) {
+        progressBar->reactivateBar();
+    }
+}
+
 }
diff --git a/src/libmain/progress-bar.hh b/src/libmain/progress-bar.hh
index e682d75fe..a35e348b8 100644
--- a/src/libmain/progress-bar.hh
+++ b/src/libmain/progress-bar.hh
@@ -109,6 +109,16 @@ struct ProgressBar : public Logger
     void setPrintBuildLogs(bool printBuildLogs) override;
 
     void setPrintMultiline(bool printMultiline) override;
+
+    /** Deactivate the progress bar and reset activity statistics, but leave other
+     * logging alone (unlike pause() which pauses all logging).
+    */
+    void deactivateBar();
+
+    /** Reactivate the progress bar after having previously been deactivated.
+     * No-op if already active.
+     */
+    void reactivateBar();
 };
 
 Logger * makeProgressBar();
@@ -117,4 +127,10 @@ void startProgressBar();
 
 void stopProgressBar();
 
+/** Call ProgressBar::deactivateBar() iff nix::Logger is a progress bar logger. */
+void deactivateProgressBar();
+
+/** Call ProgressBar::reactivateBar() iff nix::Logger is a progress bar logger. */
+void reactivateProgressBar();
+
 }