From c6540a895036428a135e59a813a837aefc08d32a Mon Sep 17 00:00:00 2001 From: Michael Carlberg Date: Thu, 1 Dec 2016 08:41:49 +0100 Subject: [PATCH] refactor: Signaling --- include/components/controller.hpp | 25 +++- include/components/eventloop.hpp | 15 +- include/components/screen.hpp | 2 +- include/components/signals.hpp | 25 ++++ include/utils/inotify.hpp | 2 +- src/components/config.cpp | 2 +- src/components/controller.cpp | 224 +++++++++++++----------------- src/components/eventloop.cpp | 51 +++---- src/components/screen.cpp | 10 +- src/components/signals.cpp | 70 ++++++---- src/utils/inotify.cpp | 4 +- 11 files changed, 227 insertions(+), 203 deletions(-) diff --git a/include/components/controller.hpp b/include/components/controller.hpp index d19f170e..f4ac032b 100644 --- a/include/components/controller.hpp +++ b/include/components/controller.hpp @@ -1,5 +1,7 @@ #pragma once +#include + #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; - - vector m_threads; + map m_threads; inotify_util::watch_t& m_confwatch; command_util::command_t m_command; diff --git a/include/components/eventloop.hpp b/include/components/eventloop.hpp index 9d7599d3..db2bb8c0 100644 --- a/include/components/eventloop.hpp +++ b/include/components/eventloop.hpp @@ -25,6 +25,11 @@ struct event { char data[256]{'\0'}; }; +struct quit_event { + const uint8_t type{static_cast(event_type::QUIT)}; + bool reload{false}; +}; + class eventloop { public: /** @@ -35,8 +40,7 @@ class eventloop { using duration_t = chrono::duration; explicit eventloop(const logger& logger, const config& config); - - ~eventloop() noexcept; + ~eventloop(); 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 */ diff --git a/include/components/screen.hpp b/include/components/screen.hpp index 89acff4f..8cb4af97 100644 --- a/include/components/screen.hpp +++ b/include/components/screen.hpp @@ -34,7 +34,7 @@ class screen : public xpp::event::sink { 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}; diff --git a/include/components/signals.hpp b/include/components/signals.hpp index 57750c14..72957235 100644 --- a/include/components/signals.hpp +++ b/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 + static callback noop = [](T...) {}; + + /** + * Signals used to communicate with the event loop + */ + namespace event { + extern callback enqueue; + extern callback enqueue_delayed; + } + + /** + * Signals used to communicate with the bar window + */ namespace bar { extern callback action_click; extern callback visibility_change; } + /** + * Signals used to communicate with the input parser + */ namespace parser { extern callback background_change; extern callback foreground_change; @@ -42,6 +64,9 @@ namespace g_signals { extern callback string_write; } + /** + * Signals used to communicate with the tray manager + */ namespace tray { extern callback report_slotcount; } diff --git a/include/utils/inotify.hpp b/include/utils/inotify.hpp index 458f5ea6..bd868ce0 100644 --- a/include/utils/inotify.hpp +++ b/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 get_event(); bool await_match(); diff --git a/src/components/config.cpp b/src/components/config.cpp index de33a0a0..b9359e8b 100644 --- a/src/components/config.cpp +++ b/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)) { diff --git a/src/components/controller.cpp b/src/components/controller.cpp index 19bcabda..58fee357 100644 --- a/src/components/controller.cpp +++ b/src/components/controller.cpp @@ -1,5 +1,4 @@ #include -#include #include #include "components/bar.hpp" @@ -74,35 +73,42 @@ di::injector> 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(); } - m_log.info("Interrupting X event loop"); - m_connection.send_dummy_event(m_connection.root()); + if (m_ipc) { + m_log.info("Deconstructing ipc"); + m_ipc.reset(); + } - if (!m_threads.empty()) { - m_log.info("Joining active threads"); - for (auto&& thread : m_threads) { - if (thread.joinable()) { - thread.join(); - } + 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()); + } + + 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(); - install_confwatch(); + if (m_confwatch && !m_writeback) { + m_threads[thread_role::CONF_LISTENER] = thread(&controller::wait_for_configwatch, this); + } // Start ipc receiver if its enabled if (m_conf.get(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_eventloop->wait(); + m_threads[thread_role::EVENT_QUEUE] = thread(&controller::wait_for_eventloop, this); } - // Wake up signal thread - if (m_waiting) { - kill(getpid(), SIGTERM); - } + m_log.trace("controller: Wait for signal"); + m_waiting = true; - uninstall_sigmask(); - uninstall_confwatch(); + sigemptyset(&m_waitmask); + 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,110 +244,26 @@ 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() { - 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); - - m_log.trace("controller: Wait for config file inotify event"); - if (m_confwatch->await_match() && m_running) { - m_log.info("Configuration file changed"); - kill(getpid(), SIGUSR1); - } - } catch (const system_error& err) { - m_log.err(err.what()); - m_log.trace("controller: Reset config watch"); - m_confwatch.reset(); - } - }); -} - -/** - * Remove the config inotify watch - */ -void controller::uninstall_confwatch() { +void controller::wait_for_configwatch() { try { - if (m_confwatch) { - m_log.info("Removing config watch"); - m_confwatch->remove(); + m_log.trace("controller: Attach config watch"); + m_confwatch->attach(IN_MODIFY); + + m_log.trace("controller: Wait for config file inotify event"); + if (m_confwatch->await_match() && m_running) { + m_log.info("Configuration file changed"); + kill(getpid(), SIGUSR1); } } catch (const system_error& err) { + m_log.err(err.what()); + m_log.trace("controller: Reset config watch"); + m_confwatch.reset(); } } -/** - * 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; -} - /** * Wait for X events and forward them to * the event registry @@ -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 */ diff --git a/src/components/eventloop.cpp b/src/components/eventloop.cpp index 008ea750..d7e1e87e 100644 --- a/src/components/eventloop.cpp +++ b/src/components/eventloop.cpp @@ -1,5 +1,8 @@ +#include + #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("settings", "eventloop-delayed-time", 25)}; m_swallow_time = duration_t{m_conf.get("settings", "eventloop-swallow-time", 10)}; m_swallow_limit = m_conf.get("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; + g_signals::event::enqueue_delayed = g_signals::noop; + m_update_cb = nullptr; m_unrecognized_input_cb = nullptr; if (m_delayed_thread.joinable()) { m_delayed_thread.join(); } - - std::lock_guard guard(m_modulelock, std::adopt_lock); + if (m_queue_thread.joinable()) { + m_queue_thread.join(); + } for (auto&& block : m_modules) { for (auto&& module : block.second) { @@ -129,8 +139,6 @@ void eventloop::set_input_db(callback&& cb) { * Add module to alignment block */ void eventloop::add_module(const alignment pos, module_t&& module) { - std::lock_guard 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 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(event_type::CHECK)) { on_check(); } else if (evt.type == static_cast(event_type::QUIT)) { - on_quit(); + on_quit(reinterpret_cast(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 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 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 diff --git a/src/components/screen.cpp b/src/components/screen.cpp index 32222835..a92f0a96 100644 --- a/src/components/screen.cpp +++ b/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("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(quit)); } } diff --git a/src/components/signals.cpp b/src/components/signals.cpp index b2b4bea4..184d0e31 100644 --- a/src/components/signals.cpp +++ b/src/components/signals.cpp @@ -3,34 +3,50 @@ POLYBAR_NS -/** - * Signals used to communicate with the bar window - */ -callback g_signals::bar::action_click{nullptr}; -callback g_signals::bar::visibility_change{nullptr}; +namespace g_signals { + /** + * Signals used to communicate with the event loop + */ + namespace event { + callback enqueue{noop}; + callback enqueue_delayed{noop}; + } -/** - * Signals used to communicate with the input parser - */ -callback g_signals::parser::background_change{nullptr}; -callback g_signals::parser::foreground_change{nullptr}; -callback g_signals::parser::underline_change{nullptr}; -callback g_signals::parser::overline_change{nullptr}; -callback g_signals::parser::alignment_change{nullptr}; -callback g_signals::parser::attribute_set{nullptr}; -callback g_signals::parser::attribute_unset{nullptr}; -callback g_signals::parser::attribute_toggle{nullptr}; -callback g_signals::parser::font_change{nullptr}; -callback g_signals::parser::pixel_offset{nullptr}; -callback g_signals::parser::action_block_open{nullptr}; -callback g_signals::parser::action_block_close{nullptr}; -callback g_signals::parser::ascii_text_write{nullptr}; -callback g_signals::parser::unicode_text_write{nullptr}; -callback g_signals::parser::string_write{nullptr}; + /** + * Signals used to communicate with the bar window + */ + namespace bar { + callback action_click{noop}; + callback visibility_change{noop}; + } -/** - * Signals used to communicate with the tray manager - */ -callback g_signals::tray::report_slotcount{nullptr}; + /** + * Signals used to communicate with the input parser + */ + namespace parser { + callback background_change{noop}; + callback foreground_change{noop}; + callback underline_change{noop}; + callback overline_change{noop}; + callback alignment_change{noop}; + callback attribute_set{noop}; + callback attribute_unset{noop}; + callback attribute_toggle{noop}; + callback font_change{noop}; + callback pixel_offset{noop}; + callback action_block_open{noop}; + callback action_block_close{noop}; + callback ascii_text_write{noop}; + callback unicode_text_write{noop}; + callback string_write{noop}; + } + + /** + * Signals used to communicate with the tray manager + */ + namespace tray { + callback report_slotcount{noop}; + } +} POLYBAR_NS_END diff --git a/src/utils/inotify.cpp b/src/utils/inotify.cpp index be93e355..c457e4f6 100644 --- a/src/utils/inotify.cpp +++ b/src/utils/inotify.cpp @@ -35,8 +35,8 @@ namespace inotify_util { /** * Remove inotify watch */ - void inotify_watch::remove() { - if (inotify_rm_watch(m_fd, m_wd) == -1) { + void inotify_watch::remove(bool force) { + if (inotify_rm_watch(m_fd, m_wd) == -1 && !force) { throw system_error("Failed to remove inotify watch"); } m_wd = -1;