tree-wide: unify progress bar inactive and paused states

Previously, the progress bar had two subtly different states in which the bar
would not actually render, both with their own shortcomings: inactive (which
was irreversible) and paused (reversible, but swallowing logs). Furthermore,
there was no way of resetting the statistics, so a very bad solution was
implemented (243c0f18dae2a08ea0e46f7ff33277c63f7506d7) that would create a new
logger for each line of the repl, leaking the previous one and discarding the
value of printBuildLogs. Finally, if stderr was not attached to a TTY, the
update thread was started even though the logger was not active, violating the
invariant required by the destructor (which is not observed because the logger
is leaked).

In this commit, the two aforementioned states are unified into a single one,
which can be exited again, correctly upholds the invariant that the update
thread is only running while the progress bar is active, and does not swallow
logs. The latter change in behavior is not expected to be a problems in the
rare cases where the paused state was used before, since other loggers (like
the simple one) don't exhibit it anyway. The startProgressBar/stopProgressBar
API is removed due to being a footgun, and a new method for properly resetting
the progress is added.

Co-Authored-By: Qyriad <qyriad@qyriad.me>
Change-Id: I2b7c3eb17d439cd0c16f7b896cfb61239ac7ff3a

3 files changed, 39 insertions, 52 deletions

diff --git a/src/libmain/progress-bar.cc b/src/libmain/progress-bar.cc
index 68654c636..e4afcd829 100644
--- a/src/libmain/progress-bar.cc
+++ b/src/libmain/progress-bar.cc
@@ -4,7 +4,6 @@
 #include "names.hh"
 #include "terminal.hh"
 
-#include <atomic>
 #include <map>
 #include <thread>
 #include <sstream>
@@ -44,50 +43,56 @@ 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));
-        }
-    });
+    resume();
 }
 
 ProgressBar::~ProgressBar()
 {
-    stop();
+    pause();
 }
 
-/* Called by destructor, can't be overridden */
-void ProgressBar::stop()
+void ProgressBar::pause()
 {
+    if (!isTTY) return;
     {
         auto state(state_.lock());
-        if (!state->active) return;
-        state->active = false;
-        writeToStderr("\r\e[K");
+        state->paused++;
+        if (state->paused > 1) return; // recursive pause, the update thread is already gone
         updateCV.notify_one();
         quitCV.notify_one();
     }
     updateThread.join();
 }
 
-void ProgressBar::pause()
+void ProgressBar::resetProgress()
 {
-    state_.lock()->paused = true;
-    writeToStderr("\r\e[K");
+    auto state(state_.lock());
+    auto prevPaused = state->paused;
+    *state = ProgressBar::State {
+        .paused = prevPaused,
+    };
+    update(*state);
 }
 
 void ProgressBar::resume()
 {
-    state_.lock()->paused = false;
-    writeToStderr("\r\e[K");
-    state_.lock()->haveUpdate = true;
-    updateCV.notify_one();
+    if (!isTTY) return;
+    auto state(state_.lock());
+    assert(state->paused > 0); // should be paused
+    state->paused--;
+    if (state->paused > 0) return; // recursive pause, wait for the parents to resume too
+    state->haveUpdate = true;
+    updateThread = std::thread([&]() {
+        auto state(state_.lock());
+        auto nextWakeup = A_LONG_TIME;
+        while (state->paused == 0) {
+            if (!state->haveUpdate)
+                state.wait_for(updateCV, nextWakeup);
+            nextWakeup = draw(*state, {});
+            state.wait_for(quitCV, std::chrono::milliseconds(50));
+        }
+        writeToStderr("\r\e[K");
+    });
 }
 
 bool ProgressBar::isVerbose()
@@ -114,7 +119,7 @@ void ProgressBar::logEI(const ErrorInfo & ei)
 
 void ProgressBar::log(State & state, Verbosity lvl, std::string_view s)
 {
-    if (state.active) {
+    if (state.paused == 0) {
         draw(state, s);
     } else {
         auto s2 = s + ANSI_NORMAL "\n";
@@ -318,7 +323,7 @@ std::chrono::milliseconds ProgressBar::draw(State & state, const std::optional<s
     auto nextWakeup = A_LONG_TIME;
 
     state.haveUpdate = false;
-    if (state.paused || !state.active) return nextWakeup;
+    if (state.paused > 0) return nextWakeup;
 
     auto windowSize = getWindowSize();
     auto width = windowSize.second;
@@ -525,7 +530,7 @@ std::string ProgressBar::getStatus(State & state)
 void ProgressBar::writeToStdout(std::string_view s)
 {
     auto state(state_.lock());
-    if (state->active) {
+    if (state->paused == 0) {
         Logger::writeToStdout(s);
         draw(*state, {});
     } else {
@@ -536,7 +541,7 @@ void ProgressBar::writeToStdout(std::string_view s)
 std::optional<char> ProgressBar::ask(std::string_view msg)
 {
     auto state(state_.lock());
-    if (!state->active || !isatty(STDIN_FILENO)) return {};
+    if (state->paused > 0 || !isatty(STDIN_FILENO)) return {};
     std::cerr << fmt("\r\e[K%s ", msg);
     auto s = trim(readLine(STDIN_FILENO));
     if (s.size() != 1) return {};
@@ -559,16 +564,4 @@ Logger * makeProgressBar()
     return new ProgressBar(shouldANSI());
 }
 
-void startProgressBar()
-{
-    logger = makeProgressBar();
-}
-
-void stopProgressBar()
-{
-    auto progressBar = dynamic_cast<ProgressBar *>(logger);
-    if (progressBar) progressBar->stop();
-
-}
-
 }
diff --git a/src/libmain/progress-bar.hh b/src/libmain/progress-bar.hh
index e682d75fe..ad500de6e 100644
--- a/src/libmain/progress-bar.hh
+++ b/src/libmain/progress-bar.hh
@@ -48,9 +48,8 @@ struct ProgressBar : public Logger
 
         uint64_t corruptedPaths = 0, untrustedPaths = 0;
 
-        bool active = true;
-        bool paused = false;
-        bool haveUpdate = true;
+        uint32_t paused = 1;
+        bool haveUpdate = false;
     };
 
     Sync<State> state_;
@@ -67,10 +66,10 @@ struct ProgressBar : public Logger
 
     ~ProgressBar();
 
-    void stop() override final;
-
     void pause() override;
 
+    void resetProgress() override;
+
     void resume() override;
 
     bool isVerbose() override;
@@ -113,8 +112,4 @@ struct ProgressBar : public Logger
 
 Logger * makeProgressBar();
 
-void startProgressBar();
-
-void stopProgressBar();
-
 }
diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc
index 29538a9ca..81ca204e3 100644
--- a/src/libmain/shared.cc
+++ b/src/libmain/shared.cc
@@ -4,7 +4,6 @@
 #include "gc-store.hh"
 #include "signals.hh"
 #include "loggers.hh"
-#include "progress-bar.hh"
 #include "current-process.hh"
 
 #include <algorithm>
@@ -349,7 +348,7 @@ RunPager::RunPager()
     if (!pager) pager = getenv("PAGER");
     if (pager && ((std::string) pager == "" || (std::string) pager == "cat")) return;
 
-    stopProgressBar();
+    logger->pause();
 
     Pipe toPager;
     toPager.create();