diff --git a/src/kdbindings/connection_evaluator.h b/src/kdbindings/connection_evaluator.h index 857218e..64b0caa 100644 --- a/src/kdbindings/connection_evaluator.h +++ b/src/kdbindings/connection_evaluator.h @@ -61,12 +61,30 @@ class ConnectionEvaluator */ void evaluateDeferredConnections() { - std::lock_guard lock(m_slotInvocationMutex); + std::lock_guard 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 &slotInvocation) { { - std::lock_guard lock(m_slotInvocationMutex); + std::lock_guard 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 lock(m_slotInvocationMutex); + std::lock_guard 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>> 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 diff --git a/src/kdbindings/connection_handle.h b/src/kdbindings/connection_handle.h index 81eb447..57750be 100644 --- a/src/kdbindings/connection_handle.h +++ b/src/kdbindings/connection_handle.h @@ -36,9 +36,9 @@ class SignalImplBase : public std::enable_shared_from_this 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 checkedLock() const + std::shared_ptr checkedLock() const noexcept { if (m_id.has_value()) { auto shared_impl = m_signalImpl.lock(); @@ -228,7 +231,7 @@ 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; @@ -236,7 +239,7 @@ class ScopedConnection 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,7 +249,7 @@ class ScopedConnection /** * A ScopedConnection can be constructed from a ConnectionHandle */ - ScopedConnection(ConnectionHandle &&h) + ScopedConnection(ConnectionHandle &&h) noexcept : m_connection(std::move(h)) { } @@ -254,7 +257,7 @@ class ScopedConnection /** * 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(); } diff --git a/src/kdbindings/signal.h b/src/kdbindings/signal.h index d0786ec..75b2c2d 100644 --- a/src/kdbindings/signal.h +++ b/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 slotReflective; std::weak_ptr 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 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 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(); } @@ -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(); diff --git a/tests/signal/tst_signal.cpp b/tests/signal/tst_signal.cpp index 252aafd..c551817 100644 --- a/tests/signal/tst_signal.cpp +++ b/tests/signal/tst_signal.cpp @@ -45,7 +45,7 @@ static_assert(std::is_nothrow_default_constructible{}); static_assert(!std::is_copy_constructible{}); static_assert(!std::is_copy_assignable{}); static_assert(std::is_nothrow_move_constructible{}); -static_assert(!std::is_nothrow_move_assignable{}); +static_assert(std::is_nothrow_move_assignable{}); 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")