Add noexcept specifiers where appropriate

src/kdbindings/connection_evaluator.h

@@ -58,15 +58,35 @@ class ConnectionEvaluator
      *
      * This function is responsible for evaluating and executing deferred connections.
      * This function is thread safe.
+     *
+     * @warning Evaluating slots that throw an exception is currently undefined behavior.
      */
     void evaluateDeferredConnections()
     {
-        std::lock_guard<std::mutex> lock(m_slotInvocationMutex);
+        std::lock_guard<std::recursive_mutex> lock(m_slotInvocationMutex);
 
-        for (auto &pair : m_deferredSlotInvocations) {
-            pair.second();
+        if (m_isEvaluating) {
+            // We're already evaluating, so we don't want to re-enter this function.
+            return;
+        }
+        m_isEvaluating = true;
+
+        // Current best-effort error handling will remove any further invocations that were queued.
+        // We could use a queue and use a `while(!empty) { pop_front() }` loop instead to avoid this.
+        // However, we would then ideally use a ring-buffer to avoid excessive allocations, which isn't in the STL.
+        try {
+            for (auto &pair : m_deferredSlotInvocations) {
+                pair.second();
+            }
+        } catch (...) {
+            // Best-effort: Reset the ConnectionEvaluator so that it at least doesn't execute the same erroneous slot multiple times.
+            m_deferredSlotInvocations.clear();
+            m_isEvaluating = false;
+            throw;
         }
+
         m_deferredSlotInvocations.clear();
+        m_isEvaluating = false;
     }
 
 protected:
@@ -94,15 +114,27 @@ class ConnectionEvaluator
     void enqueueSlotInvocation(const ConnectionHandle &handle, const std::function<void()> &slotInvocation)
     {
         {
-            std::lock_guard<std::mutex> lock(m_slotInvocationMutex);
+            std::lock_guard<std::recursive_mutex> lock(m_slotInvocationMutex);
             m_deferredSlotInvocations.push_back({ handle, std::move(slotInvocation) });
         }
         onInvocationAdded();
     }
 
-    void dequeueSlotInvocation(const ConnectionHandle &handle)
+    // Note: This function is marked with noexcept but may theoretically encounter an exception and terminate the program if locking the mutex fails.
+    // If this does happen though, there's likely something very wrong, so std::terminate is actually a reasonable way to handle this.
+    //
+    // In addition, we do need to use a recursive_mutex, as otherwise a slot from `enqueueSlotInvocation` may theoretically call this function and cause undefined behavior.
+    void dequeueSlotInvocation(const ConnectionHandle &handle) noexcept
     {
-        std::lock_guard<std::mutex> lock(m_slotInvocationMutex);
+        std::lock_guard<std::recursive_mutex> lock(m_slotInvocationMutex);
+
+        if (m_isEvaluating) {
+            // It's too late, we're already evaluating the deferred connections.
+            // We can't remove the invocation now, as it might be currently evaluated.
+            // And removing any invocations would be undefined behavior as we would invalidate
+            // the loop indices in `evaluateDeferredConnections`.
+            return;
+        }
 
         auto handleMatches = [&handle](const auto &invocationPair) {
             return invocationPair.first == handle;
@@ -115,6 +147,10 @@ class ConnectionEvaluator
     }
 
     std::vector<std::pair<ConnectionHandle, std::function<void()>>> m_deferredSlotInvocations;
-    std::mutex m_slotInvocationMutex;
+    // We need to use a recursive mutex here, as `evaluateDeferredConnections` executes arbitrary user code.
+    // This may end up in a call to dequeueSlotInvocation, which locks the same mutex.
+    // We'll also need to add a flag to make sure we don't actually dequeue invocations while we're evaluating them.
+    std::recursive_mutex m_slotInvocationMutex;
+    bool m_isEvaluating = false;
 };
 } // namespace KDBindings

src/kdbindings/connection_handle.h

@@ -36,9 +36,9 @@ class SignalImplBase : public std::enable_shared_from_this<SignalImplBase>
 
     virtual ~SignalImplBase() = default;
 
-    virtual void disconnect(const ConnectionHandle &handle) = 0;
+    virtual void disconnect(const ConnectionHandle &handle) noexcept = 0;
     virtual bool blockConnection(const GenerationalIndex &id, bool blocked) = 0;
-    virtual bool isConnectionActive(const GenerationalIndex &id) const = 0;
+    virtual bool isConnectionActive(const GenerationalIndex &id) const noexcept = 0;
     virtual bool isConnectionBlocked(const GenerationalIndex &id) const = 0;
 };
 
@@ -64,14 +64,14 @@ class ConnectionHandle
     /**
      * A ConnectionHandle can be copied.
      **/
-    ConnectionHandle(const ConnectionHandle &) = default;
-    ConnectionHandle &operator=(const ConnectionHandle &) = default;
+    ConnectionHandle(const ConnectionHandle &) noexcept = default;
+    ConnectionHandle &operator=(const ConnectionHandle &) noexcept = default;
 
     /**
      * A ConnectionHandle can be moved.
      **/
-    ConnectionHandle(ConnectionHandle &&) = default;
-    ConnectionHandle &operator=(ConnectionHandle &&) = default;
+    ConnectionHandle(ConnectionHandle &&) noexcept = default;
+    ConnectionHandle &operator=(ConnectionHandle &&) noexcept = default;
 
     /**
      * Disconnect the slot.
@@ -84,8 +84,11 @@ class ConnectionHandle
      *
      * After this call, the ConnectionHandle will be inactive (i.e. isActive() returns false)
      * and will no longer belong to any Signal (i.e. belongsTo returns false).
+     *
+     * @warning While this function is marked with noexcept, it *may* terminate the program
+     * if it is not possible to allocate memory or if mutex locking isn't possible.
      **/
-    void disconnect()
+    void disconnect() noexcept
     {
         if (auto shared_impl = checkedLock()) {
             shared_impl->disconnect(*this);
@@ -196,7 +199,7 @@ class ConnectionHandle
 
     // Checks that the weak_ptr can be locked and that the connection is
     // still active
-    std::shared_ptr<Private::SignalImplBase> checkedLock() const
+    std::shared_ptr<Private::SignalImplBase> checkedLock() const noexcept
     {
         if (m_id.has_value()) {
             auto shared_impl = m_signalImpl.lock();
@@ -228,15 +231,15 @@ class ScopedConnection
     ScopedConnection() = default;
 
     /** A ScopedConnection can be move constructed */
-    ScopedConnection(ScopedConnection &&) = default;
+    ScopedConnection(ScopedConnection &&) noexcept = default;
 
     /** A ScopedConnection cannot be copied */
     ScopedConnection(const ScopedConnection &) = delete;
     /** A ScopedConnection cannot be copied */
     ScopedConnection &operator=(const ScopedConnection &) = delete;
 
     /** A ScopedConnection can be move assigned */
-    ScopedConnection &operator=(ScopedConnection &&other)
+    ScopedConnection &operator=(ScopedConnection &&other) noexcept
     {
         m_connection.disconnect();
         m_connection = std::move(other.m_connection);
@@ -246,15 +249,15 @@ class ScopedConnection
     /**
      * A ScopedConnection can be constructed from a ConnectionHandle
      */
-    ScopedConnection(ConnectionHandle &&h)
+    ScopedConnection(ConnectionHandle &&h) noexcept
         : m_connection(std::move(h))
     {
     }
 
     /**
      * A ScopedConnection can be assigned from a ConnectionHandle
      */
-    ScopedConnection &operator=(ConnectionHandle &&h)
+    ScopedConnection &operator=(ConnectionHandle &&h) noexcept
     {
         return *this = ScopedConnection(std::move(h));
     }
@@ -293,8 +296,11 @@ class ScopedConnection
 
     /**
      * When a ConnectionHandle is destructed it disconnects the connection it guards.
+     *
+     * @warning While this function isn't marked as throwing, it *may* throw and terminate the program
+     * if it is not possible to allocate memory or if mutex locking isn't possible.
      */
-    ~ScopedConnection()
+    ~ScopedConnection() noexcept
     {
         m_connection.disconnect();
     }

src/kdbindings/signal.h

@@ -135,13 +135,11 @@ class Signal
         }
 
         // Disconnects a previously connected function
-        void disconnect(const ConnectionHandle &handle) override
+        //
+        // WARNING: While this function is marked with noexcept, it *may* terminate the program
+        // if it is not possible to allocate memory or if mutex locking isn't possible.
+        void disconnect(const ConnectionHandle &handle) noexcept override
         {
-            if (m_isEmitting) {
-                m_connectionsToDisconnect.push_front(handle);
-                return;
-            }
-
             // 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 idOpt = handle.m_id; // Retrieve the connection associated with this id
@@ -152,18 +150,31 @@ class Signal
 
                 // Retrieve the connection associated with this id
                 auto connection = m_connections.get(id);
+                if (connection && m_isEmitting) {
+                    // We are currently still emitting the signal, so we need to defer the actual
+                    // disconnect until the emit is done.
+                    connection->toBeDisconnected = true;
+                    m_disconnectedDuringEmit = true;
+                    return;
+                }
+
                 if (connection && connection->m_connectionEvaluator.lock()) {
                     if (auto evaluatorPtr = connection->m_connectionEvaluator.lock()) {
                         evaluatorPtr->dequeueSlotInvocation(handle);
                     }
                 }
 
+                // Note: This function may throw if we're out of memory.
+                // As `disconnect` is marked as `noexcept`, this will terminate the program.
                 m_connections.erase(id);
             }
         }
 
         // Disconnects all previously connected functions
-        void disconnectAll()
+        //
+        // WARNING: While this function is marked with noexcept, it *may* terminate the program
+        // if it is not possible to allocate memory or if mutex locking isn't possible.
+        void disconnectAll() noexcept
         {
             const auto numEntries = m_connections.entriesSize();
 
@@ -188,7 +199,7 @@ class Signal
             }
         }
 
-        bool isConnectionActive(const Private::GenerationalIndex &id) const override
+        bool isConnectionActive(const Private::GenerationalIndex &id) const noexcept override
         {
             return m_connections.get(id);
         }
@@ -212,8 +223,8 @@ class Signal
 
             const auto numEntries = m_connections.entriesSize();
 
-            // This loop can tolerate signal handles being disconnected inside a slot,
-            // but adding new connections to a signal inside a slot will still be undefined behaviour
+            // This loop can *not* tolerate new connections being added to the signal inside a slot
+            // Doing so will be undefined behavior
             for (auto i = decltype(numEntries){ 0 }; i < numEntries; ++i) {
                 const auto index = m_connections.indexAtEntry(i);
 
@@ -234,10 +245,22 @@ class Signal
             }
             m_isEmitting = false;
 
-            // Remove all slots that were disconnected during the emit
-            while (!m_connectionsToDisconnect.empty()) {
-                disconnect(m_connectionsToDisconnect.front());
-                m_connectionsToDisconnect.pop_front();
+            if (m_disconnectedDuringEmit) {
+                m_disconnectedDuringEmit = false;
+
+                // Because m_connections is using a GenerationIndexArray, this loop can tolerate
+                // deletions inside the loop. So iterating over the array and deleting entries from it
+                // should not lead to undefined behavior.
+                for (auto i = decltype(numEntries){ 0 }; i < numEntries; ++i) {
+                    const auto index = m_connections.indexAtEntry(i);
+
+                    if (index.has_value()) {
+                        const auto con = m_connections.get(index.value());
+                        if (con->toBeDisconnected) {
+                            disconnect(ConnectionHandle(shared_from_this(), index));
+                        }
+                    }
+                }
             }
         }
 
@@ -248,6 +271,9 @@ class Signal
             std::function<void(ConnectionHandle &, Args...)> slotReflective;
             std::weak_ptr<ConnectionEvaluator> m_connectionEvaluator;
             bool blocked{ false };
+            // When we disconnect while the signal is still emitting, we need to defer the actual disconnection
+            // until the emit is done. This flag is set to true when the connection should be disconnected.
+            bool toBeDisconnected{ false };
         };
 
         mutable Private::GenerationalIndexArray<Connection> m_connections;
@@ -256,10 +282,15 @@ class Signal
         // while it is still running.
         // Therefore, defer all slot disconnections until the emit is done.
         //
-        // We deliberately choose a forward_list here for the smaller memory footprint when empty.
-        // This list will only be used as a LIFO stack, which is also O(1) for forward_list.
-        std::forward_list<ConnectionHandle> m_connectionsToDisconnect;
+        // Previously, we stored the ConnectionHandles that were to be disconnected in a list.
+        // However, that would mean that disconnecting cannot be noexcept, as it may need to allocate memory in that list.
+        // We can fix this by storing this information within each connection itself (the toBeDisconnected flag).
+        // Because of the memory layout of the `struct Connection`, this shouldn't even use any more memory.
+        //
+        // The only downside is that we need to iterate over all connections to find the ones that need to be disconnected.
+        // This is helped by using the m_disconnedDuringEmit flag to avoid unnecessary iterations.
         bool m_isEmitting = false;
+        bool m_disconnectedDuringEmit = false;
     };
 
 public:
@@ -281,8 +312,11 @@ class Signal
      *
      * Therefore, all active ConnectionHandles that belonged to this Signal
      * will no longer be active (i.e. ConnectionHandle::isActive will return false).
+     *
+     * @warning While this function isn't marked as throwing, it *may* throw and terminate the program
+     * if it is not possible to allocate memory or if mutex locking isn't possible.
      */
-    ~Signal()
+    ~Signal() noexcept
     {
         disconnectAll();
     }
@@ -295,6 +329,10 @@ class Signal
      *
      * @return An instance of ConnectionHandle, that can be used to disconnect
      * or temporarily block the connection.
+     *
+     * @warning Connecting functions to a signal that throw an exception when called is currently undefined behavior.
+     * All connected functions should handle their own exceptions.
+     * For backwards-compatibility, the slot function is not required to be noexcept.
      */
     KDBINDINGS_WARN_UNUSED ConnectionHandle connect(std::function<void(Args...)> const &slot)
     {
@@ -312,6 +350,10 @@ class Signal
      *
      * @param slot A std::function that takes a ConnectionHandle reference followed by the signal's parameter types.
      * @return A ConnectionHandle to the newly established connection, allowing for advanced connection management.
+     *
+     * @warning Connecting functions to a signal that throw an exception when called is currently undefined behavior.
+     * All connected functions should handle their own exceptions.
+     * For backwards-compatibility, the slot function is not required to be noexcept.
      */
     ConnectionHandle connectReflective(std::function<void(ConnectionHandle &, Args...)> const &slot)
     {
@@ -337,6 +379,10 @@ class Signal
      * @note
      * The 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.
+     *
+     * @warning Connecting functions to a signal that throw an exception when called is currently undefined behavior.
+     * All connected functions should handle their own exceptions.
+     * For backwards-compatibility, the slot function is not required to be noexcept.
      */
     KDBINDINGS_WARN_UNUSED ConnectionHandle connectDeferred(const std::shared_ptr<ConnectionEvaluator> &evaluator, std::function<void(Args...)> const &slot)
     {
@@ -376,6 +422,10 @@ class Signal
      * @return An instance of a Signal::ConnectionHandle that refers to this connection.
      *          Warning: When connecting a member function you must use the returned ConnectionHandle
      *          to disconnect when the object containing the slot goes out of scope!
+     *
+     * @warning Connecting functions to a signal that throw an exception when called is currently undefined behavior.
+     * All connected functions should handle their own exceptions.
+     * For backwards-compatibility, the slot function is not required to be noexcept.
      **/
     // The enable_if_t makes sure that this connect function specialization is only
     // available if we provide a function that cannot be otherwise converted to a
@@ -412,8 +462,11 @@ class Signal
      *
      * All currently active ConnectionHandles that belong to this Signal will no
      * longer be active afterwards. (i.e. ConnectionHandle::isActive will return false).
+     *
+     * @warning While this function is marked with noexcept, it *may* terminate the program
+     * if it is not possible to allocate memory or if mutex locking isn't possible.
      */
-    void disconnectAll()
+    void disconnectAll() noexcept
     {
         if (m_impl) {
             m_impl->disconnectAll();