From cb7a2dad7c979f13491ef4e9e6b76c028fc1bcb2 Mon Sep 17 00:00:00 2001 From: pixl Date: Fri, 19 May 2023 22:43:06 -0400 Subject: [PATCH] Resolve deadlocking when adding to IOMonitor Do not lock run_mutex while running an I/O handler. --- src/logid/DeviceManager.cpp | 6 +- src/logid/backend/hidpp/Device.cpp | 2 +- src/logid/backend/raw/DeviceMonitor.cpp | 36 +++--- src/logid/backend/raw/IOMonitor.cpp | 150 ++++++++++-------------- src/logid/backend/raw/IOMonitor.h | 16 ++- src/logid/backend/raw/RawDevice.cpp | 17 ++- src/logid/backend/raw/RawDevice.h | 24 +++- 7 files changed, 130 insertions(+), 121 deletions(-) diff --git a/src/logid/DeviceManager.cpp b/src/logid/DeviceManager.cpp index 71cb9241..e41ac306 100644 --- a/src/logid/DeviceManager.cpp +++ b/src/logid/DeviceManager.cpp @@ -66,11 +66,11 @@ void DeviceManager::addDevice(std::string path) { // Check if device is ignored before continuing { - raw::RawDevice raw_dev(path, self().lock()); + auto raw_dev = raw::RawDevice::make(path, self().lock()); if (config()->ignore.has_value() && - config()->ignore.value().contains(raw_dev.productId())) { + config()->ignore.value().contains(raw_dev->productId())) { logPrintf(DEBUG, "%s: Device 0x%04x ignored.", - path.c_str(), raw_dev.productId()); + path.c_str(), raw_dev->productId()); return; } } diff --git a/src/logid/backend/hidpp/Device.cpp b/src/logid/backend/hidpp/Device.cpp index 1faee337..f3bf362e 100644 --- a/src/logid/backend/hidpp/Device.cpp +++ b/src/logid/backend/hidpp/Device.cpp @@ -53,7 +53,7 @@ Device::Device(const std::string& path, DeviceIndex index, const std::shared_ptr& monitor, double timeout) : io_timeout(duration_cast( duration(timeout))), - _raw_device(std::make_shared(path, monitor)), + _raw_device(raw::RawDevice::make(path, monitor)), _receiver(nullptr), _path(path), _index(index) { } diff --git a/src/logid/backend/raw/DeviceMonitor.cpp b/src/logid/backend/raw/DeviceMonitor.cpp index 0e5cb996..7b8ae01c 100644 --- a/src/logid/backend/raw/DeviceMonitor.cpp +++ b/src/logid/backend/raw/DeviceMonitor.cpp @@ -89,23 +89,25 @@ void DeviceMonitor::ready() { _ready = true; _io_monitor->add(_fd, { - [this]() { - struct udev_device* device = udev_monitor_receive_device(_udev_monitor); - std::string action = udev_device_get_action(device); - std::string dev_node = udev_device_get_devnode(device); - - if (action == "add") - run_task([self_weak = _self, dev_node]() { - if (auto self = self_weak.lock()) - self->_addHandler(dev_node); - }); - else if (action == "remove") - run_task([self_weak = _self, dev_node]() { - if (auto self = self_weak.lock()) - self->_removeHandler(dev_node); - }); - - udev_device_unref(device); + [self_weak = _self]() { + if (auto self = self_weak.lock()) { + struct udev_device* device = udev_monitor_receive_device(self->_udev_monitor); + std::string action = udev_device_get_action(device); + std::string dev_node = udev_device_get_devnode(device); + + if (action == "add") + run_task([self_weak, dev_node]() { + if (auto self = self_weak.lock()) + self->_addHandler(dev_node); + }); + else if (action == "remove") + run_task([self_weak, dev_node]() { + if (auto self = self_weak.lock()) + self->_removeHandler(dev_node); + }); + + udev_device_unref(device); + } }, []() { throw std::runtime_error("udev hangup"); diff --git a/src/logid/backend/raw/IOMonitor.cpp b/src/logid/backend/raw/IOMonitor.cpp index 39907afe..7a7a3236 100644 --- a/src/logid/backend/raw/IOMonitor.cpp +++ b/src/logid/backend/raw/IOMonitor.cpp @@ -16,7 +16,7 @@ * */ #include -#include +#include #include extern "C" @@ -36,55 +36,6 @@ IOHandler::IOHandler(std::function r, error(std::move(err)) { } -class IOMonitor::io_lock { - std::optional> _lock; - IOMonitor* _io_monitor; - const uint64_t counter = 1; - -public: - explicit io_lock(IOMonitor* io_monitor) : _io_monitor(io_monitor) { - _io_monitor->_interrupting = true; - [[maybe_unused]] ssize_t ret = ::write(_io_monitor->_event_fd, &counter, sizeof(counter)); - assert(ret == sizeof(counter)); - _lock.emplace(_io_monitor->_run_mutex); - } - - io_lock(const io_lock&) = delete; - - io_lock& operator=(const io_lock&) = delete; - - io_lock(io_lock&& o) noexcept: _lock(std::move(o._lock)), _io_monitor(o._io_monitor) { - o._lock.reset(); - o._io_monitor = nullptr; - } - - io_lock& operator=(io_lock&& o) noexcept { - if (this != &o) { - _lock = std::move(o._lock); - _io_monitor = o._io_monitor; - o._lock.reset(); - o._io_monitor = nullptr; - } - - return *this; - } - - ~io_lock() noexcept { - if (_lock && _io_monitor) { - uint64_t buf{}; - [[maybe_unused]] const ssize_t ret = ::read( - _io_monitor->_event_fd, &buf, sizeof(counter)); - - assert(ret != -1); - - if (buf == counter) { - _io_monitor->_interrupting = false; - _io_monitor->_interrupt_cv.notify_one(); - } - } - } -}; - IOMonitor::IOMonitor() : _epoll_fd(epoll_create1(0)), _event_fd(eventfd(0, EFD_NONBLOCK)) { if (_epoll_fd < 0) { @@ -106,12 +57,7 @@ IOMonitor::IOMonitor() : _epoll_fd(epoll_create1(0)), throw std::system_error(errno, std::generic_category()); } - _fds.emplace(std::piecewise_construct, std::forward_as_tuple(_event_fd), - std::forward_as_tuple([]() {}, []() { - throw std::runtime_error("event_fd hangup"); - }, []() { - throw std::runtime_error("event_fd error"); - })); + _fds.emplace(_event_fd, nullptr); _io_thread = std::make_unique([this]() { _listen(); @@ -122,70 +68,100 @@ IOMonitor::~IOMonitor() noexcept { _stop(); if (_event_fd >= 0) - close(_event_fd); + ::close(_event_fd); if (_epoll_fd >= 0) - close(_epoll_fd); + ::close(_epoll_fd); } void IOMonitor::_listen() { std::unique_lock lock(_run_mutex); std::vector events; + if (_is_running) + throw std::runtime_error("IOMonitor double run"); + _is_running = true; while (_is_running) { - if (_interrupting) { - _interrupt_cv.wait(lock, [this]() { - return !_interrupting; - }); - - if (!_is_running) - break; - } - if (events.size() != _fds.size()) events.resize(_fds.size()); + int ev_count = ::epoll_wait(_epoll_fd, events.data(), (int) events.size(), -1); for (int i = 0; i < ev_count; ++i) { - const auto& handler = _fds.at(events[i].data.fd); - if (events[i].events & EPOLLIN) - handler.read(); - if (events[i].events & EPOLLHUP) - handler.hangup(); - if (events[i].events & EPOLLERR) - handler.error(); + std::shared_ptr handler; + + if (events[i].data.fd == _event_fd) { + if (events[i].events & EPOLLIN) { + lock.unlock(); + /* Wait until done yielding */ + const std::lock_guard yield_lock(_yield_mutex); + uint64_t event; + while (-1 != ::eventfd_read(_event_fd, &event)) { } + lock.lock(); + } + } else { + try { + handler = _fds.at(events[i].data.fd); + } catch (std::out_of_range& e) { + continue; + } + + lock.unlock(); + try { + if (events[i].events & EPOLLIN) + handler->read(); + if (events[i].events & EPOLLHUP) + handler->hangup(); + if (events[i].events & EPOLLERR) + handler->error(); + } catch (std::exception& e) { + logPrintf(ERROR, "Unhandled I/O handler error: %s", e.what()); + } + lock.lock(); + + } } } } void IOMonitor::_stop() noexcept { - { - [[maybe_unused]] const io_lock lock(this); - _is_running = false; - } + _is_running = false; + _yield(); _io_thread->join(); } +std::unique_lock IOMonitor::_yield() noexcept { + /* Prevent listener thread from grabbing lock during yielding */ + std::unique_lock yield_lock(_yield_mutex); + + std::unique_lock run_lock(_run_mutex, std::try_to_lock); + if (!run_lock.owns_lock()) { + ::eventfd_write(_event_fd, 1); + run_lock = std::unique_lock(_run_mutex); + } + + return run_lock; +} + void IOMonitor::add(int fd, IOHandler handler) { - [[maybe_unused]] const io_lock lock(this); + const auto lock = _yield(); struct epoll_event event{}; event.events = EPOLLIN | EPOLLHUP | EPOLLERR; event.data.fd = fd; - // TODO: EPOLL_CTL_MOD - if (_fds.contains(fd)) + if (!_fds.contains(fd)) { + if (::epoll_ctl(_epoll_fd, EPOLL_CTL_ADD, fd, &event)) + throw std::system_error(errno, std::generic_category()); + _fds.emplace(fd, std::make_shared(std::move(handler))); + } else { throw std::runtime_error("duplicate io fd"); - - if (::epoll_ctl(_epoll_fd, EPOLL_CTL_ADD, fd, &event)) - throw std::system_error(errno, std::generic_category()); - _fds.emplace(fd, std::move(handler)); + } } void IOMonitor::remove(int fd) noexcept { - [[maybe_unused]] const io_lock lock(this); - + const auto lock = _yield(); ::epoll_ctl(_epoll_fd, EPOLL_CTL_DEL, fd, nullptr); _fds.erase(fd); } \ No newline at end of file diff --git a/src/logid/backend/raw/IOMonitor.h b/src/logid/backend/raw/IOMonitor.h index 2ca44ed5..83b4ff16 100644 --- a/src/logid/backend/raw/IOMonitor.h +++ b/src/logid/backend/raw/IOMonitor.h @@ -21,9 +21,10 @@ #include #include #include +#include #include -#include #include +#include namespace logid::backend::raw { struct IOHandler { @@ -53,24 +54,21 @@ namespace logid::backend::raw { void add(int fd, IOHandler handler); void remove(int fd) noexcept; - private: void _listen(); // This is a blocking call void _stop() noexcept; + std::unique_lock _yield() noexcept; std::unique_ptr _io_thread; - std::map _fds; - mutable std::mutex _run_mutex; - std::atomic_bool _is_running; + std::mutex _run_mutex; + std::mutex _yield_mutex; - std::atomic_bool _interrupting; - std::condition_variable _interrupt_cv; + std::map> _fds; + std::atomic_bool _is_running; const int _epoll_fd; const int _event_fd; - - class io_lock; }; } diff --git a/src/logid/backend/raw/RawDevice.cpp b/src/logid/backend/raw/RawDevice.cpp index 54e4c9af..86478998 100644 --- a/src/logid/backend/raw/RawDevice.cpp +++ b/src/logid/backend/raw/RawDevice.cpp @@ -117,11 +117,22 @@ RawDevice::RawDevice(std::string path, const std::shared_ptr& mon auto phys = get_phys(_fd); _sub_device = std::regex_match(phys, virtual_path_regex); } +} +void RawDevice::_ready() { _io_monitor->add(_fd, { - [this]() { _readReports(); }, - [this]() { _valid = false; }, - [this]() { _valid = false; } + [self_weak = _self]() { + if (auto self = self_weak.lock()) + self->_readReports(); + }, + [self_weak = _self]() { + if (auto self = self_weak.lock()) + self->_valid = false; + }, + [self_weak = _self]() { + if (auto self = self_weak.lock()) + self->_valid = false; + } }); } diff --git a/src/logid/backend/raw/RawDevice.h b/src/logid/backend/raw/RawDevice.h index f1669fbe..7e93a30f 100644 --- a/src/logid/backend/raw/RawDevice.h +++ b/src/logid/backend/raw/RawDevice.h @@ -34,7 +34,16 @@ namespace logid::backend::raw { class IOMonitor; + template + class RawDeviceWrapper : public T { + public: + template + RawDeviceWrapper(Args... args) : T(std::forward(args)...) { } + }; + class RawDevice { + template + friend class RawDeviceWrapper; public: static constexpr int max_data_length = 32; typedef RawEventHandler EventHandler; @@ -51,7 +60,14 @@ namespace logid::backend::raw { BusType bus_type; }; - RawDevice(std::string path, const std::shared_ptr& monitor); + template + static std::shared_ptr make(Args... args) { + auto raw_dev = std::make_shared>( + std::forward(args)...); + raw_dev->_self = raw_dev; + raw_dev->_ready(); + return raw_dev; + } ~RawDevice() noexcept; @@ -79,6 +95,10 @@ namespace logid::backend::raw { [[nodiscard]] EventHandlerLock addEventHandler(RawEventHandler handler); private: + RawDevice(std::string path, const std::shared_ptr& monitor); + + void _ready(); + void _readReports(); std::atomic_bool _valid; @@ -91,6 +111,8 @@ namespace logid::backend::raw { std::shared_ptr _io_monitor; + std::weak_ptr _self; + bool _sub_device = false; std::shared_ptr> _event_handlers;