From 41d41ca8df10c94315110d5cda79f00ca23c79ab Mon Sep 17 00:00:00 2001
From: Maxim Kolesnikov <indev.src@gmail.com>
Date: Sun, 3 Apr 2022 18:11:13 +0700
Subject: [PATCH] fix(script): output timing inconsistencies (#2667)

Fixes #2650
---
 CHANGELOG.md                       |  1 +
 include/adapters/script_runner.hpp | 29 +++++++------
 include/modules/script.hpp         |  8 +++-
 src/adapters/script_runner.cpp     | 66 ++++++++++++++++--------------
 src/modules/script.cpp             | 36 +++++++++++-----
 5 files changed, 82 insertions(+), 58 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 50a763e0..10761c2b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Positioning in awesomeWM ([`#2651`](https://github.com/polybar/polybar/pull/2651))
 - `internal/xworkspaces`: The module sometimes crashed polybar when windows were closed. ([`#2655`](https://github.com/polybar/polybar/pull/2655))
 - Mouseover error when only one cursor is defined ([`#2656`](https://github.com/polybar/polybar/pull/2656))
+- Timing inconsistencies with `custom/script` output ([`#2650`](https://github.com/polybar/polybar/issues/2650), first described at [`#2630`](https://github.com/polybar/polybar/pull/2630))
 
 ## [3.6.1] - 2022-03-05
 ### Build
diff --git a/include/adapters/script_runner.hpp b/include/adapters/script_runner.hpp
index a2322e50..5bb5a616 100644
--- a/include/adapters/script_runner.hpp
+++ b/include/adapters/script_runner.hpp
@@ -11,9 +11,18 @@ POLYBAR_NS
 
 class script_runner {
  public:
+  struct data {
+    int counter{0};
+    int pid{-1};
+    int exit_status{0};
+    string output;
+  };
+
+  using on_update = std::function<void(const data&)>;
   using interval = std::chrono::duration<double>;
-  script_runner(std::function<void(void)> on_update, const string& exec, const string& exec_if, bool tail,
-      interval interval, const vector<pair<string, string>>& env);
+
+  script_runner(on_update on_update, const string& exec, const string& exec_if, bool tail, interval interval,
+      const vector<pair<string, string>>& env);
 
   bool check_condition() const;
   interval process();
@@ -22,16 +31,11 @@ class script_runner {
 
   void stop();
 
-  int get_pid() const;
-  int get_counter() const;
-  int get_exit_status() const;
-
-  string get_output();
-
   bool is_stopping() const;
 
  protected:
   bool set_output(string&&);
+  bool set_exit_status(int);
 
   interval run_tail();
   interval run();
@@ -39,7 +43,7 @@ class script_runner {
  private:
   const logger& m_log;
 
-  const std::function<void(void)> m_on_update;
+  const on_update m_on_update;
 
   const string m_exec;
   const string m_exec_if;
@@ -47,13 +51,8 @@ class script_runner {
   const interval m_interval;
   const vector<pair<string, string>> m_env;
 
-  std::mutex m_output_lock;
-  string m_output;
-
-  std::atomic_int m_counter{0};
+  data m_data;
   std::atomic_bool m_stopping{false};
-  std::atomic_int m_pid{-1};
-  std::atomic_int m_exit_status{0};
 };
 
 POLYBAR_NS_END
diff --git a/include/modules/script.hpp b/include/modules/script.hpp
index 94308249..47c20a85 100644
--- a/include/modules/script.hpp
+++ b/include/modules/script.hpp
@@ -26,6 +26,8 @@ namespace modules {
     bool check_condition();
 
    private:
+    void handle_runner_update(const script_runner::data&);
+
     static constexpr auto TAG_LABEL = "<label>";
     static constexpr auto TAG_LABEL_FAIL = "<label-fail>";
     static constexpr auto FORMAT_FAIL = "format-fail";
@@ -39,7 +41,11 @@ namespace modules {
 
     label_t m_label;
     label_t m_label_fail;
+
+    int m_exit_status{0};
+    script_runner::data m_data;
+    std::mutex m_data_mutex;
   };
-}  // namespace modules
+} // namespace modules
 
 POLYBAR_NS_END
diff --git a/src/adapters/script_runner.cpp b/src/adapters/script_runner.cpp
index ce8fef73..23a791f4 100644
--- a/src/adapters/script_runner.cpp
+++ b/src/adapters/script_runner.cpp
@@ -11,7 +11,7 @@
 
 POLYBAR_NS
 
-script_runner::script_runner(std::function<void(void)> on_update, const string& exec, const string& exec_if, bool tail,
+script_runner::script_runner(on_update on_update, const string& exec, const string& exec_if, bool tail,
     interval interval, const vector<pair<string, string>>& env)
     : m_log(logger::make())
     , m_on_update(on_update)
@@ -52,23 +52,6 @@ void script_runner::stop() {
   m_stopping = true;
 }
 
-int script_runner::get_pid() const {
-  return m_pid;
-}
-
-int script_runner::get_counter() const {
-  return m_counter;
-}
-
-int script_runner::get_exit_status() const {
-  return m_exit_status;
-}
-
-string script_runner::get_output() {
-  std::lock_guard<std::mutex> guard(m_output_lock);
-  return m_output;
-}
-
 bool script_runner::is_stopping() const {
   return m_stopping;
 }
@@ -79,19 +62,28 @@ bool script_runner::is_stopping() const {
  * Returns true if the output changed.
  */
 bool script_runner::set_output(string&& new_output) {
-  std::lock_guard<std::mutex> guard(m_output_lock);
-
-  if (m_output != new_output) {
-    m_output = std::move(new_output);
-    m_on_update();
+  if (m_data.output != new_output) {
+    m_data.output = std::move(new_output);
     return true;
   }
 
   return false;
 }
 
+/**
+ * Updates the current exit status
+ *
+ * Returns true if the exit status changed.
+ */
+bool script_runner::set_exit_status(int new_status) {
+  auto changed = (m_data.exit_status != new_status);
+  m_data.exit_status = new_status;
+
+  return changed;
+}
+
 script_runner::interval script_runner::run() {
-  auto exec = string_util::replace_all(m_exec, "%counter%", to_string(++m_counter));
+  auto exec = string_util::replace_all(m_exec, "%counter%", to_string(++m_data.counter));
   m_log.info("script_runner: Invoking shell command: \"%s\"", exec);
   auto cmd = command_util::make_command<output_policy::REDIRECTED>(exec);
 
@@ -124,13 +116,17 @@ script_runner::interval script_runner::run() {
     return 0s;
   }
 
-  m_exit_status = cmd->wait();
+  auto exit_status_changed = set_exit_status(cmd->wait());
 
-  if (!changed && m_exit_status != 0) {
+  if (!changed && m_data.exit_status != 0) {
     clear_output();
   }
 
-  if (m_exit_status == 0) {
+  if (changed || exit_status_changed) {
+    m_on_update(m_data);
+  }
+
+  if (m_data.exit_status == 0) {
     return m_interval;
   } else {
     return std::max(m_interval, interval{1s});
@@ -138,7 +134,7 @@ script_runner::interval script_runner::run() {
 }
 
 script_runner::interval script_runner::run_tail() {
-  auto exec = string_util::replace_all(m_exec, "%counter%", to_string(++m_counter));
+  auto exec = string_util::replace_all(m_exec, "%counter%", to_string(++m_data.counter));
   m_log.info("script_runner: Invoking shell command: \"%s\"", exec);
   auto cmd = command_util::make_command<output_policy::REDIRECTED>(exec);
 
@@ -148,15 +144,23 @@ script_runner::interval script_runner::run_tail() {
     throw modules::module_error("Failed to execute command: " + string(err.what()));
   }
 
-  auto pid_guard = scope_util::make_exit_handler([this]() { m_pid = -1; });
-  m_pid = cmd->get_pid();
+  auto pid_guard = scope_util::make_exit_handler([this]() {
+    m_data.pid = -1;
+    m_on_update(m_data);
+  });
+
+  m_data.pid = cmd->get_pid();
 
   int fd = cmd->get_stdout(PIPE_READ);
   assert(fd != -1);
 
   while (!m_stopping && cmd->is_running() && !io_util::poll(fd, POLLHUP, 0)) {
     if (io_util::poll_read(fd, 25)) {
-      set_output(cmd->readline());
+      auto changed = set_output(cmd->readline());
+
+      if (changed) {
+        m_on_update(m_data);
+      }
     }
   }
 
diff --git a/src/modules/script.cpp b/src/modules/script.cpp
index ec9c856c..6c3f1f58 100644
--- a/src/modules/script.cpp
+++ b/src/modules/script.cpp
@@ -10,8 +10,8 @@ namespace modules {
       : module<script_module>(bar, move(name_))
       , m_tail(m_conf.get(name(), "tail", false))
       , m_interval(m_conf.get<script_runner::interval>(name(), "interval", m_tail ? 0s : 5s))
-      , m_runner([this]() { broadcast(); }, m_conf.get(name(), "exec", ""s), m_conf.get(name(), "exec-if", ""s), m_tail,
-            m_interval, m_conf.get_with_prefix(name(), "env-")) {
+      , m_runner([this](const auto& data) { handle_runner_update(data); }, m_conf.get(name(), "exec", ""s),
+            m_conf.get(name(), "exec-if", ""s), m_tail, m_interval, m_conf.get_with_prefix(name(), "env-")) {
     // Load configured click handlers
     m_actions[mousebtn::LEFT] = m_conf.get(name(), "click-left", ""s);
     m_actions[mousebtn::MIDDLE] = m_conf.get(name(), "click-middle", ""s);
@@ -76,29 +76,35 @@ namespace modules {
    * Generate module output
    */
   string script_module::get_format() const {
-    if (m_runner.get_exit_status() != 0 && m_conf.has(name(), FORMAT_FAIL)) {
+    if (m_exit_status != 0 && m_conf.has(name(), FORMAT_FAIL)) {
       return FORMAT_FAIL;
     }
     return DEFAULT_FORMAT;
   }
 
   string script_module::get_output() {
-    auto script_output = m_runner.get_output();
-    if (script_output.empty()) {
+    auto data = [this]{
+      std::lock_guard<std::mutex> lk(m_data_mutex);
+      return m_data;
+    }();
+
+    m_exit_status = data.exit_status;
+
+    if (data.output.empty()) {
       return "";
     }
 
     if (m_label) {
       m_label->reset_tokens();
-      m_label->replace_token("%output%", script_output);
+      m_label->replace_token("%output%", data.output);
     }
 
     if (m_label_fail) {
       m_label_fail->reset_tokens();
-      m_label_fail->replace_token("%output%", script_output);
+      m_label_fail->replace_token("%output%", data.output);
     }
 
-    string cnt{to_string(m_runner.get_counter())};
+    string cnt{to_string(data.counter)};
     string output{module::get_output()};
 
     for (const auto& a : m_actions) {
@@ -112,9 +118,8 @@ namespace modules {
          * The pid token is only for tailed commands.
          * If the command is not specified or running, replacement is unnecessary as well
          */
-        int pid = m_runner.get_pid();
-        if (pid != -1) {
-          action_replaced = string_util::replace_all(action_replaced, "%pid%", to_string(pid));
+        if (data.pid != -1) {
+          action_replaced = string_util::replace_all(action_replaced, "%pid%", to_string(data.pid));
         }
         m_builder->action(btn, action_replaced);
       }
@@ -139,6 +144,15 @@ namespace modules {
 
     return true;
   }
+
+  void script_module::handle_runner_update(const script_runner::data& data) {
+    {
+      std::lock_guard<std::mutex> lk(m_data_mutex);
+      m_data = data;
+    }
+
+    broadcast();
+  }
 } // namespace modules
 
 POLYBAR_NS_END