Add noexcept specifiers where appropriate

We can mark the following as `noexcept`:
- ~ScopedConnection
- ~Signal
- ConnectionHandle::disconnect
- several move constructors

There are some caveats though:
When disconnecting, we need to lock the mutex in the ConnectionEvaluator
and allocate in the GenerationalIndexArray, which can both throw
exceptions.
However, we currently do not have any out of memory guarantees in
KDBindings, so terminating in that case should be a sufficient way of
handling this case.
The same goes for when locking a mutex isn't possible. This likely
indicates some kind of OS issue from which we can't really recover, so
terminating is appropriate.

We could improve the situation by avoiding allocations in the GenerationalIndexArray
when erasing slots.
This would be possible by using an inline linked-list inside the array
to store free indices.
Which would save memory and would no longer require us to allocate
memory when erasing entries.
However, it's also not trivial to implement, so leave this as a TODO.

Error checking in ConnectionEvaluator
-------------------------------------
This patch also adds some basic exception handling to the
ConnectionEvaluator.
It is now somewhat protected from slots disconnecting themselves while they are
evaluated and slots throwing exceptions.
However, this error-handling is rather basic and will simply **not**
dequeue any slots while the evaluator is already evaluating and will
dequeue all slots if any slot throws an exception.
We could improve this situation by using a queue and popping elements 1-by-1
when evaluating without actually iterating over it.
That way we could erase from the queue while its being evaluated.
And when a slot throws, calling evaluate again could simply continue
with the next slot, instead of skipping all queued slots.

However, we should ideally implement this queue as a ring-buffer to
avoid excessive allocations by std::list or std::dequeu.
But this isn't part of the STL, so would require us to roll our own
ring-buffer implementation.

For now, this error handling isn't publicly documented and throwing
within a slot or dequeuing while evaluating remains *undefined*,
so that we can improve the situation in the future.

src/kdbindings/connection_evaluator.h

@@ -61,12 +61,30 @@ class ConnectionEvaluator
      */
     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 +112,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 +145,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 is marked with noexcept, it *may* 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 is marked with noexcept, it *may* terminate the program
+     * if it is not possible to allocate memory or if mutex locking isn't possible.
      */
-    ~Signal()
+    ~Signal() noexcept
     {
         disconnectAll();
     }
@@ -412,8 +446,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();

tests/signal/tst_signal.cpp

@@ -45,7 +45,7 @@ static_assert(std::is_nothrow_default_constructible<ScopedConnection>{});
 static_assert(!std::is_copy_constructible<ScopedConnection>{});
 static_assert(!std::is_copy_assignable<ScopedConnection>{});
 static_assert(std::is_nothrow_move_constructible<ScopedConnection>{});
-static_assert(!std::is_nothrow_move_assignable<ScopedConnection>{});
+static_assert(std::is_nothrow_move_assignable<ScopedConnection>{});
 
 class Button
 {
@@ -513,6 +513,14 @@ TEST_CASE("ConnectionEvaluator")
         evaluator->evaluateDeferredConnections();
 
         REQUIRE(val == 6);
+
+        signal.emit(2);
+        REQUIRE(val == 6);
+
+        evaluator->evaluateDeferredConnections();
+        evaluator->evaluateDeferredConnections();
+
+        REQUIRE(val == 8);
     }
 
     SUBCASE("Disconnect deferred connection from deleted signal")