signals: store listeners in a linked list and avoid emit allocs

include/hyprutils/signal/Listener.hpp

@@ -8,7 +8,38 @@ namespace Hyprutils {
     namespace Signal {
         class CUntypedSignal;
 
-        class CSignalListener {
+        class CSignalListener;
+        class CSignalListenerListHead;
+
+        class CSignalListenerListEntry {
+          public:
+            ~CSignalListenerListEntry();
+
+          protected:
+            CSignalListenerListEntry* m_previous = nullptr;
+            CSignalListener*          m_next     = nullptr;
+
+            friend class CSignalListenerListHead; // ?
+        };
+
+        class CSignalListenerListHead : public CSignalListenerListEntry {
+          public:
+            CSignalListenerListHead(bool ownsListeners);
+            ~CSignalListenerListHead(); // intentionally non-virtual
+            void append(CSignalListener* listener);
+            void removeLast();
+            // returns true if the list head is still alive
+            bool             emitAll(void* args);
+
+            CSignalListener* m_last            = nullptr;
+            CSignalListener* m_lastEmit        = nullptr;
+            CSignalListener* m_currentListener = nullptr;
+
+          private:
+            bool m_ownsListeners;
+        };
+
+        class CSignalListener : public CSignalListenerListEntry {
           public:
             CSignalListener(CSignalListener&&)       = delete;
             CSignalListener(CSignalListener&)        = delete;
@@ -20,10 +51,13 @@ namespace Hyprutils {
           private:
             CSignalListener(std::function<void(void*)> handler);
 
+            // returns true if the list head is still alive
+            bool                       emitUntil(void* args, CSignalListenerListHead* head, bool headOwnsListeners);
             void                       emitInternal(void* args);
 
             std::function<void(void*)> m_fHandler;
 
+            friend class CSignalListenerListHead;
             friend class CUntypedSignal;
         };
 

include/hyprutils/signal/Signal.hpp

@@ -12,14 +12,15 @@ namespace Hyprutils {
     namespace Signal {
         class CUntypedSignal {
           protected:
-            CHyprSignalListener                                           registerListenerInternal(std::function<void(void*)> handler);
-            void                                                          registerStaticListenerInternal(std::function<void(void*)> handler);
-            void                                                          emitInternal(void* args);
+            CHyprSignalListener     registerListenerInternal(std::function<void(void*)> handler);
+            void                    registerStaticListenerInternal(std::function<void(void*)> handler);
+            void                    emitInternal(void* args);
 
-            std::vector<Hyprutils::Memory::CWeakPointer<CSignalListener>> m_vListeners;
-            std::vector<std::unique_ptr<CSignalListener>>                 m_vStaticListeners;
+            CSignalListenerListHead m_listeners{false};
+            CSignalListenerListHead m_staticListeners{true};
         };
 
+        // Note: Signals are NOT re-entrant.
         template <typename... Args>
         class CSignalT : public CUntypedSignal {
           public:

src/signal/Listener.cpp

@@ -1,4 +1,5 @@
 #include <hyprutils/signal/Listener.hpp>
+#include <stdexcept>
 #include <tuple>
 
 using namespace Hyprutils::Signal;
@@ -7,6 +8,142 @@ Hyprutils::Signal::CSignalListener::CSignalListener(std::function<void(void*)> h
     ;
 }
 
+Hyprutils::Signal::CSignalListenerListEntry::~CSignalListenerListEntry() {
+    if (m_previous) {
+        m_previous->m_next = m_next;
+
+        if (m_next) {
+            m_next->m_previous = m_previous;
+        } else {
+            auto* head = m_previous;
+            while (head->m_previous)
+                head = head->m_previous;
+
+            // The head of the list will always be a CSignalListenerListHead. On destruction, the head
+            // unlinks m_previous of all elements ensuring this is the case.
+            static_cast<CSignalListenerListHead*>(head)->removeLast(); // NOLINT: always a ListHead
+        }
+    } else {
+        // If m_previous is null, the list head has been destroyed, and any m_next pointers are unreliable.
+        // As the head as been destroyed, they will never be used again and do not need to be unlinked.
+    }
+}
+
+Hyprutils::Signal::CSignalListenerListHead::CSignalListenerListHead(bool ownsListeners) : m_ownsListeners(ownsListeners) {}
+
+Hyprutils::Signal::CSignalListenerListHead::~CSignalListenerListHead() {
+    auto  seenCurrent  = false;
+    auto  seenLastEmit = false;
+
+    auto* list = m_next;
+    while (list) {
+        auto entry = list;
+        list       = list->m_next;
+
+        // If the head owns its listeners, destroy all listeners that arent currently in the exec chain.
+        if (m_ownsListeners && !seenCurrent) {
+            if (entry != m_currentListener) {
+                // delete any already-called emitters (or all of them if not currently emitting)
+                delete entry;
+                continue;
+            }
+
+            seenCurrent = true;
+        }
+
+        // Unlink m_previous for all nodes. This ensures removeLast() will never be called on a non head node.
+        // and acts as a flag to avoid accessing the head.
+        entry->m_previous = nullptr;
+
+        // If the listener is past the last emit marker, it won't execute or destroy itself.
+        if (m_ownsListeners && seenLastEmit)
+            delete entry;
+
+        // Nulling m_next prevents listeners added in the same emit as signal destruction
+        // after m_last has been changed from being fired.
+        if (m_lastEmit && entry == m_lastEmit) {
+            entry->m_next = nullptr;
+            seenLastEmit  = true;
+        }
+    }
+}
+
+void Hyprutils::Signal::CSignalListenerListHead::append(CSignalListener* listener) {
+    auto tail            = m_last ? static_cast<CSignalListenerListEntry*>(m_last) : this;
+    tail->m_next         = listener;
+    listener->m_previous = tail;
+    listener->m_next     = nullptr;
+    m_last               = listener;
+}
+
+void Hyprutils::Signal::CSignalListenerListHead::removeLast() {
+    auto prev = m_last->m_previous;
+    // NOLINTNEXTLINE: new tail cannot be the head meaning it must be a listener
+    auto newTail = prev == this ? nullptr : static_cast<CSignalListener*>(prev);
+
+    // If we're removing a handler that was scheduled to be run as part of the
+    // current emit, we move the pointer back to exclude it.
+    if (m_lastEmit == m_last)
+        m_lastEmit = newTail;
+
+    m_last = newTail;
+}
+
+bool Hyprutils::Signal::CSignalListenerListHead::emitAll(void* args) {
+    if (!m_next)
+        return true;
+
+    if (m_lastEmit)
+        throw std::logic_error("CSignal is not reentrant.");
+
+    // Stopping at m_last prevents any handlers added as part of an emit callback from being triggered.
+    m_lastEmit = m_last;
+    auto live  = m_next->emitUntil(args, this, m_ownsListeners);
+
+    // The signal and its list head can be destroyed during emit. emitUntil returns true
+    // if this has not happened.
+    if (live) {
+        m_lastEmit        = nullptr;
+        m_currentListener = nullptr;
+    }
+
+    return live;
+}
+
+bool Hyprutils::Signal::CSignalListener::emitUntil(void* data, CSignalListenerListHead* head, bool headOwnsListeners) {
+    // If the head is owning, it needs to know which listener is currently running.
+    // It uses this to destroy all prior listeners if the signal itself is destroyed.
+    // Current and future listeners are expected to destroy themselves as seen below.
+    if (headOwnsListeners && m_previous)
+        head->m_currentListener = this;
+
+    emitInternal(data);
+
+    auto next     = m_next;
+    auto previous = m_previous;
+
+    // As a result of emitInternal, the signal might be destroyed. m_previous being null
+    // marks this case. m_previous will be the head or another node in all other cases.
+    // If the head owns listeners and is destroyed during emit, listeners should be destroyed
+    // after execution.
+    if (!previous && headOwnsListeners)
+        delete this;
+
+    // following the above delete, no fields of this can be accessed.
+
+    if (next) {
+        // The lastEmit pointer marks the last listener that should be executed.
+        // If the signal is destroted, m_next will be unlinked at the last node already,
+        // rendering the lastEmit pointer redundant.
+        if (!previous || this != head->m_lastEmit) [[gnu::musttail]]
+            return next->emitUntil(data, head, headOwnsListeners);
+    }
+
+    // If we're at the end of the list, return true if the signal has not been destroyed.
+    // See above comment for more details.
+    return previous;
+}
+
 void Hyprutils::Signal::CSignalListener::emitInternal(void* data) {
     if (!m_fHandler)
         return;

src/signal/Signal.cpp

@@ -9,52 +9,20 @@ using namespace Hyprutils::Memory;
 #define WP CWeakPointer
 
 void Hyprutils::Signal::CUntypedSignal::emitInternal(void* args) {
-    std::vector<SP<CSignalListener>> listeners;
-    for (auto& l : m_vListeners) {
-        if (l.expired())
-            continue;
+    auto live = m_listeners.emitAll(args);
 
-        listeners.emplace_back(l.lock());
-    }
-
-    std::vector<CSignalListener*> statics;
-    statics.reserve(m_vStaticListeners.size());
-    for (auto& l : m_vStaticListeners) {
-        statics.emplace_back(l.get());
-    }
-
-    for (auto& l : listeners) {
-        // if there is only one lock, it means the event is only held by the listeners
-        // vector and was removed during our iteration
-        if (l.strongRef() == 1)
-            continue;
-
-        l->emitInternal(args);
-    }
-
-    for (auto& l : statics) {
-        l->emitInternal(args);
-    }
-
-    // release SPs
-    listeners.clear();
-
-    // we cannot release any expired refs here as one of the listeners could've removed this object and
-    // as such we'd be doing a UAF
+    if (live)
+        m_staticListeners.emitAll(args);
 }
 
 CHyprSignalListener Hyprutils::Signal::CUntypedSignal::registerListenerInternal(std::function<void(void*)> handler) {
     CHyprSignalListener listener = SP<CSignalListener>(new CSignalListener(handler));
-    m_vListeners.emplace_back(listener);
-
-    // housekeeping: remove any stale listeners
-    std::erase_if(m_vListeners, [](const auto& other) { return other.expired(); });
-
+    m_listeners.append(listener.get());
     return listener;
 }
 
 void Hyprutils::Signal::CUntypedSignal::registerStaticListenerInternal(std::function<void(void*)> handler) {
-    m_vStaticListeners.emplace_back(std::unique_ptr<CSignalListener>(new CSignalListener(handler)));
+    m_staticListeners.append(new CSignalListener(handler));
 }
 
 void Hyprutils::Signal::CSignal::emit(std::any data) {

tests/signal.cpp

@@ -1,6 +1,10 @@
 #include <any>
 #include <hyprutils/signal/Signal.hpp>
 #include <hyprutils/memory/WeakPtr.hpp>
+#include <memory>
+#include <stdexcept>
+#include "hyprutils/memory/SharedPtr.hpp"
+#include "hyprutils/signal/Listener.hpp"
 #include "shared.hpp"
 
 using namespace Hyprutils::Signal;
@@ -76,6 +80,101 @@ void typedMany(int& ret) {
     EXPECT(data3, 3);
 }
 
+void listenerAdded(int& ret) {
+    int                 count = 0;
+
+    CSignalT<>          signal;
+    CHyprSignalListener secondListener;
+
+    auto                listener = signal.registerListener([&] {
+        count += 1;
+
+        if (!secondListener)
+            secondListener = signal.registerListener([&] { count += 1; });
+    });
+
+    signal.emit();
+    EXPECT(count, 1); // second should NOT be invoked as it was registed during emit
+
+    signal.emit();
+    EXPECT(count, 3); // second should be invoked
+}
+
+void lastListenerSwapped(int& ret) {
+    int                 count = 0;
+
+    CSignalT<>          signal;
+    CHyprSignalListener removedListener;
+    CHyprSignalListener addedListener;
+
+    auto                firstListener = signal.registerListener([&] {
+        removedListener.reset(); // dropped and should NOT be invoked
+
+        if (!addedListener)
+            addedListener = signal.registerListener([&] { count += 2; });
+    });
+
+    removedListener = signal.registerListener([&] { count += 1; });
+
+    signal.emit();
+    EXPECT(count, 0); // neither the removed nor added listeners should fire
+
+    signal.emit();
+    EXPECT(count, 2); // only the new listener should fire
+}
+
+void signalDestroyed(int& ret) {
+    int  count = 0;
+
+    auto signal = std::make_unique<CSignalT<>>();
+
+    // This ensures a destructor of a listener called before signal reset is safe.
+    auto preListener = signal->registerListener([&] { count += 1; });
+
+    auto listener = signal->registerListener([&] { signal.reset(); });
+
+    // This ensures a destructor of a listener called after signal reset is safe
+    // and gets called.
+    auto postListener = signal->registerListener([&] { count += 1; });
+
+    signal->emit();
+    EXPECT(count, 2); // all listeners should fire regardless of signal deletion
+}
+
+void signalDestroyedWithAddedListener(int& ret) {
+    int                 count = 0;
+
+    auto                signal = std::make_unique<CSignalT<>>();
+    CHyprSignalListener shouldNotRun;
+
+    auto                listener = signal->registerListener([&] {
+        shouldNotRun = signal->registerListener([&] { count += 2; });
+        signal.reset();
+    });
+
+    signal->emit();
+    EXPECT(count, 0);
+}
+
+void signalDestroyedWithRemovedAndAddedListener(int& ret) {
+    int                 count = 0;
+
+    auto                signal = std::make_unique<CSignalT<>>();
+    CHyprSignalListener removed;
+    CHyprSignalListener shouldNotRun;
+
+    auto                listener = signal->registerListener([&] {
+        removed.reset();
+        shouldNotRun = signal->registerListener([&] { count += 2; });
+        signal.reset();
+    });
+
+    removed = signal->registerListener([&] { count += 1; });
+
+    signal->emit();
+    EXPECT(count, 0);
+}
+
 void staticListener(int& ret) {
     struct STestOwner {
         int data = 0;
@@ -88,6 +187,40 @@ void staticListener(int& ret) {
     EXPECT(owner.data, 1);
 }
 
+void staticListenerDestroy(int& ret) {
+    int  data = 0;
+
+    auto signal = makeShared<CSignalT<>>();
+    signal->registerStaticListener([&] { data += 1; });
+
+    signal->registerStaticListener([&] {
+        // should not fire but SHOULD be freed
+        signal->registerStaticListener([&] { data += 3; });
+
+        signal.reset();
+    });
+
+    signal->registerStaticListener([&] { data += 1; });
+
+    signal->emit();
+    EXPECT(data, 2);
+}
+
+void notReentrant(int& ret) {
+    int count = 0;
+
+    CSignalT<> signal;
+    auto listener = signal.registerListener([&] { signal.emit(); });
+
+    try {
+        signal.emit();
+    } catch(std::logic_error& e) {
+        count += 1;
+    }
+
+    EXPECT(count, 1);
+}
+
 int main(int argc, char** argv, char** envp) {
     int ret = 0;
     legacy(ret);
@@ -95,6 +228,13 @@ int main(int argc, char** argv, char** envp) {
     empty(ret);
     typed(ret);
     typedMany(ret);
+    listenerAdded(ret);
+    lastListenerSwapped(ret);
+    signalDestroyed(ret);
+    signalDestroyedWithAddedListener(ret);
+    signalDestroyedWithRemovedAndAddedListener(ret);
     staticListener(ret);
+    staticListenerDestroy(ret);
+    notReentrant(ret);
     return ret;
 }