deque on deconstruct and nits

KDAB · Dec 18, 2023 · 6a9fef8 · 6a9fef8
1 parent bcbbd34
commit 6a9fef8
Show file tree

Hide file tree

Showing 4 changed files with 90 additions and 28 deletions.
diff --git 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<void()> &connection)
     {
+        std::lock_guard<std::mutex> 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<std::pair<ConnectionHandle, std::function<void()>>> m_deferredConnections;
     std::mutex m_connectionsMutex;

diff --git a/src/kdbindings/connection_handle.h b/src/kdbindings/connection_handle.h
@@ -33,7 +33,7 @@ class SignalImplBase : public std::enable_shared_from_this<SignalImplBase>
 
     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
@@ -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<void(Args...)> slot;
             std::function<void(const ConnectionHandle &, Args...)> slotDeferred;
-            std::weak_ptr<ConnectionEvaluator> m_connectionEvaluator; // Use shared_ptr for stronger ownership.
+            std::weak_ptr<ConnectionEvaluator> 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<ConnectionEvaluator> &evaluator, std::function<void(const ConnectionHandle &, Args...)> const &slot)
+    ConnectionHandle connectDeferred(const std::shared_ptr<ConnectionEvaluator> &evaluator, std::function<void(Args...)> 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
@@ -100,14 +100,14 @@ TEST_CASE("Signal connections")
         int val = 4;
         auto evaluator = std::make_shared<ConnectionEvaluator>();
 
-    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<ConnectionEvaluator>();
 
         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<ConnectionEvaluator>();
 
-        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<ConnectionEvaluator>();
 
-        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<ConnectionEvaluator>();
 
-        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<ConnectionEvaluator>();
+        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<std::string, int> signal;