Signal handling fixes

include/client.hpp

@@ -15,11 +15,16 @@ struct zwp_idle_inhibit_manager_v1;
 
 namespace waybar {
 
-class Client {
+class Client : public sigc::trackable {
  public:
   static Client *inst();
   int main(int argc, char *argv[]);
-  void reset();
+
+  /* signal handlers */
+  void handleSignal(int signum);
+  void reload(int signum = 0);
+  void toggle(int signum = 0);
+  void quit(int signum = 0);
 
   Glib::RefPtr<Gtk::Application> gtk_app;
   Glib::RefPtr<Gdk::Display> gdk_display;
@@ -54,8 +59,11 @@ class Client {
   Glib::RefPtr<Gtk::CssProvider> css_provider_;
   std::unique_ptr<Portal> portal;
   std::list<struct waybar_output> outputs_;
+  sigc::connection output_added_, output_removed_;
   std::unique_ptr<CssReloadHelper> m_cssReloadHelper;
   std::string m_cssFile;
+  std::string config_opt_;
+  std::string style_opt_;
 };
 
 }  // namespace waybar

include/util/command.hpp

@@ -15,8 +15,7 @@
 
 #include <array>
 
-extern std::mutex reap_mtx;
-extern std::list<pid_t> reap;
+#include "util/unix_signal.hpp"
 
 namespace waybar::util::command {
 
@@ -160,9 +159,7 @@ inline int32_t forkExec(const std::string& cmd) {
     execl("/bin/sh", "sh", "-c", cmd.c_str(), (char*)0);
     exit(0);
   } else {
-    reap_mtx.lock();
-    reap.push_back(pid);
-    reap_mtx.unlock();
+    signal_handler.addChild(pid);
     spdlog::debug("Added child to reap list: {}", pid);
   }
 

include/util/unix_signal.hpp

@@ -0,0 +1,45 @@
+#pragma once
+
+#include <csignal>
+#include <list>
+#include <map>
+#include <mutex>
+#include <thread>
+
+#include "util/SafeSignal.hpp"
+
+namespace waybar {
+
+class UnixSignalHandler {
+ public:
+  class SignalProxy : private sigc::signal<void(int)> {
+   public:
+    friend class UnixSignalHandler;
+
+    using sigc::signal<void(int)>::connect;
+  };
+
+  UnixSignalHandler();
+  ~UnixSignalHandler();
+
+  void addChild(pid_t pid);
+  SignalProxy& signal(int signum);
+
+  void start();
+
+ private:
+  void run();
+
+  sigset_t mask_;
+  std::thread thread_;
+  std::mutex reap_mtx_;
+  std::list<pid_t> reap_;
+  std::atomic<bool> running_;
+
+  SafeSignal<int> signal_;
+  std::map<int, SignalProxy> handlers_;
+};
+
+}  // namespace waybar
+
+extern waybar::UnixSignalHandler signal_handler;

meson.build

@@ -183,7 +183,8 @@ src_files = files(
     'src/util/rewrite_string.cpp',
     'src/util/gtk_icon.cpp',
     'src/util/regex_collection.cpp',
-    'src/util/css_reload_helper.cpp'
+    'src/util/css_reload_helper.cpp',
+    'src/util/unix_signal.cpp',
 )
 
 man_files = files(

src/client.cpp

@@ -1,12 +1,12 @@
 #include "client.hpp"
 
 #include <gtk-layer-shell.h>
+#include <gtkmm/icontheme.h>
 #include <spdlog/spdlog.h>
 
 #include <iostream>
 #include <utility>
 
-#include "gtkmm/icontheme.h"
 #include "idle-inhibit-unstable-v1-client-protocol.h"
 #include "util/clara.hpp"
 #include "util/format.hpp"
@@ -205,26 +205,16 @@ void waybar::Client::bindInterfaces() {
   if (xdg_output_manager == nullptr) {
     throw std::runtime_error("Failed to acquire required resources.");
   }
-  // add existing outputs and subscribe to updates
-  for (auto i = 0; i < gdk_display->get_n_monitors(); ++i) {
-    auto monitor = gdk_display->get_monitor(i);
-    handleMonitorAdded(monitor);
-  }
-  gdk_display->signal_monitor_added().connect(sigc::mem_fun(*this, &Client::handleMonitorAdded));
-  gdk_display->signal_monitor_removed().connect(
-      sigc::mem_fun(*this, &Client::handleMonitorRemoved));
 }
 
 int waybar::Client::main(int argc, char *argv[]) {
   bool show_help = false;
   bool show_version = false;
-  std::string config_opt;
-  std::string style_opt;
   std::string log_level;
   auto cli = clara::detail::Help(show_help) |
              clara::detail::Opt(show_version)["-v"]["--version"]("Show version") |
-             clara::detail::Opt(config_opt, "config")["-c"]["--config"]("Config path") |
-             clara::detail::Opt(style_opt, "style")["-s"]["--style"]("Style path") |
+             clara::detail::Opt(config_opt_, "config")["-c"]["--config"]("Config path") |
+             clara::detail::Opt(style_opt_, "style")["-s"]["--style"]("Style path") |
              clara::detail::Opt(
                  log_level,
                  "trace|debug|info|warning|error|critical|off")["-l"]["--log-level"]("Log level") |
@@ -260,40 +250,72 @@ int waybar::Client::main(int argc, char *argv[]) {
     throw std::runtime_error("Bar need to run under Wayland");
   }
   wl_display = gdk_wayland_display_get_wl_display(gdk_display->gobj());
-  config.load(config_opt);
-  if (!portal) {
-    portal = std::make_unique<waybar::Portal>();
-  }
-  m_cssFile = getStyle(style_opt);
-  setupCss(m_cssFile);
-  m_cssReloadHelper = std::make_unique<CssReloadHelper>(m_cssFile, [&]() { setupCss(m_cssFile); });
+
+  bindInterfaces();
+
+  portal = std::make_unique<waybar::Portal>();
   portal->signal_appearance_changed().connect([&](waybar::Appearance appearance) {
-    auto css_file = getStyle(style_opt, appearance);
+    auto css_file = getStyle(style_opt_, appearance);
     setupCss(css_file);
   });
 
-  auto m_config = config.getConfig();
-  if (m_config.isObject() && m_config["reload_style_on_change"].asBool()) {
+  reload();
+
+  gtk_app->hold();
+  gtk_app->run();
+  m_cssReloadHelper.reset();  // stop watching css file
+  bars.clear();
+  return 0;
+}
+
+void waybar::Client::handleSignal(int signum) {
+  spdlog::debug("RT signal {} received", signum);
+  for (auto &bar : bars) {
+    bar->handleSignal(signum);
+  }
+}
+
+void waybar::Client::reload(G_GNUC_UNUSED int signum) {
+  config.load(config_opt_);
+
+  m_cssFile = getStyle(style_opt_);
+  setupCss(m_cssFile);
+
+  m_cssReloadHelper = std::make_unique<CssReloadHelper>(m_cssFile, [&]() { setupCss(m_cssFile); });
+
+  auto confObj = config.getConfig();
+  if (confObj.isObject() && confObj["reload_style_on_change"].asBool()) {
     m_cssReloadHelper->monitorChanges();
-  } else if (m_config.isArray()) {
-    for (const auto &conf : m_config) {
+  } else if (confObj.isArray()) {
+    for (const auto &conf : confObj) {
       if (conf["reload_style_on_change"].asBool()) {
         m_cssReloadHelper->monitorChanges();
         break;
       }
     }
   }
 
-  bindInterfaces();
-  gtk_app->hold();
-  gtk_app->run();
-  m_cssReloadHelper.reset();  // stop watching css file
+  output_added_.disconnect();
+  output_removed_.disconnect();
   bars.clear();
-  return 0;
+  outputs_.clear();
+
+  // add existing outputs and subscribe to updates
+  for (auto i = 0; i < gdk_display->get_n_monitors(); ++i) {
+    auto monitor = gdk_display->get_monitor(i);
+    handleMonitorAdded(monitor);
+  }
+
+  output_added_ = gdk_display->signal_monitor_added().connect(
+      sigc::mem_fun(*this, &Client::handleMonitorAdded));
+  output_removed_ = gdk_display->signal_monitor_removed().connect(
+      sigc::mem_fun(*this, &Client::handleMonitorRemoved));
 }
 
-void waybar::Client::reset() {
-  gtk_app->quit();
-  // delete signal handler for css changes
-  portal->signal_appearance_changed().clear();
+void waybar::Client::toggle(G_GNUC_UNUSED int signum) {
+  for (auto &bar : bars) {
+    bar->toggle();
+  }
 }
+
+void waybar::Client::quit(G_GNUC_UNUSED int signum) { gtk_app->quit(); }

src/main.cpp

@@ -1,112 +1,23 @@
 #include <spdlog/spdlog.h>
-#include <sys/types.h>
-#include <sys/wait.h>
-
-#include <csignal>
-#include <list>
-#include <mutex>
 
 #include "client.hpp"
+#include "util/unix_signal.hpp"
 
-std::mutex reap_mtx;
-std::list<pid_t> reap;
-volatile bool reload;
-
-void* signalThread(void* args) {
-  int err;
-  int signum;
-  sigset_t mask;
-  sigemptyset(&mask);
-  sigaddset(&mask, SIGCHLD);
-
-  while (true) {
-    err = sigwait(&mask, &signum);
-    if (err != 0) {
-      spdlog::error("sigwait failed: {}", strerror(errno));
-      continue;
-    }
-
-    switch (signum) {
-      case SIGCHLD:
-        spdlog::debug("Received SIGCHLD in signalThread");
-        if (!reap.empty()) {
-          reap_mtx.lock();
-          for (auto it = reap.begin(); it != reap.end(); ++it) {
-            if (waitpid(*it, nullptr, WNOHANG) == *it) {
-              spdlog::debug("Reaped child with PID: {}", *it);
-              it = reap.erase(it);
-            }
-          }
-          reap_mtx.unlock();
-        }
-        break;
-      default:
-        spdlog::debug("Received signal with number {}, but not handling", signum);
-        break;
-    }
-  }
-}
-
-void startSignalThread() {
-  int err;
-  sigset_t mask;
-  sigemptyset(&mask);
-  sigaddset(&mask, SIGCHLD);
-
-  // Block SIGCHLD so it can be handled by the signal thread
-  // Any threads created by this one (the main thread) should not
-  // modify their signal mask to unblock SIGCHLD
-  err = pthread_sigmask(SIG_BLOCK, &mask, nullptr);
-  if (err != 0) {
-    spdlog::error("pthread_sigmask failed in startSignalThread: {}", strerror(err));
-    exit(1);
-  }
-
-  pthread_t thread_id;
-  err = pthread_create(&thread_id, nullptr, signalThread, nullptr);
-  if (err != 0) {
-    spdlog::error("pthread_create failed in startSignalThread: {}", strerror(err));
-    exit(1);
-  }
-}
+waybar::UnixSignalHandler signal_handler;
 
 int main(int argc, char* argv[]) {
   try {
     auto* client = waybar::Client::inst();
 
-    std::signal(SIGUSR1, [](int /*signal*/) {
-      for (auto& bar : waybar::Client::inst()->bars) {
-        bar->toggle();
-      }
-    });
-
-    std::signal(SIGUSR2, [](int /*signal*/) {
-      spdlog::info("Reloading...");
-      reload = true;
-      waybar::Client::inst()->reset();
-    });
-
-    std::signal(SIGINT, [](int /*signal*/) {
-      spdlog::info("Quitting.");
-      reload = false;
-      waybar::Client::inst()->reset();
-    });
-
+    signal_handler.signal(SIGUSR1).connect(sigc::mem_fun(*client, &waybar::Client::toggle));
+    signal_handler.signal(SIGUSR2).connect(sigc::mem_fun(*client, &waybar::Client::reload));
+    signal_handler.signal(SIGINT).connect(sigc::mem_fun(*client, &waybar::Client::quit));
     for (int sig = SIGRTMIN + 1; sig <= SIGRTMAX; ++sig) {
-      std::signal(sig, [](int sig) {
-        for (auto& bar : waybar::Client::inst()->bars) {
-          bar->handleSignal(sig);
-        }
-      });
+      signal_handler.signal(sig).connect(sigc::mem_fun(*client, &waybar::Client::handleSignal));
     }
-    startSignalThread();
-
-    auto ret = 0;
-    do {
-      reload = false;
-      ret = client->main(argc, argv);
-    } while (reload);
+    signal_handler.start();
 
+    auto ret = client->main(argc, argv);
     delete client;
     return ret;
   } catch (const std::exception& e) {