From c2f087225c7804a821040a04ce85c8a19677cfb7 Mon Sep 17 00:00:00 2001 From: Patrick Ziegler Date: Sat, 22 Jan 2022 22:00:26 +0100 Subject: [PATCH] Eventloop cleanup (#2577) * eventloop: Use eventloop namespace in cpp files * changelog: Add missing deprecated hook message * Make eventloop and ipc classes non-copyable and non-movable * Remove functional.hpp * eventloop: Don't close handles in error cases Client should be responsible for closing handles. * eventloop: Address invalidation of handle references --- CHANGELOG.md | 2 ++ include/cairo/font.hpp | 20 ++++++++++---------- include/components/bar.hpp | 6 +++--- include/components/controller.hpp | 6 +++--- include/components/eventloop.hpp | 13 +++++++++---- include/events/signal.hpp | 1 - include/ipc/ipc.hpp | 15 ++++++++------- include/modules/meta/base.hpp | 1 - include/utils/command.hpp | 3 +-- include/utils/scope.hpp | 3 --- src/components/bar.cpp | 11 ++++++----- src/components/controller.cpp | 24 ++++++++++++++---------- src/components/eventloop.cpp | 12 +++++------- src/ipc/ipc.cpp | 26 ++++++++++++++------------ src/main.cpp | 3 ++- src/polybar-msg.cpp | 18 ++++++++---------- src/utils/command.cpp | 2 +- 17 files changed, 86 insertions(+), 80 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b12f450..08627b21 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -74,6 +74,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Directly writing ipc messages to `/tmp/polybar_mqueue.` is deprecated, users should always use `polybar-msg`. As a consequence the message format used for IPC is deprecated as well. +- `polybar-msg hook` is deprecated in favor of using the hook action. + `polybar-msg` will tell you the correct command to use. ### Removed - `DEBUG_SHADED` cmake variable and its associated functionality. diff --git a/include/cairo/font.hpp b/include/cairo/font.hpp index 1681a1bd..4a45dbdb 100644 --- a/include/cairo/font.hpp +++ b/include/cairo/font.hpp @@ -5,6 +5,7 @@ #include "cairo/types.hpp" #include "cairo/utils.hpp" #include "common.hpp" +#include "components/logger.hpp" #include "errors.hpp" #include "settings.hpp" #include "utils/math.hpp" @@ -55,7 +56,8 @@ namespace cairo { */ class font_fc : public font { public: - explicit font_fc(cairo_t* cairo, FcPattern* pattern, double offset, double dpi_x, double dpi_y) : font(cairo, offset), m_pattern(pattern) { + explicit font_fc(cairo_t* cairo, FcPattern* pattern, double offset, double dpi_x, double dpi_y) + : font(cairo, offset), m_pattern(pattern) { cairo_matrix_t fm; cairo_matrix_t ctm; cairo_matrix_init_scale(&fm, size(dpi_x), size(dpi_y)); @@ -139,8 +141,8 @@ namespace cairo { property(FC_PIXEL_SIZE, &fc_pixelsize); // Fall back to a default value if the size is 0 - double pixelsize = fc_pixelsize == 0? 10 : fc_pixelsize; - double size = fc_size == 0? 10 : fc_size; + double pixelsize = fc_pixelsize == 0 ? 10 : fc_pixelsize; + double size = fc_size == 0 ? 10 : fc_size; // Font size in pixels if we use the pixelsize property int px_pixelsize = pixelsize + 0.5; @@ -155,7 +157,7 @@ namespace cairo { int px_size = size / 72.0 * dpi + 0.5; if (fc_size == 0 && fc_pixelsize == 0) { - return scalable? px_size : px_pixelsize; + return scalable ? px_size : px_pixelsize; } if (scalable) { @@ -165,8 +167,7 @@ namespace cairo { */ if (fc_size != 0) { return px_size; - } - else { + } else { return px_pixelsize; } } else { @@ -176,8 +177,7 @@ namespace cairo { */ if (fc_pixelsize != 0) { return px_pixelsize; - } - else { + } else { return px_size; } } @@ -314,7 +314,7 @@ namespace cairo { auto pattern = FcNameParse((FcChar8*)fontname.c_str()); - if(!pattern) { + if (!pattern) { logger::make().err("Could not parse font \"%s\"", fontname); throw application_error("Could not parse font \"" + fontname + "\""); } @@ -336,6 +336,6 @@ namespace cairo { return make_shared(cairo, match, offset, dpi_x, dpi_y); } -} +} // namespace cairo POLYBAR_NS_END diff --git a/include/components/bar.hpp b/include/components/bar.hpp index 81ad1df7..ea0091c8 100644 --- a/include/components/bar.hpp +++ b/include/components/bar.hpp @@ -63,9 +63,9 @@ class bar : public xpp::event::sink { public: using make_type = unique_ptr; - static make_type make(eventloop::eventloop&, bool only_initialize_values = false); + static make_type make(eventloop::loop&, bool only_initialize_values = false); - explicit bar(connection&, signal_emitter&, const config&, const logger&, eventloop::eventloop&, unique_ptr&&, + explicit bar(connection&, signal_emitter&, const config&, const logger&, eventloop::loop&, unique_ptr&&, unique_ptr&&, unique_ptr&&, unique_ptr&&, bool only_initialize_values); ~bar(); @@ -111,7 +111,7 @@ class bar : public xpp::event::sink m_screen; unique_ptr m_tray; unique_ptr m_renderer; diff --git a/include/components/controller.hpp b/include/components/controller.hpp index 526b39f0..ef2d28e9 100644 --- a/include/components/controller.hpp +++ b/include/components/controller.hpp @@ -40,9 +40,9 @@ class controller : public signal_receiver { public: using make_type = unique_ptr; - static make_type make(bool has_ipc, eventloop::eventloop&); + static make_type make(bool has_ipc, eventloop::loop&); - explicit controller(connection&, signal_emitter&, const logger&, const config&, bool has_ipc, eventloop::eventloop&); + explicit controller(connection&, signal_emitter&, const logger&, const config&, bool has_ipc, eventloop::loop&); ~controller(); bool run(bool writeback, string snapshot_dst, bool confwatch); @@ -98,7 +98,7 @@ class controller : public signal_receiver m_bar; bool m_has_ipc; diff --git a/include/components/eventloop.hpp b/include/components/eventloop.hpp index 1673910b..39848713 100644 --- a/include/components/eventloop.hpp +++ b/include/components/eventloop.hpp @@ -30,7 +30,7 @@ namespace eventloop { using cb_event = std::function; template - class Handle : public non_copyable_mixin { + class Handle : public non_copyable_mixin, public non_movable_mixin { public: Handle(uv_loop_t* l) : uv_loop(l) { get()->data = this; @@ -53,6 +53,11 @@ namespace eventloop { return get(); } + /** + * Close this handle and free associated memory. + * + * After this function returns, any reference to this object should be considered invalid. + */ void close() { if (!is_closing()) { uv_close((uv_handle_t*)get(), [](uv_handle_t* handle) { close_callback(*static_cast(handle->data)); }); @@ -385,10 +390,10 @@ namespace eventloop { cb_connect connect_callback; }; - class eventloop { + class loop : public non_copyable_mixin, public non_movable_mixin { public: - eventloop(); - ~eventloop(); + loop(); + ~loop(); void run(); void stop(); diff --git a/include/events/signal.hpp b/include/events/signal.hpp index 52ba5c75..f992b1e2 100644 --- a/include/events/signal.hpp +++ b/include/events/signal.hpp @@ -2,7 +2,6 @@ #include "common.hpp" #include "components/types.hpp" -#include "utils/functional.hpp" POLYBAR_NS diff --git a/include/ipc/ipc.hpp b/include/ipc/ipc.hpp index cb13e7c4..0ba8af1b 100644 --- a/include/ipc/ipc.hpp +++ b/include/ipc/ipc.hpp @@ -23,12 +23,12 @@ namespace ipc { * running process which will allow messages and * events to be sent to the process externally. */ - class ipc : non_copyable_mixin, non_movable_mixin { + class ipc : public non_copyable_mixin, public non_movable_mixin { public: using make_type = unique_ptr; - static make_type make(eventloop::eventloop& loop); + static make_type make(eventloop::loop& loop); - explicit ipc(signal_emitter& emitter, const logger& logger, eventloop::eventloop& loop); + explicit ipc(signal_emitter& emitter, const logger& logger, eventloop::loop& loop); ~ipc(); static string get_socket_path(int pid); @@ -42,14 +42,15 @@ namespace ipc { private: signal_emitter& m_sig; const logger& m_log; - eventloop::eventloop& m_loop; + eventloop::loop& m_loop; eventloop::PipeHandle& socket; - class connection : public non_movable_mixin { + class connection : public non_copyable_mixin, public non_movable_mixin { public: using cb = std::function&)>; - connection(eventloop::eventloop& loop, cb msg_callback); + connection(eventloop::loop& loop, cb msg_callback); + ~connection(); eventloop::PipeHandle& client_pipe; decoder dec; }; @@ -78,7 +79,7 @@ namespace ipc { // Named pipe properties (deprecated) struct fifo { - fifo(eventloop::eventloop& loop, ipc& ipc, const string& path); + fifo(eventloop::loop& loop, ipc& ipc, const string& path); ~fifo(); eventloop::PipeHandle& pipe_handle; }; diff --git a/include/modules/meta/base.hpp b/include/modules/meta/base.hpp index 60a82e5e..8264a96b 100644 --- a/include/modules/meta/base.hpp +++ b/include/modules/meta/base.hpp @@ -11,7 +11,6 @@ #include "components/types.hpp" #include "errors.hpp" #include "utils/concurrency.hpp" -#include "utils/functional.hpp" #include "utils/inotify.hpp" #include "utils/string.hpp" POLYBAR_NS diff --git a/include/utils/command.hpp b/include/utils/command.hpp index c05b7a63..db9a5564 100644 --- a/include/utils/command.hpp +++ b/include/utils/command.hpp @@ -5,7 +5,6 @@ #include "components/types.hpp" #include "errors.hpp" #include "utils/file.hpp" -#include "utils/functional.hpp" POLYBAR_NS @@ -86,7 +85,7 @@ class command : private command::get_pid; using command::get_exit_status; - void tail(callback cb); + void tail(std::function cb); string readline(); int get_stdout(int c); diff --git a/include/utils/scope.hpp b/include/utils/scope.hpp index f50ee7c3..c246ecd4 100644 --- a/include/utils/scope.hpp +++ b/include/utils/scope.hpp @@ -1,9 +1,6 @@ #pragma once -// TODO: move to functional.hpp - #include "common.hpp" -#include "components/logger.hpp" POLYBAR_NS diff --git a/src/components/bar.cpp b/src/components/bar.cpp index ded48e60..6a8b8f15 100644 --- a/src/components/bar.cpp +++ b/src/components/bar.cpp @@ -32,11 +32,12 @@ POLYBAR_NS using namespace signals::ui; +using namespace eventloop; /** * Create instance */ -bar::make_type bar::make(eventloop::eventloop& loop, bool only_initialize_values) { +bar::make_type bar::make(loop& loop, bool only_initialize_values) { auto action_ctxt = make_unique(); // clang-format off @@ -59,9 +60,9 @@ bar::make_type bar::make(eventloop::eventloop& loop, bool only_initialize_values * * TODO: Break out all tray handling */ -bar::bar(connection& conn, signal_emitter& emitter, const config& config, const logger& logger, - eventloop::eventloop& loop, unique_ptr&& screen, unique_ptr&& tray_manager, - unique_ptr&& dispatch, unique_ptr&& action_ctxt, bool only_initialize_values) +bar::bar(connection& conn, signal_emitter& emitter, const config& config, const logger& logger, loop& loop, + unique_ptr&& screen, unique_ptr&& tray_manager, unique_ptr&& dispatch, + unique_ptr&& action_ctxt, bool only_initialize_values) : m_connection(conn) , m_sig(emitter) , m_conf(config) @@ -737,7 +738,7 @@ void bar::handle(const evt::button_press& evt) { * the configured interval and if in that time another click arrives, we * need to trigger a double click. */ - const auto check_double = [this](eventloop::TimerHandle& handle, mousebtn btn, int pos) { + const auto check_double = [this](TimerHandle& handle, mousebtn btn, int pos) { if (!handle.is_active()) { handle.start(m_opts.double_click_interval, 0, [=]() { trigger_click(btn, pos); }); } else { diff --git a/src/components/controller.cpp b/src/components/controller.cpp index e34fde59..fe536fc2 100644 --- a/src/components/controller.cpp +++ b/src/components/controller.cpp @@ -26,10 +26,12 @@ POLYBAR_NS +using namespace eventloop; + /** * Build controller instance */ -controller::make_type controller::make(bool has_ipc, eventloop::eventloop& loop) { +controller::make_type controller::make(bool has_ipc, loop& loop) { return std::make_unique( connection::make(), signal_emitter::make(), logger::make(), config::make(), has_ipc, loop); } @@ -37,8 +39,8 @@ controller::make_type controller::make(bool has_ipc, eventloop::eventloop& loop) /** * Construct controller */ -controller::controller(connection& conn, signal_emitter& emitter, const logger& logger, const config& config, - bool has_ipc, eventloop::eventloop& loop) +controller::controller( + connection& conn, signal_emitter& emitter, const logger& logger, const config& config, bool has_ipc, loop& loop) : m_connection(conn) , m_sig(emitter) , m_log(logger) @@ -235,30 +237,32 @@ void controller::read_events(bool confwatch) { m_log.info("Entering event loop (thread-id=%lu)", this_thread::get_id()); try { - auto& poll_handle = m_loop.handle(m_connection.get_file_descriptor()); + auto& poll_handle = m_loop.handle(m_connection.get_file_descriptor()); poll_handle.start( UV_READABLE, [this](const auto&) { conn_cb(); }, - [](const auto& e) { - throw runtime_error("libuv error while polling X connection: "s + uv_strerror(e.status)); + [this](const auto& e) { + m_log.err("libuv error while polling X connection: "s + uv_strerror(e.status)); + stop(false); }); for (auto s : {SIGINT, SIGQUIT, SIGTERM, SIGUSR1, SIGALRM}) { - auto& signal_handle = m_loop.handle(); + auto& signal_handle = m_loop.handle(); signal_handle.start(s, [this](const auto& e) { signal_handler(e.signum); }); } if (confwatch) { - auto& fs_event_handle = m_loop.handle(); + auto& fs_event_handle = m_loop.handle(); fs_event_handle.start( m_conf.filepath(), 0, [this](const auto& e) { confwatch_handler(e.path); }, - [this](const auto& e) { + [this, &fs_event_handle](const auto& e) { m_log.err("libuv error while watching config file for changes: %s", uv_strerror(e.status)); + fs_event_handle.close(); }); } if (!m_snapshot_dst.empty()) { // Trigger a single screenshot after 3 seconds - auto& timer_handle = m_loop.handle(); + auto& timer_handle = m_loop.handle(); timer_handle.start(3000, 0, [this]() { screenshot_handler(); }); } diff --git a/src/components/eventloop.cpp b/src/components/eventloop.cpp index 694fac0e..623e3994 100644 --- a/src/components/eventloop.cpp +++ b/src/components/eventloop.cpp @@ -68,7 +68,6 @@ namespace eventloop { void PollHandle::poll_callback(uv_poll_t* handle, int status, int events) { auto& self = cast(handle); if (status < 0) { - self.close(); self.err_cb(ErrorEvent{status}); return; } @@ -91,7 +90,6 @@ namespace eventloop { void FSEventHandle::fs_event_callback(uv_fs_event_t* handle, const char* path, int events, int status) { auto& self = cast(handle); if (status < 0) { - self.close(); self.err_cb(ErrorEvent{status}); return; } @@ -176,13 +174,13 @@ namespace eventloop { UV(uv_run, loop, UV_RUN_DEFAULT); } - eventloop::eventloop() { + loop::loop() { m_loop = std::make_unique(); UV(uv_loop_init, m_loop.get()); m_loop->data = this; } - eventloop::~eventloop() { + loop::~loop() { if (m_loop) { try { close_loop(m_loop.get()); @@ -195,15 +193,15 @@ namespace eventloop { } } - void eventloop::run() { + void loop::run() { UV(uv_run, m_loop.get(), UV_RUN_DEFAULT); } - void eventloop::stop() { + void loop::stop() { uv_stop(m_loop.get()); } - uv_loop_t* eventloop::get() const { + uv_loop_t* loop::get() const { return m_loop.get(); } // }}} diff --git a/src/ipc/ipc.cpp b/src/ipc/ipc.cpp index 06b12168..66506f15 100644 --- a/src/ipc/ipc.cpp +++ b/src/ipc/ipc.cpp @@ -18,6 +18,8 @@ POLYBAR_NS +using namespace eventloop; + namespace ipc { /** @@ -30,15 +32,15 @@ namespace ipc { /** * Create instance */ - ipc::make_type ipc::make(eventloop::eventloop& loop) { + ipc::make_type ipc::make(loop& loop) { return std::make_unique(signal_emitter::make(), logger::make(), loop); } /** * Construct ipc handler */ - ipc::ipc(signal_emitter& emitter, const logger& logger, eventloop::eventloop& loop) - : m_sig(emitter), m_log(logger), m_loop(loop), socket(loop.handle()) { + ipc::ipc(signal_emitter& emitter, const logger& logger, loop& loop) + : m_sig(emitter), m_log(logger), m_loop(loop), socket(loop.handle()) { m_pipe_path = string_util::replace(PATH_MESSAGING_FIFO, "%pid%", to_string(getpid())); if (file_util::exists(m_pipe_path) && unlink(m_pipe_path.c_str()) == -1) { @@ -72,8 +74,6 @@ namespace ipc { if (unlink(m_pipe_path.c_str()) == -1) { m_log.err("Failed to delete ipc named pipe: %s", strerror(errno)); } - - socket.close(); } string ipc::get_socket_path(int pid) { @@ -161,18 +161,20 @@ namespace ipc { } void ipc::remove_client(connection& conn) { - conn.client_pipe.close(); connections.erase(connections.find(conn)); } - ipc::connection::connection(eventloop::eventloop& loop, cb msg_callback) - : client_pipe(loop.handle()) + ipc::connection::connection(loop& loop, cb msg_callback) + : client_pipe(loop.handle()) , dec(logger::make(), [this, msg_callback](uint8_t version, auto type, const auto& msg) { msg_callback(*this, version, type, msg); }) {} - ipc::fifo::fifo(eventloop::eventloop& loop, ipc& ipc, const string& path) - : pipe_handle(loop.handle()) { + ipc::connection::~connection() { + client_pipe.close(); + } + + ipc::fifo::fifo(loop& loop, ipc& ipc, const string& path) : pipe_handle(loop.handle()) { int fd; if ((fd = open(path.c_str(), O_RDONLY | O_NONBLOCK)) == -1) { throw system_error("Failed to open pipe '" + path + "'"); @@ -181,9 +183,9 @@ namespace ipc { pipe_handle.open(fd); pipe_handle.read_start([&ipc](const auto& e) mutable { ipc.receive_data(string(e.data, e.len)); }, [&ipc]() { ipc.receive_eof(); }, - [this, &ipc](const auto& e) mutable { + [&ipc](const auto& e) mutable { ipc.m_log.err("libuv error while listening to IPC channel: %s", uv_strerror(e.status)); - pipe_handle.close(); + ipc.ipc_pipe.reset(); }); } diff --git a/src/main.cpp b/src/main.cpp index 960d3e5d..a5286f0b 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -10,6 +10,7 @@ #include "x11/connection.hpp" using namespace polybar; +using namespace eventloop; int main(int argc, char** argv) { // clang-format off @@ -58,7 +59,7 @@ int main(int argc, char** argv) { return EXIT_SUCCESS; } - eventloop::eventloop loop{}; + loop loop{}; //================================================== // Connect to X server diff --git a/src/polybar-msg.cpp b/src/polybar-msg.cpp index 3e938c7b..0262bc33 100644 --- a/src/polybar-msg.cpp +++ b/src/polybar-msg.cpp @@ -17,8 +17,9 @@ #include "utils/actions.hpp" #include "utils/file.hpp" -using namespace polybar; using namespace std; +using namespace polybar; +using namespace eventloop; static const char* exec = nullptr; static constexpr auto USAGE = " [...]"; @@ -75,7 +76,7 @@ static vector get_sockets() { return sockets; } -static void on_write(eventloop::PipeHandle& conn, ipc::decoder& dec) { +static void on_write(PipeHandle& conn, ipc::decoder& dec) { conn.read_start( [&](const auto& e) { try { @@ -93,8 +94,7 @@ static void on_write(eventloop::PipeHandle& conn, ipc::decoder& dec) { }); } -static void on_connection( - eventloop::PipeHandle& conn, ipc::decoder& dec, const ipc::type_t type, const string& payload) { +static void on_connection(PipeHandle& conn, ipc::decoder& dec, const ipc::type_t type, const string& payload) { const auto data = ipc::encode(type, payload); conn.write( data, [&]() { on_write(conn, dec); }, @@ -220,12 +220,11 @@ int run(int argc, char** argv) { string payload; ipc::type_t type; std::tie(type, payload) = parse_message(args); - - const string type_str = type == to_integral(ipc::v0::ipc_type::ACTION) ? "action" : "command"; + string type_str = type == to_integral(ipc::v0::ipc_type::ACTION) ? "action" : "command"; bool success = true; - eventloop::eventloop loop; + loop loop; logger null_logger{loglevel::NONE}; @@ -235,13 +234,11 @@ int run(int argc, char** argv) { vector decoders; for (auto&& channel : sockets) { - auto& conn = loop.handle(); - int pid = ipc::get_pid_from_socket(channel); assert(pid > 0); decoders.emplace_back( - null_logger, [pid, channel, &type_str, &payload, &success](uint8_t, ipc::type_t type, const auto& response) { + null_logger, [pid, channel, &payload, &type_str, &success](uint8_t, ipc::type_t type, const auto& response) { switch (type) { case ipc::TYPE_OK: printf("Successfully wrote %s '%s' to PID %d\n", type_str.c_str(), payload.c_str(), pid); @@ -265,6 +262,7 @@ int run(int argc, char** argv) { */ int idx = decoders.size() - 1; + auto& conn = loop.handle(); conn.connect( channel, [&conn, &decoders, type, payload, channel, idx]() { on_connection(conn, decoders[idx], type, payload); }, diff --git a/src/utils/command.cpp b/src/utils/command.cpp index f9427f78..34db0ac4 100644 --- a/src/utils/command.cpp +++ b/src/utils/command.cpp @@ -184,7 +184,7 @@ int command::exec(bool wait_for_completion, const vec * \note: This is a blocking call and will not * end until the stream is closed */ -void command::tail(callback cb) { +void command::tail(std::function cb) { io_util::tail(get_stdout(PIPE_READ), cb); }