refactor: Signaling

include/components/controller.hpp

@@ -1,5 +1,7 @@
 #pragma once
 
+#include <csignal>
+
 #include "common.hpp"
 #include "components/config.hpp"
 #include "components/eventloop.hpp"
@@ -48,6 +50,8 @@ class controller {
 
   void wait_for_signal();
   void wait_for_xevent();
+  void wait_for_eventloop();
+  void wait_for_configwatch();
 
   void bootstrap_modules();
 
@@ -56,6 +60,22 @@ class controller {
   void on_unrecognized_action(string input);
   void on_update();
 
+ private:
+  enum class thread_role {
+    EVENT_QUEUE,
+    EVENT_QUEUE_X,
+    IPC_LISTENER,
+    CONF_LISTENER,
+  };
+
+  bool is_thread_joinable(thread_role&& role) {
+    if (m_threads.find(role) == m_threads.end()) {
+      return false;
+    } else {
+      return m_threads[role].joinable();
+    }
+  }
+
  private:
   connection& m_connection;
   registry m_registry{m_connection};
@@ -70,10 +90,9 @@ class controller {
   stateflag m_waiting{false};
   stateflag m_trayactivated{false};
 
+  sigset_t m_blockmask;
   sigset_t m_waitmask;
-  sigset_t m_ignmask;
+  map<thread_role, thread> m_threads;
-
-  vector<thread> m_threads;
 
   inotify_util::watch_t& m_confwatch;
   command_util::command_t m_command;

include/components/eventloop.hpp

@@ -25,6 +25,11 @@ struct event {
   char data[256]{'\0'};
 };
 
+struct quit_event {
+  const uint8_t type{static_cast<uint8_t>(event_type::QUIT)};
+  bool reload{false};
+};
+
 class eventloop {
  public:
   /**
@@ -35,8 +40,7 @@ class eventloop {
   using duration_t = chrono::duration<double, std::milli>;
 
   explicit eventloop(const logger& logger, const config& config);
-
+  ~eventloop();
-  ~eventloop() noexcept;
 
   void start();
   void wait();
@@ -64,7 +68,7 @@ class eventloop {
   void on_update();
   void on_input(char* input);
   void on_check();
-  void on_quit();
+  void on_quit(const quit_event& quit);
 
  private:
   const logger& m_log;
@@ -80,11 +84,6 @@ class eventloop {
    */
   modulemap_t m_modules;
 
-  /**
-   * @brief Lock used when accessing the module map
-   */
-  std::mutex m_modulelock;
-
   /**
    * @brief Flag to indicate current run state
    */

include/components/screen.hpp

@@ -34,7 +34,7 @@ class screen : public xpp::event::sink<evt::randr_screen_change_notify> {
   const config& m_conf;
 
   xcb_window_t m_root;
-  xcb_window_t m_proxy;
+  xcb_window_t m_proxy{XCB_NONE};
 
   struct size m_size{0U, 0U};
   bool m_sigraised{false};

include/components/signals.hpp

@@ -2,6 +2,7 @@
 
 #include "common.hpp"
 
+#include "components/eventloop.hpp"
 #include "utils/functional.hpp"
 
 POLYBAR_NS
@@ -17,13 +18,34 @@ enum class attribute : uint8_t;
 
 /**
  * @TODO: Allow multiple signal handlers
+ * @TODO: Encapsulate signals
  */
 namespace g_signals {
+  /**
+   * Helper used to create no-op "callbacks"
+   */
+  template <typename... T>
+  static callback<T...> noop = [](T...) {};
+
+  /**
+   * Signals used to communicate with the event loop
+   */
+  namespace event {
+    extern callback<const eventloop::entry_t&> enqueue;
+    extern callback<const eventloop::entry_t&> enqueue_delayed;
+  }
+
+  /**
+   * Signals used to communicate with the bar window
+   */
   namespace bar {
     extern callback<string> action_click;
     extern callback<const bool> visibility_change;
   }
 
+  /**
+   * Signals used to communicate with the input parser
+   */
   namespace parser {
     extern callback<const uint32_t> background_change;
     extern callback<const uint32_t> foreground_change;
@@ -42,6 +64,9 @@ namespace g_signals {
     extern callback<const char*, const size_t> string_write;
   }
 
+  /**
+   * Signals used to communicate with the tray manager
+   */
   namespace tray {
     extern callback<const uint16_t> report_slotcount;
   }

include/utils/inotify.hpp

@@ -25,7 +25,7 @@ namespace inotify_util {
     ~inotify_watch() noexcept;
 
     void attach(int mask = IN_MODIFY);
-    void remove();
+    void remove(bool force = false);
     bool poll(int wait_ms = 1000);
     unique_ptr<event_t> get_event();
     bool await_match();

src/components/config.cpp

@@ -69,7 +69,7 @@ void config::copy_inherited() {
 
         m_logger.trace("config: Copying missing params (sub=\"%s\", base=\"%s\")", section.first, inherit);
 
-        // Iterate the the base and copy the parameters
+        // Iterate the base and copy the parameters
         // that hasn't been defined for the sub-section
         for (auto&& base_param : *base_section) {
           if (!section.second.get_child_optional(base_param.first)) {

src/components/controller.cpp

@@ -1,5 +1,4 @@
 #include <chrono>
-#include <csignal>
 #include <mutex>
 
 #include "components/bar.hpp"
@@ -74,35 +73,42 @@ di::injector<unique_ptr<controller>> configure_controller(watch_t& confwatch) {
  * threads and spawned processes
  */
 controller::~controller() {
+  pthread_sigmask(SIG_UNBLOCK, &m_blockmask, nullptr);
+
+  sigset_t sig;
+  sigemptyset(&sig);
+  sigaddset(&sig, SIGALRM);
+  pthread_sigmask(SIG_BLOCK, &sig, nullptr);
+
   if (m_command) {
     m_log.info("Terminating running shell command");
     m_command.reset();
   }
 
-  if (m_eventloop) {
-    m_log.info("Deconstructing eventloop");
-    m_eventloop.reset();
-  }
-
-  if (m_ipc) {
-    m_log.info("Deconstructing ipc");
-    m_ipc.reset();
-  }
-
   if (m_bar) {
     m_log.info("Deconstructing bar");
     m_bar.reset();
   }
 
+  if (m_ipc) {
+    m_log.info("Deconstructing ipc");
+    m_ipc.reset();
+  }
+
+  if (m_eventloop) {
+    m_log.info("Deconstructing eventloop");
+    m_eventloop.reset();
+  }
+
+  if (!m_writeback) {
     m_log.info("Interrupting X event loop");
     m_connection.send_dummy_event(m_connection.root());
-
-  if (!m_threads.empty()) {
-    m_log.info("Joining active threads");
-    for (auto&& thread : m_threads) {
-      if (thread.joinable()) {
-        thread.join();
   }
+
+  m_log.info("Joining active threads");
+  for (auto&& th : m_threads) {
+    if (th.second.joinable()) {
+      th.second.join();
     }
   }
 
@@ -115,9 +121,16 @@ controller::~controller() {
 }
 
 /**
- * Setup X environment
+ * Initialize components and setup X environment
  */
 void controller::bootstrap(bool writeback, bool dump_wmname) {
+  // Add all signals to the block mask
+  sigfillset(&m_blockmask);
+
+  if (pthread_sigmask(SIG_BLOCK, &m_blockmask, nullptr) == -1) {
+    throw system_error("Failed to block signals");
+  }
+
   m_writeback = writeback;
 
   m_log.trace("controller: Initialize X atom cache");
@@ -171,37 +184,57 @@ void controller::run() {
   m_log.info("Starting application");
   m_running = true;
 
-  install_sigmask();
+  if (m_confwatch && !m_writeback) {
-  install_confwatch();
+    m_threads[thread_role::CONF_LISTENER] = thread(&controller::wait_for_configwatch, this);
+  }
 
   // Start ipc receiver if its enabled
   if (m_conf.get<bool>(m_conf.bar_section(), "enable-ipc", false)) {
-    m_threads.emplace_back(thread(&ipc::receive_messages, m_ipc.get()));
+    m_threads[thread_role::IPC_LISTENER] = thread(&ipc::receive_messages, m_ipc.get());
   }
 
   // Listen for X events in separate thread
   if (!m_writeback) {
-    m_threads.emplace_back(thread(&controller::wait_for_xevent, this));
+    m_threads[thread_role::EVENT_QUEUE_X] = thread(&controller::wait_for_xevent, this);
   }
 
-  // Wait for term signal in separate thread
-  m_threads.emplace_back(thread(&controller::wait_for_signal, this));
-
   // Start event loop
   if (m_eventloop) {
-    m_eventloop->start();
+    m_threads[thread_role::EVENT_QUEUE] = thread(&controller::wait_for_eventloop, this);
-    m_eventloop->wait();
   }
 
-  // Wake up signal thread
+  m_log.trace("controller: Wait for signal");
-  if (m_waiting) {
+  m_waiting = true;
-    kill(getpid(), SIGTERM);
-  }
 
-  uninstall_sigmask();
+  sigemptyset(&m_waitmask);
-  uninstall_confwatch();
+  sigaddset(&m_waitmask, SIGINT);
+  sigaddset(&m_waitmask, SIGQUIT);
+  sigaddset(&m_waitmask, SIGTERM);
+  sigaddset(&m_waitmask, SIGUSR1);
+  sigaddset(&m_waitmask, SIGALRM);
+
+  int caught_signal = 0;
+  sigwait(&m_waitmask, &caught_signal);
 
   m_running = false;
+  m_waiting = false;
+
+  if (caught_signal == SIGUSR1) {
+    m_reload = true;
+  }
+
+  m_log.warn("Termination signal received, shutting down...");
+  m_log.trace("controller: Caught signal %d", caught_signal);
+
+  if (m_eventloop) {
+    m_log.trace("controller: Stopping event loop");
+    m_eventloop->stop();
+  }
+
+  if (!m_writeback && m_confwatch) {
+    m_log.trace("controller: Removing config watch");
+    m_confwatch->remove(true);
+  }
 }
 
 /**
@@ -211,60 +244,11 @@ bool controller::completed() {
   return !m_running && !m_reload;
 }
 
-/**
- * Set signal mask for the current and future threads
- */
-void controller::install_sigmask() {
-  m_log.trace("controller: Set pthread_sigmask to block term signals");
-
-  sigemptyset(&m_waitmask);
-  sigaddset(&m_waitmask, SIGINT);
-  sigaddset(&m_waitmask, SIGQUIT);
-  sigaddset(&m_waitmask, SIGTERM);
-  sigaddset(&m_waitmask, SIGUSR1);
-
-  if (pthread_sigmask(SIG_BLOCK, &m_waitmask, nullptr) == -1) {
-    throw system_error();
-  }
-
-  sigemptyset(&m_ignmask);
-  sigaddset(&m_ignmask, SIGPIPE);
-
-  if (pthread_sigmask(SIG_BLOCK, &m_ignmask, nullptr) == -1) {
-    throw system_error();
-  }
-}
-
-/**
- * Uninstall sigmask to allow term signals
- */
-void controller::uninstall_sigmask() {
-  m_log.trace("controller: Set pthread_sigmask to unblock term signals");
-
-  if (pthread_sigmask(SIG_UNBLOCK, &m_waitmask, nullptr) == -1) {
-    throw system_error();
-  }
-}
-
 /**
  * Listen for changes to the config file
  */
-void controller::install_confwatch() {
+void controller::wait_for_configwatch() {
-  if (!m_running) {
-    return;
-  }
-
-  if (!m_confwatch) {
-    m_log.trace("controller: Config watch not set, skip...");
-    return;
-  }
-
-  m_threads.emplace_back([&] {
   try {
-      if (!m_running) {
-        return;
-      }
-
     m_log.trace("controller: Attach config watch");
     m_confwatch->attach(IN_MODIFY);
 
@@ -278,41 +262,6 @@ void controller::install_confwatch() {
     m_log.trace("controller: Reset config watch");
     m_confwatch.reset();
   }
-  });
-}
-
-/**
- * Remove the config inotify watch
- */
-void controller::uninstall_confwatch() {
-  try {
-    if (m_confwatch) {
-      m_log.info("Removing config watch");
-      m_confwatch->remove();
-    }
-  } catch (const system_error& err) {
-  }
-}
-
-/**
- * Wait for termination signal
- */
-void controller::wait_for_signal() {
-  m_log.trace("controller: Wait for signal");
-  m_waiting = true;
-
-  int caught_signal = 0;
-  sigwait(&m_waitmask, &caught_signal);
-
-  m_log.warn("Termination signal received, shutting down...");
-  m_log.trace("controller: Caught signal %d", caught_signal);
-
-  if (m_eventloop) {
-    m_eventloop->stop();
-  }
-
-  m_reload = (caught_signal == SIGUSR1);
-  m_waiting = false;
 }
 
 /**
@@ -341,6 +290,21 @@ void controller::wait_for_xevent() {
   }
 }
 
+/**
+ * Start event loop and wait for it to finish
+ */
+void controller::wait_for_eventloop() {
+  m_eventloop->start();
+  m_eventloop->wait();
+
+  this_thread::sleep_for(250ms);
+
+  if (m_running) {
+    m_log.trace("controller: eventloop ended, raising SIGALRM");
+    kill(getpid(), SIGALRM);
+  }
+}
+
 /**
  * Create and initialize bar modules
  */

src/components/eventloop.cpp

@@ -1,5 +1,8 @@
+#include <csignal>
+
 #include "components/eventloop.hpp"
 #include "components/types.hpp"
+#include "components/signals.hpp"
 #include "utils/string.hpp"
 #include "utils/time.hpp"
 #include "x11/color.hpp"
@@ -13,20 +16,27 @@ eventloop::eventloop(const logger& logger, const config& config) : m_log(logger)
   m_delayed_time = duration_t{m_conf.get<double>("settings", "eventloop-delayed-time", 25)};
   m_swallow_time = duration_t{m_conf.get<double>("settings", "eventloop-swallow-time", 10)};
   m_swallow_limit = m_conf.get<size_t>("settings", "eventloop-swallow", 5U);
+
+  g_signals::event::enqueue = bind(&eventloop::enqueue, this, placeholders::_1);
+  g_signals::event::enqueue_delayed = bind(&eventloop::enqueue_delayed, this, placeholders::_1);
 }
 
 /**
  * Deconstruct eventloop
  */
-eventloop::~eventloop() noexcept {
+eventloop::~eventloop() {
+  g_signals::event::enqueue = g_signals::noop<const eventloop::entry_t&>;
+  g_signals::event::enqueue_delayed = g_signals::noop<const eventloop::entry_t&>;
+
   m_update_cb = nullptr;
   m_unrecognized_input_cb = nullptr;
 
   if (m_delayed_thread.joinable()) {
     m_delayed_thread.join();
   }
-
+  if (m_queue_thread.joinable()) {
-  std::lock_guard<std::mutex> guard(m_modulelock, std::adopt_lock);
+    m_queue_thread.join();
+  }
 
   for (auto&& block : m_modules) {
     for (auto&& module : block.second) {
@@ -129,8 +139,6 @@ void eventloop::set_input_db(callback<string>&& cb) {
  * Add module to alignment block
  */
 void eventloop::add_module(const alignment pos, module_t&& module) {
-  std::lock_guard<std::mutex> guard(m_modulelock, std::adopt_lock);
-
   auto it = m_modules.lower_bound(pos);
 
   if (it != m_modules.end() && !(m_modules.key_comp()(pos, it->first))) {
@@ -164,8 +172,6 @@ size_t eventloop::module_count() const {
  * Start module threads
  */
 void eventloop::dispatch_modules() {
-  std::lock_guard<std::mutex> guard(m_modulelock);
-
   for (const auto& block : m_modules) {
     for (const auto& module : block.second) {
       try {
@@ -211,7 +217,7 @@ void eventloop::dispatch_queue_worker() {
     forward_event(evt);
   }
 
-  m_log.trace("eventloop: Reached end of queue worker thread");
+  m_log.info("Queue worker done");
 }
 
 /**
@@ -233,7 +239,7 @@ void eventloop::dispatch_delayed_worker() {
     m_delayed_entry.type = 0;
   }
 
-  m_log.trace("eventloop: Reached end of delayed worker thread");
+  m_log.info("Delayed worker done");
 }
 
 /**
@@ -267,7 +273,7 @@ void eventloop::forward_event(entry_t evt) {
   } else if (evt.type == static_cast<uint8_t>(event_type::CHECK)) {
     on_check();
   } else if (evt.type == static_cast<uint8_t>(event_type::QUIT)) {
-    on_quit();
+    on_quit(reinterpret_cast<const quit_event&>(evt));
   } else {
     m_log.warn("Unknown event type for enqueued event (%d)", evt.type);
   }
@@ -292,12 +298,6 @@ void eventloop::on_update() {
 void eventloop::on_input(char* input) {
   m_log.trace("eventloop: Received INPUT event");
 
-  if (!m_modulelock.try_lock()) {
-    return;
-  }
-
-  std::lock_guard<std::mutex> guard(m_modulelock, std::adopt_lock);
-
   for (auto&& block : m_modules) {
     for (auto&& module : block.second) {
       if (!module->receive_events()) {
@@ -320,34 +320,29 @@ void eventloop::on_input(char* input) {
  * Handler for enqueued CHECK events
  */
 void eventloop::on_check() {
-  if (!m_running) {
-    return;
-  }
-
-  if (!m_modulelock.try_lock()) {
-    return;
-  }
-
-  std::lock_guard<std::mutex> guard(m_modulelock, std::adopt_lock);
-
   for (const auto& block : m_modules) {
     for (const auto& module : block.second) {
-      if (module->running()) {
+      if (m_running && module->running()) {
         return;
       }
     }
   }
 
   m_log.warn("No running modules...");
+
   stop();
 }
 
 /**
  * Handler for enqueued QUIT events
  */
-void eventloop::on_quit() {
+void eventloop::on_quit(const quit_event& quit) {
   m_log.trace("eventloop: Received QUIT event");
   m_running = false;
+
+  if (quit.reload) {
+    kill(getpid(), SIGUSR1);
+  }
 }
 
 POLYBAR_NS_END

src/components/screen.cpp

@@ -5,6 +5,8 @@
 #include "components/logger.hpp"
 #include "components/screen.hpp"
 #include "components/types.hpp"
+#include "components/signals.hpp"
+#include "components/eventloop.hpp"
 #include "x11/connection.hpp"
 #include "x11/randr.hpp"
 #include "x11/winspec.hpp"
@@ -27,6 +29,8 @@ screen::screen(connection& conn, const logger& logger, const config& conf)
     , m_conf(conf)
     , m_root(conn.root())
     , m_size({conn.screen()->width_in_pixels, conn.screen()->height_in_pixels}) {
+  assert(g_signals::event::enqueue != nullptr);
+
   // Check if the reloading has been disabled by the user
   if (!m_conf.get<bool>("settings", "screenchange-reload", false)) {
     return;
@@ -65,7 +69,7 @@ screen::screen(connection& conn, const logger& logger, const config& conf)
 screen::~screen() {
   m_connection.detach_sink(this, 1);
 
-  if (m_proxy) {
+  if (m_proxy != XCB_NONE) {
     m_connection.destroy_window(m_proxy);
   }
 }
@@ -79,7 +83,9 @@ void screen::handle(const evt::randr_screen_change_notify& evt) {
   if (!m_sigraised && evt->request_window == m_proxy) {
     m_log.warn("randr_screen_change_notify (%ux%u)... reloading", evt->width, evt->height);
     m_sigraised = true;
-    kill(getpid(), SIGUSR1);
+    quit_event quit{};
+    quit.reload = true;
+    g_signals::event::enqueue(reinterpret_cast<const eventloop::entry_t&>(quit));
   }
 }
 

src/components/signals.cpp

@@ -3,34 +3,50 @@
 
 POLYBAR_NS
 
+namespace g_signals {
+  /**
+   * Signals used to communicate with the event loop
+   */
+  namespace event {
+    callback<const eventloop::entry_t&> enqueue{noop<const eventloop::entry_t&>};
+    callback<const eventloop::entry_t&> enqueue_delayed{noop<const eventloop::entry_t&>};
+  }
+
   /**
    * Signals used to communicate with the bar window
    */
-callback<const string> g_signals::bar::action_click{nullptr};
+  namespace bar {
-callback<const bool> g_signals::bar::visibility_change{nullptr};
+    callback<const string> action_click{noop<const string>};
+    callback<const bool> visibility_change{noop<const bool>};
+  }
 
   /**
    * Signals used to communicate with the input parser
    */
-callback<const uint32_t> g_signals::parser::background_change{nullptr};
+  namespace parser {
-callback<const uint32_t> g_signals::parser::foreground_change{nullptr};
+    callback<const uint32_t> background_change{noop<const uint32_t>};
-callback<const uint32_t> g_signals::parser::underline_change{nullptr};
+    callback<const uint32_t> foreground_change{noop<const uint32_t>};
-callback<const uint32_t> g_signals::parser::overline_change{nullptr};
+    callback<const uint32_t> underline_change{noop<const uint32_t>};
-callback<const alignment> g_signals::parser::alignment_change{nullptr};
+    callback<const uint32_t> overline_change{noop<const uint32_t>};
-callback<const attribute> g_signals::parser::attribute_set{nullptr};
+    callback<const alignment> alignment_change{noop<const alignment>};
-callback<const attribute> g_signals::parser::attribute_unset{nullptr};
+    callback<const attribute> attribute_set{noop<const attribute>};
-callback<const attribute> g_signals::parser::attribute_toggle{nullptr};
+    callback<const attribute> attribute_unset{noop<const attribute>};
-callback<const int8_t> g_signals::parser::font_change{nullptr};
+    callback<const attribute> attribute_toggle{noop<const attribute>};
-callback<const int16_t> g_signals::parser::pixel_offset{nullptr};
+    callback<const int8_t> font_change{noop<const int8_t>};
-callback<const mousebtn, const string> g_signals::parser::action_block_open{nullptr};
+    callback<const int16_t> pixel_offset{noop<const int16_t>};
-callback<const mousebtn> g_signals::parser::action_block_close{nullptr};
+    callback<const mousebtn, const string> action_block_open{noop<const mousebtn, const string>};
-callback<const uint16_t> g_signals::parser::ascii_text_write{nullptr};
+    callback<const mousebtn> action_block_close{noop<const mousebtn>};
-callback<const uint16_t> g_signals::parser::unicode_text_write{nullptr};
+    callback<const uint16_t> ascii_text_write{noop<const uint16_t>};
-callback<const char*, const size_t> g_signals::parser::string_write{nullptr};
+    callback<const uint16_t> unicode_text_write{noop<const uint16_t>};
+    callback<const char*, const size_t> string_write{noop<const char*, const size_t>};
+  }
 
   /**
    * Signals used to communicate with the tray manager
    */
-callback<const uint16_t> g_signals::tray::report_slotcount{nullptr};
+  namespace tray {
+    callback<const uint16_t> report_slotcount{noop<const uint16_t>};
+  }
+}
 
 POLYBAR_NS_END

src/utils/inotify.cpp

@@ -35,8 +35,8 @@ namespace inotify_util {
   /**
    * Remove inotify watch
    */
-  void inotify_watch::remove() {
+  void inotify_watch::remove(bool force) {
-    if (inotify_rm_watch(m_fd, m_wd) == -1) {
+    if (inotify_rm_watch(m_fd, m_wd) == -1 && !force) {
       throw system_error("Failed to remove inotify watch");
     }
     m_wd = -1;