From 4e4815a25b8236d5f2abe1d1c3bf94f252a91df1 Mon Sep 17 00:00:00 2001 From: outfoxxed Date: Mon, 23 Jun 2025 15:44:59 -0700 Subject: [PATCH] signals: store listeners in a linked list and avoid emit allocs --- include/hyprutils/signal/Listener.hpp | 36 ++++++- include/hyprutils/signal/Signal.hpp | 11 +- src/signal/Listener.cpp | 137 +++++++++++++++++++++++++ src/signal/Signal.cpp | 42 +------- tests/signal.cpp | 140 ++++++++++++++++++++++++++ 5 files changed, 323 insertions(+), 43 deletions(-) diff --git a/include/hyprutils/signal/Listener.hpp b/include/hyprutils/signal/Listener.hpp index 4453896..44308df 100644 --- a/include/hyprutils/signal/Listener.hpp +++ b/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 handler); + // returns true if the list head is still alive + bool emitUntil(void* args, CSignalListenerListHead* head, bool headOwnsListeners); void emitInternal(void* args); std::function m_fHandler; + friend class CSignalListenerListHead; friend class CUntypedSignal; }; diff --git a/include/hyprutils/signal/Signal.hpp b/include/hyprutils/signal/Signal.hpp index e78b48a..5176a8b 100644 --- a/include/hyprutils/signal/Signal.hpp +++ b/include/hyprutils/signal/Signal.hpp @@ -12,14 +12,15 @@ namespace Hyprutils { namespace Signal { class CUntypedSignal { protected: - CHyprSignalListener registerListenerInternal(std::function handler); - void registerStaticListenerInternal(std::function handler); - void emitInternal(void* args); + CHyprSignalListener registerListenerInternal(std::function handler); + void registerStaticListenerInternal(std::function handler); + void emitInternal(void* args); - std::vector> m_vListeners; - std::vector> m_vStaticListeners; + CSignalListenerListHead m_listeners{false}; + CSignalListenerListHead m_staticListeners{true}; }; + // Note: Signals are NOT re-entrant. template class CSignalT : public CUntypedSignal { public: diff --git a/src/signal/Listener.cpp b/src/signal/Listener.cpp index da49943..bd6a8e2 100644 --- a/src/signal/Listener.cpp +++ b/src/signal/Listener.cpp @@ -1,4 +1,5 @@ #include +#include #include using namespace Hyprutils::Signal; @@ -7,6 +8,142 @@ Hyprutils::Signal::CSignalListener::CSignalListener(std::function 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(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(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(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; diff --git a/src/signal/Signal.cpp b/src/signal/Signal.cpp index e9bfea9..79cf90d 100644 --- a/src/signal/Signal.cpp +++ b/src/signal/Signal.cpp @@ -9,52 +9,20 @@ using namespace Hyprutils::Memory; #define WP CWeakPointer void Hyprutils::Signal::CUntypedSignal::emitInternal(void* args) { - std::vector> listeners; - for (auto& l : m_vListeners) { - if (l.expired()) - continue; + auto live = m_listeners.emitAll(args); - listeners.emplace_back(l.lock()); - } - - std::vector 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 handler) { CHyprSignalListener listener = SP(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 handler) { - m_vStaticListeners.emplace_back(std::unique_ptr(new CSignalListener(handler))); + m_staticListeners.append(new CSignalListener(handler)); } void Hyprutils::Signal::CSignal::emit(std::any data) { diff --git a/tests/signal.cpp b/tests/signal.cpp index df8177b..cc8820b 100644 --- a/tests/signal.cpp +++ b/tests/signal.cpp @@ -1,6 +1,10 @@ #include #include #include +#include +#include +#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>(); + + // 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>(); + 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>(); + 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>(); + 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; }