Avoid event handler data races

src/logid/backend/EventHandlerList.h

@@ -24,21 +24,64 @@
 #include <mutex>
 #include <shared_mutex>
 #include <list>
+#include <atomic>
 
 template <class T>
 class EventHandlerLock;
 
 template <class T>
-struct EventHandlerList {
+class EventHandlerList {
-    typedef std::list<typename T::EventHandler> list_t;
+public:
-    typedef typename list_t::const_iterator iterator_t;
+    typedef std::list<std::pair<typename T::EventHandler, std::atomic_bool>> list_t;
+    typedef typename list_t::iterator iterator_t;
+private:
+    list_t list;
+    std::shared_mutex mutex;
+    std::shared_mutex add_mutex;
 
-    std::list<typename T::EventHandler> list;
+    void cleanup() {
-    mutable std::shared_mutex mutex;
+        std::unique_lock lock(mutex, std::try_to_lock);
+        if (lock.owns_lock()) {
+            std::list<iterator_t> to_remove;
+            for (auto it = list.begin(); it != list.end(); ++it) {
+                if (!it->second)
+                    to_remove.push_back(it);
+            }
+
+            for(auto& it : to_remove)
+                list.erase(it);
+        }
+    }
+public:
+    iterator_t add(typename T::EventHandler handler) {
+        std::unique_lock add_lock(add_mutex);
+        list.emplace_front(std::move(handler), true);
+        return list.begin();
+    }
 
     void remove(iterator_t iterator) {
-        std::unique_lock lock(mutex);
+        std::unique_lock lock(mutex, std::try_to_lock);
-        list.erase(iterator);
+        if (lock.owns_lock()) {
+            std::unique_lock add_lock(add_mutex);
+            list.erase(iterator);
+        } else {
+            iterator->second = false;
+        }
+    }
+
+    template <typename Arg>
+    void run_all(Arg arg) {
+        cleanup();
+        std::shared_lock lock(mutex);
+        std::shared_lock add_lock(add_mutex);
+        for (auto& handler : list) {
+            add_lock.unlock();
+            if (handler.second) {
+                if (handler.first.condition(arg))
+                    handler.first.callback(arg);
+            }
+            add_lock.lock();
+        }
     }
 };
 

src/logid/backend/hidpp/Device.cpp

@@ -214,19 +214,14 @@ void Device::_init() {
 }
 
 EventHandlerLock<Device> Device::addEventHandler(EventHandler handler) {
-    std::unique_lock lock(_event_handlers->mutex);
+    return {_event_handlers, _event_handlers->add(std::move(handler))};
-    _event_handlers->list.emplace_front(std::move(handler));
-    return {_event_handlers, _event_handlers->list.cbegin()};
 }
 
 void Device::handleEvent(Report& report) {
     if (responseReport(report))
         return;
 
-    std::shared_lock lock(_event_handlers->mutex);
+    _event_handlers->run_all(report);
-    for (auto& handler: _event_handlers->list)
-        if (handler.condition(report))
-            handler.callback(report);
 }
 
 Report Device::sendReport(const Report& report) {

src/logid/backend/raw/RawDevice.cpp

@@ -156,9 +156,7 @@ void RawDevice::sendReport(const std::vector<uint8_t>& report) {
 }
 
 EventHandlerLock<RawDevice> RawDevice::addEventHandler(RawEventHandler handler) {
-    std::unique_lock<std::shared_mutex> lock(_event_handlers->mutex);
+    return {_event_handlers, _event_handlers->add(std::forward<RawEventHandler>(handler))};
-    _event_handlers->list.emplace_front(std::move(handler));
-    return {_event_handlers, _event_handlers->list.cbegin()};
 }
 
 void RawDevice::_readReports() {
@@ -181,8 +179,5 @@ void RawDevice::_readReports() {
 }
 
 void RawDevice::_handleEvent(const std::vector<uint8_t>& report) {
-    std::shared_lock<std::shared_mutex> lock(_event_handlers->mutex);
+    _event_handlers->run_all(report);
-    for (auto& handler : _event_handlers->list)
-        if (handler.condition(report))
-            handler.callback(report);
 }