From 85f64b3f3e72a09f578aa5c10214bec85ff68811 Mon Sep 17 00:00:00 2001 From: Leon Matthes Date: Mon, 15 Jul 2024 09:05:16 +0200 Subject: [PATCH 1/2] 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 | 48 ++++++++++++++--- src/kdbindings/connection_handle.h | 32 +++++++----- src/kdbindings/signal.h | 75 ++++++++++++++++++++------- tests/signal/tst_signal.cpp | 10 +++- 4 files changed, 125 insertions(+), 40 deletions(-) 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") From db9891b01c2ec8be7a7b20058033fc2759b4f7ac Mon Sep 17 00:00:00 2001 From: Leon Matthes Date: Mon, 15 Jul 2024 16:52:44 +0200 Subject: [PATCH 2/2] Document that throwing within a slot is UB We currently don't really deal with this correctly. Until we do so, at least mention that it's UB. --- src/kdbindings/connection_evaluator.h | 2 ++ src/kdbindings/signal.h | 16 ++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/src/kdbindings/connection_evaluator.h b/src/kdbindings/connection_evaluator.h index 64b0caa..83ffdb9 100644 --- a/src/kdbindings/connection_evaluator.h +++ b/src/kdbindings/connection_evaluator.h @@ -58,6 +58,8 @@ 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() { diff --git a/src/kdbindings/signal.h b/src/kdbindings/signal.h index 75b2c2d..ace595c 100644 --- a/src/kdbindings/signal.h +++ b/src/kdbindings/signal.h @@ -329,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 const &slot) { @@ -346,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 const &slot) { @@ -371,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 &evaluator, std::function const &slot) { @@ -410,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