From 75284a5263faa12e818a2918af750d2b28e5ffa2 Mon Sep 17 00:00:00 2001 From: Alois Wohlschlager Date: Mon, 24 Jun 2024 20:37:28 +0200 Subject: [PATCH 1/2] libmain/progress-bar: do not start the update thread when not on a TTY The progress bar normally maintains the invariant that the update thread is only running when the progress bar is active. This was violated when it is already created in inactive state, which happens when stderr is not a TTY. The update thread is superfluous and would cause std::terminate if the logger were to be destroyed. Change-Id: I6ed3e26cdba00e5ac316078fea272d3a1aa0d112 --- src/libmain/progress-bar.cc | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/libmain/progress-bar.cc b/src/libmain/progress-bar.cc index e36bc0b01..8c8b60099 100644 --- a/src/libmain/progress-bar.cc +++ b/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() From 491aff75628755ca2379dafa7aa14efadd84f695 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Mon, 24 Jun 2024 16:53:12 -0600 Subject: [PATCH 2/2] 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..5da2d69c9 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(logger); + if (progressBar) { + progressBar->deactivateBar(); + } +} + +void reactivateProgressBar() +{ + auto progressBar = dynamic_cast(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(); + }