From e6e7e41f311939e83297ae4b21780d75e323f23b Mon Sep 17 00:00:00 2001 From: Rafael Medina Date: Sat, 6 Jul 2024 12:05:05 +0200 Subject: [PATCH] Allow component tree to be alocated in the heap. Extra care must be taken to prevent invalid free() calls when mixing both heap and stack tree allocations. --- docs/base-package.md | 13 +++++- include/kouta/base/branch.hpp | 21 ++++++--- include/kouta/base/component.hpp | 65 ++++++++++++++++++++++++-- include/kouta/base/root.hpp | 18 ++++++- tests/base/dummy-component.cpp | 10 ++++ tests/base/dummy-component.hpp | 14 +++++- tests/base/test-base.cpp | 80 +++++++++++++++++++++++++++++++- 7 files changed, 206 insertions(+), 15 deletions(-) diff --git a/docs/base-package.md b/docs/base-package.md index 2a39d12..b7fb3f8 100644 --- a/docs/base-package.md +++ b/docs/base-package.md @@ -10,6 +10,12 @@ Implemented in `kouta::base::Component`. This is the **base type for any asynchronous element that requires access to the event loop**. A `Component` **does not own the I/O context**, and instead is expected to have a **parent** from which it can reference said context (hence the aforementioned tree-like architecture). +Apart from providing access to the event loop, the **parent** also **keeps track of its children**. This allows for components to be allocated in the **heap** via the `new` keyword if desired so that, when a `Component` is deleted, it will automatically attempt to `delete` its children and remove itself from its parent. + +In the case of objects allocated in the **stack**, each object will be deleted in reverse creation order, which should prevent any issues with calling `delete` on non-heap objects due to objects removing themselves from the parent's children list. + +**Note**: take extra care when mixing stack and heap allocations, as this may cause invalid deletions. + These components can be chained as many times as needed, as long as there is one parent at the root of the tree which provides the I/O context (see `kouta::base::Component::context()`). Note that all components deriving from a single parent will be executed **in the same thread**. @@ -53,6 +59,8 @@ Implemented in `kouta::base::Root`. As opposed to the base `Component` explained above, the `Root` **owns the I/O context** and is supposed to serve as the **parent** for other components that want to share the same event loop. +While a **parent** can be provided to the root, it is only used when dealing with heap-allocated objects in order to guarantee that the `Root` is deleted with its parent. + Note that starting the event loop of a `Root` (`kouta::base::Root::run()`) **blocks the current thread** and will only be unblocked after the event loop is terminated (e.g. via the `kouta::base::Root::stop()` method). ```cpp @@ -111,7 +119,10 @@ The *problem* with the `Root` type is that there is a one-to-one relation betwee The `Branch` type was introduced for such cases. It **is a `Root`** (owns the I/O context), but manages an `std::thread` internally. As opposed to the `Root`, starting the event loop via `kouta::base::Branch::run()` **will not block**, and instead start the event loop in the child thread. -It is supposed to wrap a `Component`, allowing external components to post events to the wrapped `Component`, as well as to the `Branch` itself. +It is supposed to wrap a `Component`, allowing external components to post events to the wrapped `Component`, as well as to the `Branch` itself. In addition, the `Branch` assumes that the wrapped `Component` expects a pointer to a parent as its first argument (which will be set to the `Branch` itself). + +The **parent** provided to the `Branch` is only used when dealing with heap-allocated objects in order to guarantee that the `Root` is deleted with its parent. + ```cpp #include diff --git a/include/kouta/base/branch.hpp b/include/kouta/base/branch.hpp index 5866b04..0f504ec 100644 --- a/include/kouta/base/branch.hpp +++ b/include/kouta/base/branch.hpp @@ -18,7 +18,8 @@ namespace kouta::base /// method. As opposed to the original method defined in @ref Root, the one specified here will launch the thread /// (and the event loop) and return immediately. /// - /// @tparam TWrapped Wrapped @ref Component type. + /// @tparam TWrapped Wrapped @ref Component type. IT is assumed that the first argument of the component + /// will be a pointer to a parent component (which will be set to this Branch). template requires std::is_base_of_v class Branch : public Root @@ -31,14 +32,20 @@ namespace kouta::base /// @brief Branch component constructor. /// - /// @tparam TArgs Types of the arguments to provide the wrapped component. + /// @details + /// This constructor will register the Branch object with the parent **only to manage the memory deallocation** + /// in case the object was allocated on the heap. Regardless of having a parent, the Branch owns its event loop. + /// + /// @tparam TArgs Types of the arguments to provide the wrapped component. /// - /// @param[in] args Arguments to provide the wrapped component. + /// @param[in] parent Parent component. The lifetime of the parent must surpass that of the + /// child. + /// @param[in] args Arguments to provide the wrapped component. template - Branch(TArgs... args) - : Root{} + Branch(Component* parent, TArgs... args) + : Root{parent} , m_worker{} - , m_component{args...} + , m_component{this, args...} // Assuming first argument is the parent component { } @@ -109,7 +116,7 @@ namespace kouta::base m_component.post(method, args...); } - /// @brief Inherit @ref Root::post() to allow posting events to the worker itself. + /// @brief Inherit @ref Root::post() to allow posting events to the branch itself. /// /// @note This may be used to post a call to the @ref stop() method. using Root::post; diff --git a/include/kouta/base/component.hpp b/include/kouta/base/component.hpp index 62fb0c6..cb64b8a 100644 --- a/include/kouta/base/component.hpp +++ b/include/kouta/base/component.hpp @@ -1,5 +1,7 @@ #pragma once +#include + #include namespace kouta::base @@ -8,6 +10,8 @@ namespace kouta::base /// /// @details /// A component provies access to the underlying event loop which, by default, belongs to the parent component. + /// Moreover, specifying a parent will add the component to its children list and make sure that the component is + /// deleted when the parent is destroyed (if the component was allocated on the heap). class Component { public: @@ -16,11 +20,14 @@ namespace kouta::base /// @brief Constructor. /// - /// @param[in] parent Parent component. Note that the parent provides access to the event loop, - /// hence its lifetime must, at least, surpass that of the child. - explicit Component(Component* parent = nullptr) + /// @param[in] parent Parent component. The lifetime of the parent must surpass that of the child. + explicit Component(Component* parent) : m_parent{parent} { + if (m_parent) + { + m_parent->add_child(this); + } } // Not copyable @@ -31,7 +38,32 @@ namespace kouta::base Component(Component&&) = delete; Component& operator=(Component&&) = delete; - virtual ~Component() = default; + /// @brief Component destructor. + /// + /// @details + /// The component will go through its list of children and delete them one at a time. This is only useful if the + /// components were allocated in the heap, as stack-allocated ones will probably have been deleted automatically + /// prior to calling this destructor. + /// + /// In addition, once a component has deleted its children, it will remove itself from its parent. + /// + /// @note Child deletion happens in reverse order. + virtual ~Component() + { + // Delete children + while (!m_children.empty()) + { + // When deleted, children will remove themselves from this list + auto* component{m_children.back()}; + delete component; + } + + // Delete from parent + if (m_parent) + { + m_parent->remove_child(this); + } + } /// @brief Obtain a reference to the underlying I/O context. /// @@ -41,6 +73,30 @@ namespace kouta::base return m_parent->context(); } + /// @brief Add a child component to the list. + /// + /// @details + /// This is used to keep track of objects to delete when the component has been allocated in the heap. + /// + /// @note Normally, this will only be called from the Constructor of the component. + /// + /// @param[in] component Pointer to the component to add. + void add_child(Component* component) + { + m_children.emplace_back(component); + } + + /// @brief Remove a child component from the list. + /// + /// @details + /// When a child is removed, the parent component will not attempt to delete it itself. Note that when a + /// component allocated in the stack is destroyed, it will remove itself from the list and prevent + /// double-free issues. + void remove_child(Component* component) + { + std::erase(m_children, component); + } + /// @brief Post a method call to the event loop for deferred execution. /// /// @details @@ -107,5 +163,6 @@ namespace kouta::base private: Component* m_parent; + std::vector m_children; }; } // namespace kouta::base diff --git a/include/kouta/base/root.hpp b/include/kouta/base/root.hpp index a328869..6221bd8 100644 --- a/include/kouta/base/root.hpp +++ b/include/kouta/base/root.hpp @@ -12,8 +12,24 @@ namespace kouta::base class Root : public Component { public: + /// @brief Default constructor. + /// + /// @details + /// This constructor assumes that the Root object will not have a parent (e.g. it is the main object of the + /// tree), meaning that it will not attempt to register itself with the parent, nor remove itself from its list + /// when being destroyed. Root() - : Component{nullptr} + : Root{nullptr} + { + } + + /// @brief Construct from a parent. + /// + /// @details + /// This constructor will register the Root object with the parent **only to manage the memory deallocation** in + /// case the object was allocated on the heap. Regardless of having a parent, the Root owns its event loop. + explicit Root(Component* parent) + : Component{parent} , m_context{} { } diff --git a/tests/base/dummy-component.cpp b/tests/base/dummy-component.cpp index a8378c5..53d56a1 100644 --- a/tests/base/dummy-component.cpp +++ b/tests/base/dummy-component.cpp @@ -2,6 +2,10 @@ namespace kouta::tests::base { + DummyComponent::DummyComponent(Component* parent) + : Component{parent} + {} + DummyComponent::DummyComponent( Component* parent, const Callback& callback_a, @@ -28,6 +32,12 @@ namespace kouta::tests::base { } + DummyComponent::DummyComponent(Component* parent, const Callback callback_on_delete) + : Component{parent} + , m_callback_on_delete{callback_on_delete} + { + } + void DummyComponent::call_a(std::uint16_t value) { m_callback_a(value); diff --git a/tests/base/dummy-component.hpp b/tests/base/dummy-component.hpp index fbc539f..a5cb66a 100644 --- a/tests/base/dummy-component.hpp +++ b/tests/base/dummy-component.hpp @@ -17,6 +17,8 @@ namespace kouta::tests::base public: DummyComponent() = delete; + explicit DummyComponent(Component* parent); + DummyComponent( Component* parent, const Callback& callback_a, @@ -30,6 +32,8 @@ namespace kouta::tests::base const Callback&>& callback_c, const Callback& callback_d); + DummyComponent(Component* parent, const Callback callback_on_delete); + // Not copyable DummyComponent(const DummyComponent&) = delete; DummyComponent& operator=(const DummyComponent&) = delete; @@ -38,7 +42,14 @@ namespace kouta::tests::base DummyComponent(DummyComponent&&) = delete; DummyComponent& operator=(DummyComponent&&) = delete; - ~DummyComponent() override = default; + ~DummyComponent() override + { + // Notify that the object was deleted + if (m_callback_on_delete) + { + m_callback_on_delete.value()(this); + } + } /// @brief Callback invokers. /// @{ @@ -53,5 +64,6 @@ namespace kouta::tests::base Callback m_callback_b; Callback&> m_callback_c; Callback m_callback_d; + std::optional> m_callback_on_delete; }; } // namespace kouta::tests::base diff --git a/tests/base/test-base.cpp b/tests/base/test-base.cpp index b64975a..ca6fa68 100644 --- a/tests/base/test-base.cpp +++ b/tests/base/test-base.cpp @@ -17,6 +17,7 @@ namespace kouta::tests::base MOCK_METHOD(void, handler_b, (std::int32_t value_a, const std::string& value_b), ()); MOCK_METHOD(void, handler_c, (const std::vector& value), ()); MOCK_METHOD(void, handler_d, (std::thread::id), ()); + MOCK_METHOD(void, handler_delete, (Component*), ()); }; /// @brief Test the behaviour of the empty callback. @@ -329,7 +330,7 @@ namespace kouta::tests::base RootMock root{}; Branch worker{ - &worker, + &root, callback::DirectCallback{&root, &RootMock::handler_a}, callback::DeferredCallback{&root, &RootMock::handler_b}, callback::DeferredCallback{&root, &RootMock::handler_c}, @@ -369,4 +370,81 @@ namespace kouta::tests::base root.run(); alarm(0); } + + /// @brief Test the behaviour of a component tree when allocated in the heap. + /// + /// @details + /// The test succeeds if all components are deallocated. + TEST(BaseTest, HeapAllocation) + { + RootMock root{}; + + auto* dummy_base = new DummyComponent{&root, callback::DeferredCallback{&root, &RootMock::handler_delete}}; + + // Layer 1 + auto* comp_a = new DummyComponent{dummy_base, callback::DeferredCallback{&root, &RootMock::handler_delete}}; + auto* comp_b = new DummyComponent{dummy_base, callback::DeferredCallback{&root, &RootMock::handler_delete}}; + auto* comp_c = new DummyComponent{dummy_base, callback::DeferredCallback{&root, &RootMock::handler_delete}}; + + // Layer 2 + auto* comp_a1 = new DummyComponent{comp_a, callback::DeferredCallback{&root, &RootMock::handler_delete}}; + auto* comp_a2 = new DummyComponent{comp_a, callback::DeferredCallback{&root, &RootMock::handler_delete}}; + auto* comp_c1 = new DummyComponent{comp_c, callback::DeferredCallback{&root, &RootMock::handler_delete}}; + + // Layer 3 + auto* comp_a1_1 = new DummyComponent{comp_a1, callback::DeferredCallback{&root, &RootMock::handler_delete}}; + auto* comp_a1_2 = new DummyComponent{comp_a1, callback::DeferredCallback{&root, &RootMock::handler_delete}}; + + // Deletion will be in reverse creation order, but we don't really care + EXPECT_CALL(root, handler_delete(dummy_base)) + .WillOnce( + [&root]() + { + root.post(&RootMock::stop); + }); + + EXPECT_CALL(root, handler_delete(comp_a)).Times(1); + EXPECT_CALL(root, handler_delete(comp_a2)).Times(1); + EXPECT_CALL(root, handler_delete(comp_a1)).Times(1); + EXPECT_CALL(root, handler_delete(comp_a1_2)).Times(1); + EXPECT_CALL(root, handler_delete(comp_a1_1)).Times(1); + + EXPECT_CALL(root, handler_delete(comp_b)).Times(1); + + EXPECT_CALL(root, handler_delete(comp_c)).Times(1); + EXPECT_CALL(root, handler_delete(comp_c1)).Times(1); + + delete dummy_base; + + root.run(); + } + + /// @brief Test the behaviour of a component tree when allocated in the stack. + /// + /// @details + /// The test succeeds if no exception is thrown due to free(). + TEST(BaseTest, StackAllocation) + { + RootMock root{}; + + DummyComponent dummy_base{&root}; + + // Layer 1 + DummyComponent comp_a{&dummy_base}; + DummyComponent comp_b{&dummy_base}; + DummyComponent comp_c{&dummy_base}; + + // Layer 2 + DummyComponent comp_a1{&comp_a}; + DummyComponent comp_a2{&comp_a}; + DummyComponent comp_c1{&comp_c}; + + // Layer 3 + DummyComponent comp_a1_1{&comp_a1}; + DummyComponent comp_a1_2{&comp_a1}; + + auto* comp_c1_1{&comp_c1}; + + // Everything is deleted in reverse order, so there shouldn't be any exceptions at this point + } } // namespace kouta::tests::base