From 444120e664b68761e5a024ebfdd710ff65872476 Mon Sep 17 00:00:00 2001
From: Patrick Ziegler <p.ziegler96@gmail.com>
Date: Sun, 3 Oct 2021 01:27:11 +0200
Subject: [PATCH] script: Fix concurrency issues (#2518)

Fixes #1978

* Move tail and non-tail handler to method

Defining them in the constructor is ugly.

* script: Iterate over defined actions instead of fixed list

* Separate running logic and lock m_output

* Include POLYBAR_FLAGS in linker flags

* Stop using m_prev in script_runner

* Join module threads in stop function

Joining in the destructor may lead to UB because the subclass is already
deconstructed but the threads may still require it to be around (e.g.
for calling any functions on the instance)

* Cleanup script module

* Update changelog

* Remove AfterReturn class

* Remove m_stopping from script module

* Fix polybar not reading the entire line from child process.

For every `readline` call we created a new fd_streambuf. This means once
`readline` returns, the streambuf is destructed and and pending data in
its temporary buffer discarded and we never actually read it.

* Remove unused includes
---
 CHANGELOG.md                       |   2 +
 cmake/cxx.cmake                    |   4 +-
 include/adapters/script_runner.hpp |  57 +++++++++++
 include/components/eventloop.hpp   |   2 +-
 include/modules/meta/base.inl      |  32 +++---
 include/modules/script.hpp         |  21 +---
 include/utils/command.hpp          |  25 ++---
 include/utils/file.hpp             |  11 ++-
 include/utils/io.hpp               |  15 +--
 src/CMakeLists.txt                 |   2 +
 src/adapters/script_runner.cpp     | 154 +++++++++++++++++++++++++++++
 src/modules/script.cpp             | 154 ++++++-----------------------
 src/utils/command.cpp              |  26 +++--
 src/utils/file.cpp                 |  21 ++--
 src/utils/io.cpp                   |  65 +-----------
 tests/unit_tests/utils/command.cpp |  14 +--
 16 files changed, 316 insertions(+), 289 deletions(-)
 create mode 100644 include/adapters/script_runner.hpp
 create mode 100644 src/adapters/script_runner.cpp

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4d03cedc..04063b28 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -143,6 +143,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Increased the double click interval from 150ms to 400ms.
 
 ### Fixed
+- `custom/script`: Concurrency issues with fast-updating tailed scripts.
+  ([`#1978`](https://github.com/polybar/polybar/issues/1978))
 - Trailing space after the layout label when indicators are empty and made sure right amount
   of spacing is added between the indicator labels, in the xkeyboard module.
   ([`#2292`](https://github.com/polybar/polybar/issues/2292))
diff --git a/cmake/cxx.cmake b/cmake/cxx.cmake
index 128b12ca..fa6776af 100644
--- a/cmake/cxx.cmake
+++ b/cmake/cxx.cmake
@@ -99,11 +99,11 @@ elseif (CMAKE_BUILD_TYPE_UPPER STREQUAL "COVERAGE")
   list(APPEND cxx_flags ${cxx_coverage})
 endif()
 
-list(APPEND cxx_linker_flags ${cxx_flags})
-
 string(REPLACE " " ";" polybar_flags_list "${POLYBAR_FLAGS}")
 list(APPEND cxx_flags ${polybar_flags_list})
 
+list(APPEND cxx_linker_flags ${cxx_flags})
+
 string(REPLACE ";" " " cxx_flags_str "${cxx_flags}")
 string(REPLACE ";" " " cxx_linker_flags_str "${cxx_linker_flags}")
 
diff --git a/include/adapters/script_runner.hpp b/include/adapters/script_runner.hpp
new file mode 100644
index 00000000..9ecec1c1
--- /dev/null
+++ b/include/adapters/script_runner.hpp
@@ -0,0 +1,57 @@
+#pragma once
+
+#include <atomic>
+#include <chrono>
+#include <mutex>
+
+#include "common.hpp"
+#include "components/logger.hpp"
+
+POLYBAR_NS
+
+class script_runner {
+ public:
+  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);
+
+  bool check_condition() const;
+  interval process();
+
+  void clear_output();
+
+  void stop();
+
+  int get_pid() const;
+  int get_counter() const;
+
+  string get_output();
+
+  bool is_stopping() const;
+
+ protected:
+  bool set_output(const string&&);
+
+  interval run_tail();
+  interval run();
+
+ private:
+  const logger& m_log;
+
+  const std::function<void(void)> m_on_update;
+
+  const string m_exec;
+  const string m_exec_if;
+  const bool m_tail;
+  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};
+  std::atomic_bool m_stopping{false};
+  std::atomic_int m_pid{-1};
+};
+
+POLYBAR_NS_END
diff --git a/include/components/eventloop.hpp b/include/components/eventloop.hpp
index 26f030be..553bdff4 100644
--- a/include/components/eventloop.hpp
+++ b/include/components/eventloop.hpp
@@ -70,7 +70,7 @@ struct UVHandleGeneric {
     return unpackedThis->func(std::forward<Args>(args)...);
   }
 
-  H* handle;
+  H* handle{nullptr};
   function<void(Args...)> func;
 };
 
diff --git a/include/modules/meta/base.inl b/include/modules/meta/base.inl
index c7742744..182a2ab2 100644
--- a/include/modules/meta/base.inl
+++ b/include/modules/meta/base.inl
@@ -20,6 +20,8 @@ namespace modules {
       , m_bar(bar)
       , m_log(logger::make())
       , m_conf(config::make())
+      // TODO this cast is illegal because 'this' is not yet of type Impl but only of type module<Impl>
+      // Change action router to use lambdas
       , m_router(make_unique<action_router<Impl>>(CAST_MOD(Impl)))
       , m_name("module/" + name)
       , m_name_raw(name)
@@ -27,22 +29,21 @@ namespace modules {
       , m_formatter(make_unique<module_formatter>(m_conf, m_name))
       , m_handle_events(m_conf.get(m_name, "handle-events", true))
       , m_visible(!m_conf.get(m_name, "hidden", false)) {
-        m_router->register_action(EVENT_MODULE_TOGGLE, &module<Impl>::action_module_toggle);
-        m_router->register_action(EVENT_MODULE_SHOW, &module<Impl>::action_module_show);
-        m_router->register_action(EVENT_MODULE_HIDE, &module<Impl>::action_module_hide);
-      }
+    m_router->register_action(EVENT_MODULE_TOGGLE, &module<Impl>::action_module_toggle);
+    m_router->register_action(EVENT_MODULE_SHOW, &module<Impl>::action_module_show);
+    m_router->register_action(EVENT_MODULE_HIDE, &module<Impl>::action_module_hide);
+  }
 
   template <typename Impl>
   module<Impl>::~module() noexcept {
     m_log.trace("%s: Deconstructing", name());
 
-    for (auto&& thread_ : m_threads) {
-      if (thread_.joinable()) {
-        thread_.join();
-      }
-    }
-    if (m_mainthread.joinable()) {
-      m_mainthread.join();
+    if (running()) {
+      /*
+       * We can't stop in the destructor because we have to call the subclasses which at this point already have been
+       * destructed.
+       */
+      m_log.err("%s: Module was not stopped before deconstructing.", name());
     }
   }
 
@@ -87,6 +88,15 @@ namespace modules {
       CAST_MOD(Impl)->wakeup();
       CAST_MOD(Impl)->teardown();
 
+      for (auto&& thread_ : m_threads) {
+        if (thread_.joinable()) {
+          thread_.join();
+        }
+      }
+      if (m_mainthread.joinable()) {
+        m_mainthread.join();
+      }
+
       m_sig.emit(signals::eventqueue::check_state{});
     }
   }
diff --git a/include/modules/script.hpp b/include/modules/script.hpp
index 0ce8bea1..c00dcdbf 100644
--- a/include/modules/script.hpp
+++ b/include/modules/script.hpp
@@ -1,5 +1,6 @@
 #pragma once
 
+#include "adapters/script_runner.hpp"
 #include "modules/meta/base.hpp"
 #include "utils/command.hpp"
 #include "utils/io.hpp"
@@ -10,7 +11,6 @@ namespace modules {
   class script_module : public module<script_module> {
    public:
     explicit script_module(const bar_settings&, string);
-    ~script_module() {}
 
     void start() override;
     void stop() override;
@@ -21,32 +21,19 @@ namespace modules {
     static constexpr auto TYPE = "custom/script";
 
    protected:
-    chrono::duration<double> process(const mutex_wrapper<function<chrono::duration<double>()>>& handler) const;
     bool check_condition();
 
    private:
     static constexpr const char* TAG_LABEL{"<label>"};
 
-    mutex_wrapper<function<chrono::duration<double>()>> m_handler;
+    const bool m_tail;
+    const script_runner::interval m_interval{0};
 
-    unique_ptr<command<output_policy::REDIRECTED>> m_command;
+    script_runner m_runner;
 
-    vector<pair<string, string>> m_env;
-
-    bool m_tail;
-
-    string m_exec;
-    string m_exec_if;
-
-    chrono::duration<double> m_interval{0};
     map<mousebtn, string> m_actions;
 
     label_t m_label;
-    string m_output;
-    string m_prev;
-    int m_counter{0};
-
-    bool m_stopping{false};
   };
 }  // namespace modules
 
diff --git a/include/utils/command.hpp b/include/utils/command.hpp
index 956e885d..e8e67fe4 100644
--- a/include/utils/command.hpp
+++ b/include/utils/command.hpp
@@ -1,7 +1,5 @@
 #pragma once
 
-#include <mutex>
-
 #include "common.hpp"
 #include "components/logger.hpp"
 #include "components/types.hpp"
@@ -10,6 +8,9 @@
 
 POLYBAR_NS
 
+template <typename T>
+class fd_stream;
+
 DEFINE_ERROR(command_error);
 
 /**
@@ -30,15 +31,6 @@ DEFINE_ERROR(command_error);
  * \endcode
  *
  * \code cpp
- *   auto cmd = command_util::make_command<output_policy::REDIRECTED>(
- *    "while read -r line; do echo data from parent process: $line; done");
- *   cmd->exec(false);
- *   cmd->writeline("Test");
- *   cout << cmd->readline();
- *   cmd->wait();
- * \endcode
- *
- * \code cpp
  *   auto cmd = command_util::make_command<output_policy::REDIRECTED>("for i in 1 2 3; do echo $i; done");
  *   cmd->exec();
  *   cout << cmd->readline(); // 1
@@ -75,8 +67,8 @@ class command<output_policy::IGNORED> {
 
   string m_cmd;
 
-  pid_t m_forkpid{};
-  int m_forkstatus = -1;
+  pid_t m_forkpid{-1};
+  int m_forkstatus{-1};
 };
 
 template <>
@@ -97,17 +89,16 @@ class command<output_policy::REDIRECTED> : private command<output_policy::IGNORE
   using command<output_policy::IGNORED>::get_exit_status;
 
   void tail(callback<string> cb);
-  int writeline(string data);
   string readline();
 
   int get_stdout(int c);
   int get_stdin(int c);
 
  protected:
-  int m_stdout[2]{};
-  int m_stdin[2]{};
+  int m_stdout[2]{0, 0};
+  int m_stdin[2]{0, 0};
 
-  std::mutex m_pipelock{};
+  unique_ptr<fd_stream<std::istream>> m_stdout_reader{nullptr};
 };
 
 namespace command_util {
diff --git a/include/utils/file.hpp b/include/utils/file.hpp
index c0d5b251..a76e73db 100644
--- a/include/utils/file.hpp
+++ b/include/utils/file.hpp
@@ -34,8 +34,8 @@ class fd_streambuf : public std::streambuf {
 
   template <typename... Args>
   explicit fd_streambuf(Args&&... args) : m_fd(forward<Args>(args)...) {
-    setg(m_in, m_in, m_in);
-    setp(m_out, m_out + bufsize - 1);
+    setg(m_in, m_in + BUFSIZE_IN, m_in + BUFSIZE_IN);
+    setp(m_out, m_out + BUFSIZE_OUT - 1);
   }
   ~fd_streambuf();
 
@@ -51,10 +51,11 @@ class fd_streambuf : public std::streambuf {
   int underflow() override;
 
  private:
+  static constexpr int BUFSIZE_OUT = 1024;
+  static constexpr int BUFSIZE_IN = 1024;
   file_descriptor m_fd;
-  enum { bufsize = 1024 };
-  char m_out[bufsize];
-  char m_in[bufsize - 1];
+  char m_out[BUFSIZE_OUT];
+  char m_in[BUFSIZE_IN];
 };
 
 template <typename StreamType>
diff --git a/include/utils/io.hpp b/include/utils/io.hpp
index 921e9d12..64d9e6c1 100644
--- a/include/utils/io.hpp
+++ b/include/utils/io.hpp
@@ -5,23 +5,10 @@
 POLYBAR_NS
 
 namespace io_util {
-  string read(int read_fd, size_t bytes_to_read);
-  string readline(int read_fd);
-
-  size_t write(int write_fd, size_t bytes_to_write, const string& data);
-  size_t writeline(int write_fd, const string& data);
-
   void tail(int read_fd, const function<void(string)>& callback);
-  void tail(int read_fd, int writeback_fd);
 
   bool poll(int fd, short int events, int timeout_ms = 0);
   bool poll_read(int fd, int timeout_ms = 0);
-  bool poll_write(int fd, int timeout_ms = 0);
-
-  bool interrupt_read(int write_fd);
-
-  void set_block(int fd);
-  void set_nonblock(int fd);
-}
+}  // namespace io_util
 
 POLYBAR_NS_END
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 31f08d19..0a50ecf6 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -54,6 +54,8 @@ if(BUILD_LIBPOLY)
   set(POLY_SOURCES
     ${CMAKE_BINARY_DIR}/generated-sources/settings.cpp
 
+    ${src_dir}/adapters/script_runner.cpp
+
     ${src_dir}/cairo/utils.cpp
 
     ${src_dir}/components/bar.cpp
diff --git a/src/adapters/script_runner.cpp b/src/adapters/script_runner.cpp
new file mode 100644
index 00000000..e922c263
--- /dev/null
+++ b/src/adapters/script_runner.cpp
@@ -0,0 +1,154 @@
+#include "adapters/script_runner.hpp"
+
+#include <cassert>
+#include <functional>
+
+#include "modules/meta/base.hpp"
+#include "utils/command.hpp"
+#include "utils/io.hpp"
+#include "utils/scope.hpp"
+#include "utils/string.hpp"
+
+POLYBAR_NS
+
+script_runner::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)
+    : m_log(logger::make())
+    , m_on_update(on_update)
+    , m_exec(exec)
+    , m_exec_if(exec_if)
+    , m_tail(tail)
+    , m_interval(interval)
+    , m_env(env) {}
+
+/**
+ * Check if defined condition is met
+ */
+bool script_runner::check_condition() const {
+  if (m_exec_if.empty()) {
+    return true;
+  }
+
+  auto exec_if_cmd = command_util::make_command<output_policy::IGNORED>(m_exec_if);
+  return exec_if_cmd->exec(true) == 0;
+}
+
+/**
+ * Process mutex wrapped script handler
+ */
+script_runner::interval script_runner::process() {
+  if (m_tail) {
+    return run_tail();
+  } else {
+    return run();
+  }
+}
+
+void script_runner::clear_output() {
+  set_output("");
+}
+
+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;
+}
+
+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;
+}
+
+/**
+ * Updates the current output.
+ *
+ * Returns true if the output changed.
+ */
+bool script_runner::set_output(const 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();
+    return true;
+  }
+
+  return false;
+}
+
+script_runner::interval script_runner::run() {
+  auto exec = string_util::replace_all(m_exec, "%counter%", to_string(++m_counter));
+  m_log.info("script_runner: Invoking shell command: \"%s\"", exec);
+  auto cmd = command_util::make_command<output_policy::REDIRECTED>(exec);
+
+  try {
+    cmd->exec(true, m_env);
+  } catch (const exception& err) {
+    m_log.err("script_runner: %s", err.what());
+    throw modules::module_error("Failed to execute command, stopping module...");
+  }
+
+  int status = cmd->get_exit_status();
+  int fd = cmd->get_stdout(PIPE_READ);
+  assert(fd != -1);
+  bool changed = io_util::poll_read(fd) && set_output(cmd->readline());
+
+  if (!changed && status != 0) {
+    clear_output();
+  }
+
+  if (status == 0) {
+    return m_interval;
+  } else {
+    return std::max(m_interval, interval{1s});
+  }
+}
+
+script_runner::interval script_runner::run_tail() {
+  auto exec = string_util::replace_all(m_exec, "%counter%", to_string(++m_counter));
+  m_log.info("script_runner: Invoking shell command: \"%s\"", exec);
+  auto cmd = command_util::make_command<output_policy::REDIRECTED>(exec);
+
+  try {
+    cmd->exec(false, m_env);
+  } catch (const exception& err) {
+    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();
+
+  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());
+    }
+  }
+
+  if (m_stopping) {
+    cmd->terminate();
+    return 0s;
+  }
+
+  bool exit_status = cmd->wait();
+
+  if (exit_status == 0) {
+    return m_interval;
+  } else {
+    return std::max(m_interval, interval{1s});
+  }
+}
+
+POLYBAR_NS_END
diff --git a/src/modules/script.cpp b/src/modules/script.cpp
index a8e4d361..983ac8c9 100644
--- a/src/modules/script.cpp
+++ b/src/modules/script.cpp
@@ -6,88 +6,12 @@
 POLYBAR_NS
 
 namespace modules {
-  template class module<script_module>;
-
-  /**
-   * Construct script module by loading configuration values
-   * and setting up formatting objects
-   */
   script_module::script_module(const bar_settings& bar, string name_)
-      : module<script_module>(bar, move(name_)), m_handler([&]() -> function<chrono::duration<double>()> {
-        m_tail = m_conf.get(name(), "tail", false);
-        // Handler for continuous tail commands {{{
-
-        if (m_tail) {
-          return [&] {
-            if (!m_command || !m_command->is_running()) {
-              string exec{string_util::replace_all(m_exec, "%counter%", to_string(++m_counter))};
-              m_log.info("%s: Invoking shell command: \"%s\"", name(), exec);
-              m_command = command_util::make_command<output_policy::REDIRECTED>(exec);
-
-              try {
-                m_command->exec(false, m_env);
-              } catch (const exception& err) {
-                m_log.err("%s: %s", name(), err.what());
-                throw module_error("Failed to execute command, stopping module...");
-              }
-            }
-
-            int fd = m_command->get_stdout(PIPE_READ);
-            while (!m_stopping && fd != -1 && m_command->is_running() && !io_util::poll(fd, POLLHUP, 0)) {
-              if (!io_util::poll_read(fd, 25)) {
-                continue;
-              } else if ((m_output = m_command->readline()) != m_prev) {
-                m_prev = m_output;
-                broadcast();
-              }
-            }
-
-            if (m_stopping) {
-              return chrono::duration<double>{0};
-            } else if (m_command && !m_command->is_running()) {
-              return std::max(m_command->get_exit_status() == 0 ? m_interval : 1s, m_interval);
-            } else {
-              return m_interval;
-            }
-          };
-        }
-
-        // }}}
-        // Handler for basic shell commands {{{
-
-        return [&] {
-          try {
-            auto exec = string_util::replace_all(m_exec, "%counter%", to_string(++m_counter));
-            m_log.info("%s: Invoking shell command: \"%s\"", name(), exec);
-            m_command = command_util::make_command<output_policy::REDIRECTED>(exec);
-            m_command->exec(true, m_env);
-          } catch (const exception& err) {
-            m_log.err("%s: %s", name(), err.what());
-            throw module_error("Failed to execute command, stopping module...");
-          }
-
-          int fd = m_command->get_stdout(PIPE_READ);
-          if (fd != -1 && io_util::poll_read(fd) && (m_output = m_command->readline()) != m_prev) {
-            broadcast();
-            m_prev = m_output;
-          } else if (m_command->get_exit_status() != 0) {
-            m_output.clear();
-            m_prev.clear();
-            broadcast();
-          }
-
-          return std::max(m_command->get_exit_status() == 0 ? m_interval : 1s, m_interval);
-        };
-
-        // }}}
-      }()) {
-    // Load configuration values
-    m_exec = m_conf.get(name(), "exec", m_exec);
-    m_exec_if = m_conf.get(name(), "exec-if", m_exec_if);
-    m_interval = m_conf.get<decltype(m_interval)>(name(), "interval", m_tail ? 0s : 5s);
-
-    m_env = m_conf.get_with_prefix(name(), "env-");
-
+      : 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-")) {
     // 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);
@@ -111,14 +35,20 @@ namespace modules {
   void script_module::start() {
     m_mainthread = thread([&] {
       try {
-        while (running() && !m_stopping) {
-          if (check_condition()) {
-            sleep(process(m_handler));
-          } else if (m_interval > 1s) {
-            sleep(m_interval);
+        while (running()) {
+          script_runner::interval sleep_time;
+          if (m_runner.check_condition()) {
+            sleep_time = m_runner.process();
           } else {
-            sleep(1s);
+            m_runner.clear_output();
+            sleep_time = std::max(m_interval, script_runner::interval(1s));
           }
+
+          if (m_runner.is_stopping()) {
+            break;
+          }
+
+          sleep(sleep_time);
         }
       } catch (const exception& err) {
         halt(err.what());
@@ -130,61 +60,32 @@ namespace modules {
    * Stop the module worker by terminating any running commands
    */
   void script_module::stop() {
-    m_stopping = true;
+    m_runner.stop();
     wakeup();
 
-    std::lock_guard<decltype(m_handler)> guard(m_handler);
-
-    m_command.reset();
     module::stop();
   }
 
-  /**
-   * Check if defined condition is met
-   */
-  bool script_module::check_condition() {
-    if (m_exec_if.empty()) {
-      return true;
-    } else if (command_util::make_command<output_policy::IGNORED>(m_exec_if)->exec(true) == 0) {
-      return true;
-    } else if (!m_output.empty()) {
-      broadcast();
-      m_output.clear();
-      m_prev.clear();
-    }
-    return false;
-  }
-
-  /**
-   * Process mutex wrapped script handler
-   */
-  chrono::duration<double> script_module::process(const decltype(m_handler)& handler) const {
-    std::lock_guard<decltype(handler)> guard(handler);
-    return handler();
-  }
-
   /**
    * Generate module output
    */
   string script_module::get_output() {
-    if (m_output.empty()) {
+    auto script_output = m_runner.get_output();
+    if (script_output.empty()) {
       return "";
     }
 
     if (m_label) {
       m_label->reset_tokens();
-      m_label->replace_token("%output%", m_output);
+      m_label->replace_token("%output%", script_output);
     }
 
-    string cnt{to_string(m_counter)};
+    string cnt{to_string(m_runner.get_counter())};
     string output{module::get_output()};
 
-    for (auto btn : {mousebtn::LEFT, mousebtn::MIDDLE, mousebtn::RIGHT,
-                     mousebtn::DOUBLE_LEFT, mousebtn::DOUBLE_MIDDLE,
-                     mousebtn::DOUBLE_RIGHT, mousebtn::SCROLL_UP,
-                     mousebtn::SCROLL_DOWN}) {
-
-      auto action = m_actions[btn];
+    for (const auto& a : m_actions) {
+      auto btn = a.first;
+      auto action = a.second;
 
       if (!action.empty()) {
         auto action_replaced = string_util::replace_all(action, "%counter%", cnt);
@@ -193,8 +94,9 @@ namespace modules {
          * The pid token is only for tailed commands.
          * If the command is not specified or running, replacement is unnecessary as well
          */
-        if (m_tail && m_command && m_command->is_running()) {
-          action_replaced = string_util::replace_all(action_replaced, "%pid%", to_string(m_command->get_pid()));
+        int pid = m_runner.get_pid();
+        if (pid != -1) {
+          action_replaced = string_util::replace_all(action_replaced, "%pid%", to_string(pid));
         }
         m_builder->action(btn, action_replaced);
       }
diff --git a/src/utils/command.cpp b/src/utils/command.cpp
index fa234788..2f6d9d43 100644
--- a/src/utils/command.cpp
+++ b/src/utils/command.cpp
@@ -6,11 +6,12 @@
 #include <csignal>
 #include <cstdlib>
 #include <utility>
-#include <utils/string.hpp>
 
 #include "errors.hpp"
+#include "utils/file.hpp"
 #include "utils/io.hpp"
 #include "utils/process.hpp"
+#include "utils/string.hpp"
 
 #ifndef STDOUT_FILENO
 #define STDOUT_FILENO 1
@@ -24,9 +25,7 @@ POLYBAR_NS
 command<output_policy::IGNORED>::command(const logger& logger, string cmd) : m_log(logger), m_cmd(move(cmd)) {}
 
 command<output_policy::IGNORED>::~command() {
-  if (is_running()) {
-    terminate();
-  }
+  terminate();
 }
 
 /**
@@ -186,23 +185,20 @@ int command<output_policy::REDIRECTED>::exec(bool wait_for_completion, const vec
  * end until the stream is closed
  */
 void command<output_policy::REDIRECTED>::tail(callback<string> cb) {
-  io_util::tail(m_stdout[PIPE_READ], cb);
-}
-
-/**
- * Write line to command input channel
- */
-int command<output_policy::REDIRECTED>::writeline(string data) {
-  std::lock_guard<std::mutex> lck(m_pipelock);
-  return static_cast<int>(io_util::writeline(m_stdin[PIPE_WRITE], data));
+  io_util::tail(get_stdout(PIPE_READ), cb);
 }
 
 /**
  * Read a line from the commands output stream
  */
 string command<output_policy::REDIRECTED>::readline() {
-  std::lock_guard<std::mutex> lck(m_pipelock);
-  return io_util::readline(m_stdout[PIPE_READ]);
+  if (!m_stdout_reader) {
+    m_stdout_reader = make_unique<fd_stream<std::istream>>(get_stdout(PIPE_READ), false);
+  }
+
+  string s;
+  std::getline(*m_stdout_reader, s);
+  return s;
 }
 
 /**
diff --git a/src/utils/file.cpp b/src/utils/file.cpp
index dc8de71d..c6ecdfa7 100644
--- a/src/utils/file.cpp
+++ b/src/utils/file.cpp
@@ -90,8 +90,8 @@ void fd_streambuf::open(int fd) {
     close();
   }
   m_fd = fd;
-  setg(m_in, m_in, m_in);
-  setp(m_out, m_out + bufsize - 1);
+  setg(m_in, m_in + BUFSIZE_IN, m_in + BUFSIZE_IN);
+  setp(m_out, m_out + BUFSIZE_OUT - 1);
 }
 
 void fd_streambuf::close() {
@@ -123,13 +123,18 @@ int fd_streambuf::overflow(int c) {
 }
 
 int fd_streambuf::underflow() {
-  if (gptr() == egptr()) {
-    std::streamsize pback(std::min(gptr() - eback(), std::ptrdiff_t(16 - sizeof(int))));
-    std::copy(egptr() - pback, egptr(), eback());
-    int bytes(read(m_fd, eback() + pback, bufsize));
-    setg(eback(), eback() + pback, eback() + pback + std::max(0, bytes));
+  if (gptr() >= egptr()) {
+    int bytes = ::read(m_fd, m_in, BUFSIZE_IN);
+
+    if (bytes <= 0) {
+      setg(eback(), egptr(), egptr());
+      return traits_type::eof();
+    }
+
+    setg(m_in, m_in, m_in + bytes);
   }
-  return gptr() == egptr() ? traits_type::eof() : traits_type::to_int_type(*gptr());
+
+  return traits_type::to_int_type(*gptr());
 }
 
 // }}}
diff --git a/src/utils/io.cpp b/src/utils/io.cpp
index 8e3dc017..b2915988 100644
--- a/src/utils/io.cpp
+++ b/src/utils/io.cpp
@@ -1,49 +1,20 @@
+#include "utils/io.hpp"
+
 #include <fcntl.h>
 #include <poll.h>
 #include <unistd.h>
+
 #include <cstdio>
 #include <cstdlib>
 #include <iomanip>
 
 #include "errors.hpp"
 #include "utils/file.hpp"
-#include "utils/io.hpp"
 #include "utils/string.hpp"
 
 POLYBAR_NS
 
 namespace io_util {
-  string read(int read_fd, size_t bytes_to_read) {
-    fd_stream<std::istream> in(read_fd, false);
-    char buffer[BUFSIZ];
-    in.getline(buffer, bytes_to_read);
-    string out{buffer};
-    return out;
-  }
-
-  string readline(int read_fd) {
-    fd_stream<std::istream> in(read_fd, false);
-    string out;
-    std::getline(in, out);
-    return out;
-  }
-
-  size_t write(int write_fd, size_t bytes_to_write, const string& data) {
-    fd_stream<std::ostream> out(write_fd, false);
-    out.write(data.c_str(), bytes_to_write).flush();
-    return out.good() ? data.size() : 0;
-  }
-
-  size_t writeline(int write_fd, const string& data) {
-    fd_stream<std::ostream> out(write_fd, false);
-    if (data[data.size() - 1] != '\n') {
-      out << data << std::endl;
-    } else {
-      out << data << std::flush;
-    }
-    return out.good() ? data.size() : 0;
-  }
-
   void tail(int read_fd, const function<void(string)>& callback) {
     string line;
     fd_stream<std::istream> in(read_fd, false);
@@ -52,10 +23,6 @@ namespace io_util {
     }
   }
 
-  void tail(int read_fd, int writeback_fd) {
-    tail(read_fd, [&](string data) { io_util::writeline(writeback_fd, data); });
-  }
-
   bool poll(int fd, short int events, int timeout_ms) {
     struct pollfd fds[1];
     fds[0].fd = fd;
@@ -67,30 +34,6 @@ namespace io_util {
   bool poll_read(int fd, int timeout_ms) {
     return poll(fd, POLLIN, timeout_ms);
   }
-
-  bool poll_write(int fd, int timeout_ms) {
-    return poll(fd, POLLOUT, timeout_ms);
-  }
-
-  bool interrupt_read(int write_fd) {
-    return write(write_fd, 1, {'\n'}) > 0;
-  }
-
-  void set_block(int fd) {
-    int flags = fcntl(fd, F_GETFL, 0);
-    flags &= ~O_NONBLOCK;
-    if (fcntl(fd, F_SETFL, flags) == -1) {
-      throw system_error("Failed to unset O_NONBLOCK");
-    }
-  }
-
-  void set_nonblock(int fd) {
-    int flags = fcntl(fd, F_GETFL, 0);
-    flags |= O_NONBLOCK;
-    if (fcntl(fd, F_SETFL, flags) == -1) {
-      throw system_error("Failed to set O_NONBLOCK");
-    }
-  }
-}
+}  // namespace io_util
 
 POLYBAR_NS_END
diff --git a/tests/unit_tests/utils/command.cpp b/tests/unit_tests/utils/command.cpp
index 2283515e..4f7388dd 100644
--- a/tests/unit_tests/utils/command.cpp
+++ b/tests/unit_tests/utils/command.cpp
@@ -1,8 +1,9 @@
 #include "utils/command.hpp"
-#include "common/test.hpp"
 
 #include <unistd.h>
 
+#include "common/test.hpp"
+
 using namespace polybar;
 
 TEST(Command, status) {
@@ -44,14 +45,3 @@ TEST(Command, output) {
 
   EXPECT_EQ(str, "polybar");
 }
-
-TEST(Command, readline) {
-  auto cmd = command_util::make_command<output_policy::REDIRECTED>("read text;echo $text");
-
-  string str;
-  cmd->exec(false);
-  cmd->writeline("polybar");
-  cmd->tail([&str](string&& string) { str = string; });
-
-  EXPECT_EQ(str, "polybar");
-}