From 6a9fef8e9edebaeb665ba341255249af319c34fe Mon Sep 17 00:00:00 2001 From: Shivam Kunwar Date: Fri, 15 Dec 2023 20:54:26 +0530 Subject: [PATCH] deque on deconstruct and nits --- src/kdbindings/connection_evaluator.h | 6 +++ src/kdbindings/connection_handle.h | 4 +- src/kdbindings/signal.h | 66 +++++++++++++++++++++------ tests/signal/tst_signal.cpp | 42 +++++++++++------ 4 files changed, 90 insertions(+), 28 deletions(-) diff --git a/src/kdbindings/connection_evaluator.h b/src/kdbindings/connection_evaluator.h index cfe19e3..c01abf3 100644 --- a/src/kdbindings/connection_evaluator.h +++ b/src/kdbindings/connection_evaluator.h @@ -58,6 +58,7 @@ class ConnectionEvaluator void enqueueSlotInvocation(const ConnectionHandle &handle, const std::function &connection) { + std::lock_guard lock(m_connectionsMutex); m_deferredConnections.push_back({ handle, std::move(connection) }); } @@ -74,6 +75,11 @@ class ConnectionEvaluator std::remove_if(m_deferredConnections.begin(), m_deferredConnections.end(), handleMatches), m_deferredConnections.end()); } + void disconnectAllDeferred() + { + // Clear the vector of deferred connections + m_deferredConnections.clear(); + } std::vector>> m_deferredConnections; std::mutex m_connectionsMutex; diff --git a/src/kdbindings/connection_handle.h b/src/kdbindings/connection_handle.h index 2a56429..27abdbd 100644 --- a/src/kdbindings/connection_handle.h +++ b/src/kdbindings/connection_handle.h @@ -33,7 +33,7 @@ class SignalImplBase : public std::enable_shared_from_this virtual ~SignalImplBase() = default; - virtual void disconnect(const GenerationalIndex &id, const ConnectionHandle &handle) = 0; + virtual void disconnect(const ConnectionHandle &handle) = 0; virtual bool blockConnection(const GenerationalIndex &id, bool blocked) = 0; virtual bool isConnectionActive(const GenerationalIndex &id) const = 0; virtual bool isConnectionBlocked(const GenerationalIndex &id) const = 0; @@ -86,7 +86,7 @@ class ConnectionHandle { if (m_id.has_value()) { if (auto shared_impl = checkedLock()) { - shared_impl->disconnect(*m_id, *this); + shared_impl->disconnect(*this); } } diff --git a/src/kdbindings/signal.h b/src/kdbindings/signal.h index 8cc3aad..9f8e2ea 100644 --- a/src/kdbindings/signal.h +++ b/src/kdbindings/signal.h @@ -115,18 +115,26 @@ class Signal } // Disconnects a previously connected function - void disconnect(const Private::GenerationalIndex &id, const ConnectionHandle &handle) override + void disconnect(const ConnectionHandle &handle) override { // If the connection evaluator is still valid, remove any queued up slot invocations // associated with the given handle to prevent them from being evaluated in the future. - auto connection = m_connections.get(id); - if (connection->slotDeferred) { - if (auto evaluatorPtr = connection->m_connectionEvaluator.lock()) { + auto idOpt = handle.m_id; // Retrieve the connection associated with this id + + // Proceed only if the id is valid + if (idOpt.has_value()) { + auto id = idOpt.value(); - evaluatorPtr->dequeueSlotInvocation(handle); + // Retrieve the connection associated with this id + auto connection = m_connections.get(id); + if (connection && connection->slotDeferred) { + if (auto evaluatorPtr = connection->m_connectionEvaluator.lock()) { + evaluatorPtr->dequeueSlotInvocation(handle); + } } + + m_connections.erase(id); } - m_connections.erase(id); } // Disconnects all previously connected functions @@ -135,6 +143,29 @@ class Signal m_connections.clear(); } + void disconnectAllConnections() + { + const auto numEntries = m_connections.entriesSize(); + + for (auto i = decltype(numEntries){ 0 }; i < numEntries; ++i) { + const auto indexOpt = m_connections.indexAtEntry(i); + if (indexOpt) { + const auto con = m_connections.get(*indexOpt); + if (con) { + if (con->slot) { + m_connections.clear(); + } + + if (con->slotDeferred && con->m_connectionEvaluator.lock()) { + con->m_connectionEvaluator.lock()->disconnectAllDeferred(); + } + + m_connections.erase(*indexOpt); + } + } + } + } + bool blockConnection(const Private::GenerationalIndex &id, bool blocked) override { Connection *connection = m_connections.get(id); @@ -162,7 +193,7 @@ class Signal } } - void emit(Args... p) /*const*/ + void emit(Args... p) { const auto numEntries = m_connections.entriesSize(); @@ -195,7 +226,7 @@ class Signal struct Connection { std::function slot; std::function slotDeferred; - std::weak_ptr m_connectionEvaluator; // Use shared_ptr for stronger ownership. + std::weak_ptr m_connectionEvaluator; bool blocked{ false }; }; @@ -224,7 +255,11 @@ class Signal */ ~Signal() { - disconnectAll(); + if (m_impl) { + m_impl->disconnectAllConnections(); + } else { + disconnectAll(); + } } /** @@ -259,13 +294,18 @@ class Signal * The `KDBindings::Signal` class itself is not thread-safe. While the `ConnectionEvaluator` is inherently * thread-safe, ensure that any concurrent access to this Signal is protected externally to maintain thread safety. */ - ConnectionHandle connectDeferred(const std::shared_ptr &evaluator, std::function const &slot) + ConnectionHandle connectDeferred(const std::shared_ptr &evaluator, std::function const &slot) { ensureImpl(); - // Create a new ConnectionHandle instance for this connection + // Create a wrapper function that includes the ConnectionHandle + auto wrapper = [slot](const ConnectionHandle &handle, Args... args) { + slot(args...); + }; + + // Call connectDeferred on the implementation with the wrapper function ConnectionHandle handle(m_impl, {}); - handle.setId(m_impl->connectDeferred(evaluator, slot)); + handle.setId(m_impl->connectDeferred(evaluator, wrapper)); return handle; } @@ -322,7 +362,7 @@ class Signal void disconnect(const ConnectionHandle &handle) { if (m_impl && handle.belongsTo(*this) && handle.m_id.has_value()) { - m_impl->disconnect(*handle.m_id, handle); + m_impl->disconnect(handle); // TODO check if Impl is now empty and reset } else { throw std::out_of_range("Provided ConnectionHandle does not match any connection\nLikely the connection was deleted before!"); diff --git a/tests/signal/tst_signal.cpp b/tests/signal/tst_signal.cpp index 9cf9266..ec6cb6b 100644 --- a/tests/signal/tst_signal.cpp +++ b/tests/signal/tst_signal.cpp @@ -100,14 +100,14 @@ TEST_CASE("Signal connections") int val = 4; auto evaluator = std::make_shared(); - auto connection1 = signal1.connectDeferred(evaluator, [&val](const ConnectionHandle& handle, int value) { - val += value; - }); + auto connection1 = signal1.connectDeferred(evaluator, [&val](int value) { + val += value; + }); - auto connection2 = signal2.connectDeferred(evaluator, [&val](const ConnectionHandle& handle, int value1, int value2) { - val += value1; - val += value2; - }); + auto connection2 = signal2.connectDeferred(evaluator, [&val](int value1, int value2) { + val += value1; + val += value2; + }); REQUIRE(connection1.isActive()); @@ -134,13 +134,13 @@ TEST_CASE("Signal connections") auto evaluator = std::make_shared(); std::thread thread1([&] { - signal1.connectDeferred(evaluator, [&val](const ConnectionHandle& handle, int value) { + signal1.connectDeferred(evaluator, [&val](int value) { val += value; }); }); std::thread thread2([&] { - signal2.connectDeferred(evaluator, [&val](const ConnectionHandle& handle, int value) { + signal2.connectDeferred(evaluator, [&val](int value) { val += value; }); }); @@ -165,11 +165,11 @@ TEST_CASE("Signal connections") int val2 = 4; auto evaluator = std::make_shared(); - signal1.connectDeferred(evaluator, [&val1](const ConnectionHandle& handle, int value) { + signal1.connectDeferred(evaluator, [&val1](int value) { val1 += value; }); - signal2.connectDeferred(evaluator, [&val2](const ConnectionHandle& handle, int value) { + signal2.connectDeferred(evaluator, [&val2](int value) { val2 += value; }); @@ -199,7 +199,7 @@ TEST_CASE("Signal connections") int val = 4; auto evaluator = std::make_shared(); - auto connection = signal.connectDeferred(evaluator, [&val](const ConnectionHandle& handle, int value) { + auto connection = signal.connectDeferred(evaluator, [&val](int value) { val += value; }); @@ -220,7 +220,7 @@ TEST_CASE("Signal connections") int val = 4; auto evaluator = std::make_shared(); - signal.connectDeferred(evaluator, [&val](const ConnectionHandle& handle, int value) { + signal.connectDeferred(evaluator, [&val](int value) { val += value; }); @@ -233,6 +233,22 @@ TEST_CASE("Signal connections") REQUIRE(val == 6); } + SUBCASE("Disconnect deferred connection from deleted signal") + { + auto signal = new Signal<>(); + auto evaluator = std::make_shared(); + bool called = false; + + auto handle = signal->connectDeferred(evaluator, [&called]() { called = true; }); + signal->emit(); + delete signal; + + handle.disconnect(); + REQUIRE(called == false); + evaluator->evaluateDeferredConnections(); + REQUIRE(called == false); + } + SUBCASE("A signal with arguments can be connected to a lambda and invoked with l-value args") { Signal signal;