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

@@ -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.<PID>` 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.

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 pixelsize = fc_pixelsize == 0 ? 10 : fc_pixelsize;
-      double size = fc_size == 0? 10 : fc_size;
+      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<font_fc>(cairo, match, offset, dpi_x, dpi_y);
   }
-}
+}  // namespace cairo
 
 POLYBAR_NS_END

include/components/bar.hpp

@@ -63,9 +63,9 @@ class bar : public xpp::event::sink<evt::button_press, evt::expose, evt::propert
                 > {
  public:
   using make_type = unique_ptr<bar>;
-  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<screen>&&,
+  explicit bar(connection&, signal_emitter&, const config&, const logger&, eventloop::loop&, unique_ptr<screen>&&,
       unique_ptr<tray_manager>&&, unique_ptr<tags::dispatch>&&, unique_ptr<tags::action_context>&&,
       bool only_initialize_values);
   ~bar();
@@ -111,7 +111,7 @@ class bar : public xpp::event::sink<evt::button_press, evt::expose, evt::propert
   signal_emitter& m_sig;
   const config& m_conf;
   const logger& m_log;
-  eventloop::eventloop& m_loop;
+  eventloop::loop& m_loop;
   unique_ptr<screen> m_screen;
   unique_ptr<tray_manager> m_tray;
   unique_ptr<renderer> m_renderer;

include/components/controller.hpp

@@ -40,9 +40,9 @@ class controller : public signal_receiver<SIGN_PRIORITY_CONTROLLER, signals::eve
                        signals::ipc::hook, signals::ui::button_press, signals::ui::update_background> {
  public:
   using make_type = unique_ptr<controller>;
-  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<SIGN_PRIORITY_CONTROLLER, signals::eve
   signal_emitter& m_sig;
   const logger& m_log;
   const config& m_conf;
-  eventloop::eventloop& m_loop;
+  eventloop::loop& m_loop;
   unique_ptr<bar> m_bar;
   bool m_has_ipc;
 

include/components/eventloop.hpp

@@ -30,7 +30,7 @@ namespace eventloop {
   using cb_event = std::function<void(const Event&)>;
 
   template <typename Self, typename H>
-  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<Self*>(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();
+    loop();
-    ~eventloop();
+    ~loop();
     void run();
     void stop();
 

include/events/signal.hpp

@@ -2,7 +2,6 @@
 
 #include "common.hpp"
 #include "components/types.hpp"
-#include "utils/functional.hpp"
 
 POLYBAR_NS
 

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<ipc>;
-    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<void(connection&, uint8_t, type_t, const std::vector<uint8_t>&)>;
-      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;
     };

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

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<output_policy::REDIRECTED> : private command<output_policy::IGNORE
   using command<output_policy::IGNORED>::get_pid;
   using command<output_policy::IGNORED>::get_exit_status;
 
-  void tail(callback<string> cb);
+  void tail(std::function<void(string)> cb);
   string readline();
 
   int get_stdout(int c);

include/utils/scope.hpp

@@ -1,9 +1,6 @@
 #pragma once
 
-// TODO: move to functional.hpp
-
 #include "common.hpp"
-#include "components/logger.hpp"
 
 POLYBAR_NS
 

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<tags::action_context>();
 
   // 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,
+bar::bar(connection& conn, signal_emitter& emitter, const config& config, const logger& logger, loop& loop,
-    eventloop::eventloop& loop, unique_ptr<screen>&& screen, unique_ptr<tray_manager>&& tray_manager,
+    unique_ptr<screen>&& screen, unique_ptr<tray_manager>&& tray_manager, unique_ptr<tags::dispatch>&& dispatch,
-    unique_ptr<tags::dispatch>&& dispatch, unique_ptr<tags::action_context>&& action_ctxt, bool only_initialize_values)
+    unique_ptr<tags::action_context>&& 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 {

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<controller>(
       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,
+controller::controller(
-    bool has_ipc, eventloop::eventloop& loop)
+    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<eventloop::PollHandle>(m_connection.get_file_descriptor());
+    auto& poll_handle = m_loop.handle<PollHandle>(m_connection.get_file_descriptor());
     poll_handle.start(
         UV_READABLE, [this](const auto&) { conn_cb(); },
-        [](const auto& e) {
+        [this](const auto& e) {
-          throw runtime_error("libuv error while polling X connection: "s + uv_strerror(e.status));
+          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<eventloop::SignalHandle>();
+      auto& signal_handle = m_loop.handle<SignalHandle>();
       signal_handle.start(s, [this](const auto& e) { signal_handler(e.signum); });
     }
 
     if (confwatch) {
-      auto& fs_event_handle = m_loop.handle<eventloop::FSEventHandle>();
+      auto& fs_event_handle = m_loop.handle<FSEventHandle>();
       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<eventloop::TimerHandle>();
+      auto& timer_handle = m_loop.handle<TimerHandle>();
       timer_handle.start(3000, 0, [this]() { screenshot_handler(); });
     }
 

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_loop_t>();
     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();
   }
   // }}}

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<ipc>(signal_emitter::make(), logger::make(), loop);
   }
 
   /**
    * Construct ipc handler
    */
-  ipc::ipc(signal_emitter& emitter, const logger& logger, eventloop::eventloop& loop)
+  ipc::ipc(signal_emitter& emitter, const logger& logger, loop& loop)
-      : m_sig(emitter), m_log(logger), m_loop(loop), socket(loop.handle<eventloop::PipeHandle>()) {
+      : m_sig(emitter), m_log(logger), m_loop(loop), socket(loop.handle<PipeHandle>()) {
     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)
+  ipc::connection::connection(loop& loop, cb msg_callback)
-      : client_pipe(loop.handle<eventloop::PipeHandle>())
+      : client_pipe(loop.handle<PipeHandle>())
       , 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)
+  ipc::connection::~connection() {
-      : pipe_handle(loop.handle<eventloop::PipeHandle>()) {
+    client_pipe.close();
+  }
+
+  ipc::fifo::fifo(loop& loop, ipc& ipc, const string& path) : pipe_handle(loop.handle<PipeHandle>()) {
     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();
         });
   }
 

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

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 = "<command=(action|cmd)> <payload> [...]";
@@ -75,7 +76,7 @@ static vector<string> 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(
+static void on_connection(PipeHandle& conn, ipc::decoder& dec, const ipc::type_t type, const string& payload) {
-    eventloop::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);
-
+  string type_str = type == to_integral(ipc::v0::ipc_type::ACTION) ? "action" : "command";
-  const 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<ipc::decoder> decoders;
 
   for (auto&& channel : sockets) {
-    auto& conn = loop.handle<eventloop::PipeHandle>();
-
     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<PipeHandle>();
     conn.connect(
         channel,
         [&conn, &decoders, type, payload, channel, idx]() { on_connection(conn, decoders[idx], type, payload); },

src/utils/command.cpp

@@ -184,7 +184,7 @@ int command<output_policy::REDIRECTED>::exec(bool wait_for_completion, const vec
  * \note: This is a blocking call and will not
  * end until the stream is closed
  */
-void command<output_policy::REDIRECTED>::tail(callback<string> cb) {
+void command<output_policy::REDIRECTED>::tail(std::function<void(string)> cb) {
   io_util::tail(get_stdout(PIPE_READ), cb);
 }