Implemented TODO to make asynchronous destruction of presentation entities (publishers, subscribers and rpc clients).

Now unreferenced shared objects (with ref_count==0) will be quickly appended to the double-linked list, which will be traversed asynchronously in a near future callback.

Author: serges147

include/libcyphal/presentation/client_impl.hpp

@@ -37,6 +37,9 @@ namespace detail
 class SharedClient : public cavl::Node<SharedClient>, public SharedObject
 {
 public:
+    using Node::remove;
+    using Node::isLinked;
+
     class TimeoutNode : public Node<TimeoutNode>
     {
     public:
@@ -229,13 +232,20 @@ class SharedClient : public cavl::Node<SharedClient>, public SharedObject
             CETL_DEBUG_ASSERT(cb_nodes_by_transfer_id_.empty(), "");
             CETL_DEBUG_ASSERT(timeout_nodes_by_deadline_.empty(), "");
 
-            delegate_.releaseSharedClient(this);
+            delegate_.markSharedObjAsUnreferenced(*this);
             return true;
         }
 
         return false;
     }
 
+    // MARK: SharedObject
+
+    void destroy() noexcept override
+    {
+        delegate_.forgetSharedClient(*this);
+    }
+
 protected:
     virtual void insertNewCallbackNode(CallbackNode& callback_node)
     {
@@ -393,7 +403,7 @@ class SharedClient : public cavl::Node<SharedClient>, public SharedObject
 /// @brief Defines a shared client implementation that uses a generic transfer ID generator.
 ///
 template <typename TransferIdGeneratorMixin>
-class ClientImpl final : public SharedClient, public TransferIdGeneratorMixin
+class ClientImpl final : public SharedClient, private TransferIdGeneratorMixin
 {
 public:
     ClientImpl(IPresentationDelegate&                   delegate,
@@ -410,10 +420,13 @@ class ClientImpl final : public SharedClient, public TransferIdGeneratorMixin
 
     void destroy() noexcept override
     {
+        Base::destroy();
         destroyWithPmr(this, memory());
     }
 
 private:
+    using Base = SharedClient;
+
     // MARK: SharedClient
 
     CETL_NODISCARD cetl::optional<transport::TransferId> nextTransferId() noexcept override
@@ -440,25 +453,28 @@ class ClientImpl final : public SharedClient, public TransferIdGeneratorMixin
 template <>
 class ClientImpl<transport::detail::TrivialTransferIdGenerator> final
     : public SharedClient,
-      public transport::detail::TrivialTransferIdGenerator
+      private transport::detail::TrivialTransferIdGenerator
 {
 public:
     ClientImpl(IPresentationDelegate&                   delegate,
                IExecutor&                               executor,
                UniquePtr<transport::IRequestTxSession>  svc_request_tx_session,
                UniquePtr<transport::IResponseRxSession> svc_response_rx_session)
-        : SharedClient{delegate, executor, std::move(svc_request_tx_session), std::move(svc_response_rx_session)}
+        : Base{delegate, executor, std::move(svc_request_tx_session), std::move(svc_response_rx_session)}
     {
     }
 
     // MARK: SharedObject
 
     void destroy() noexcept override
     {
+        Base::destroy();
         destroyWithPmr(this, memory());
     }
 
 private:
+    using Base = SharedClient;
+
     // MARK: SharedClient
 
     CETL_NODISCARD cetl::optional<transport::TransferId> nextTransferId() noexcept override

include/libcyphal/presentation/presentation.hpp

@@ -97,7 +97,10 @@ struct IsFixedPortIdMessageTrait<T, true>
 /// Instance of this class is supposed to be created once per transport instance (or even per application).
 /// Main purpose of the presentation object is to create publishers, subscribers, and RPC clients and servers.
 ///
-class Presentation final : private detail::IPresentationDelegate
+/// No Sonar cpp:S4963 'The "Rule-of-Zero" should be followed'
+/// b/c we do directly handle resources here.
+///
+class Presentation final : private detail::IPresentationDelegate  // NOSONAR cpp:S4963
 {
 public:
     /// @brief Defines failure type of various `make...` methods of the presentation layer.
@@ -112,7 +115,30 @@ class Presentation final : private detail::IPresentationDelegate
         : memory_{memory}
         , executor_{executor}
         , transport_{transport}
+        , unreferenced_nodes_{&unreferenced_nodes_, &unreferenced_nodes_}
+    {
+        unref_nodes_deleter_callback_ = executor_.registerCallback([this](const auto&) {
+            //
+            destroyUnreferencedNodes();
+        });
+        CETL_DEBUG_ASSERT(unref_nodes_deleter_callback_, "Should not fail b/c we pass proper lambda.");
+    }
+
+    Presentation(const Presentation&)                = delete;
+    Presentation(Presentation&&) noexcept            = delete;
+    Presentation& operator=(const Presentation&)     = delete;
+    Presentation& operator=(Presentation&&) noexcept = delete;
+
+    ~Presentation()
     {
+        destroyUnreferencedNodes();
+
+        CETL_DEBUG_ASSERT(shared_client_nodes_.empty(),  //
+                          "RPC clients must be destroyed before presentation.");
+        CETL_DEBUG_ASSERT(publisher_impl_nodes_.empty(),  //
+                          "Message publishers must be destroyed before presentation.");
+        CETL_DEBUG_ASSERT(subscriber_impl_nodes_.empty(),  //
+                          "Message subscribers must be destroyed before presentation.");
     }
 
     /// @brief Gets reference to the executor instance of this presentation object.
@@ -161,6 +187,11 @@ class Presentation final : private detail::IPresentationDelegate
         auto* const publisher_impl = std::get<0>(publisher_existing);
         CETL_DEBUG_ASSERT(publisher_impl != nullptr, "");
 
+        // This publisher impl node might be in the list of previously unreferenced nodes -
+        // the ones that are going to be deleted asynchronously (by the `destroyUnreferencedNodes`).
+        // If it's the case, we need to remove it from the list b/c it's going to be referenced.
+        publisher_impl->unlinkIfReferenced();
+
         return Publisher<Message>{publisher_impl};
     }
 
@@ -456,6 +487,8 @@ class Presentation final : private detail::IPresentationDelegate
     }
 
 private:
+    using Schedule = IExecutor::Callback::Schedule;
+
     IPresentationDelegate& asDelegate() noexcept
     {
         return static_cast<IPresentationDelegate&>(*this);
@@ -517,6 +550,12 @@ class Presentation final : private detail::IPresentationDelegate
 
         auto* const subscriber_impl = std::get<0>(subscriber_existing);
         CETL_DEBUG_ASSERT(subscriber_impl != nullptr, "");
+
+        // This subscriber impl node might be in the list of previously unreferenced nodes -
+        // the ones that are going to be deleted asynchronously (by the `destroyUnreferencedNodes`).
+        // If it's the case, we need to remove it from the list b/c it's going to be referenced.
+        subscriber_impl->unlinkIfReferenced();
+
         return subscriber_impl;
     }
 
@@ -571,6 +610,12 @@ class Presentation final : private detail::IPresentationDelegate
 
         auto* const shared_client = std::get<0>(shared_client_existing);
         CETL_DEBUG_ASSERT(shared_client != nullptr, "");
+
+        // This client impl node might be in the list of previously unreferenced nodes -
+        // the ones that are going to be deleted asynchronously (by the `destroyUnreferencedNodes`).
+        // If it's the case, we need to remove it from the list b/c it's going to be referenced.
+        shared_client->unlinkIfReferenced();
+
         return shared_client;
     }
 
@@ -628,30 +673,57 @@ class Presentation final : private detail::IPresentationDelegate
     }
 
     template <typename SharedNode>
-    void releaseSharedNode(SharedNode* const shared_node, cavl::Tree<SharedNode>& tree) const noexcept
+    static void forgetSharedNode(SharedNode& shared_node) noexcept
     {
-        CETL_DEBUG_ASSERT(shared_node != nullptr, "");
+        CETL_DEBUG_ASSERT(shared_node.isLinked(), "");
+        CETL_DEBUG_ASSERT(!shared_node.isReferenced(), "");
 
-        // TODO: make it async (deferred to "on idle" callback).
-        tree.remove(shared_node);
-        shared_node->destroy();
+        // Remove the node from its tree (if it still there),
+        // as well as from the list of unreferenced nodes (b/c we gonna finally destroy it).
+        shared_node.remove();              // from the tree
+        shared_node.unlinkIfReferenced();  // from the list
+    }
+
+    void destroyUnreferencedNodes() const noexcept
+    {
+        // In the loop, destruction of a shared object also removes it from the list of unreferenced nodes.
+        // So, it implicitly updates the `unreferenced_nodes_` list.
+        while (unreferenced_nodes_.next_node != &unreferenced_nodes_)
+        {
+            auto* const shared_obj = static_cast<detail::SharedObject*>(unreferenced_nodes_.next_node);
+            shared_obj->destroy();
+        }
     }
 
     // MARK: IPresentationDelegate
 
-    void releaseSharedClient(detail::SharedClient* const shared_client) noexcept override
+    void markSharedObjAsUnreferenced(detail::SharedObject& shared_obj) noexcept override
+    {
+        // We are not going to destroy the shared object immediately, but schedule it for deletion.
+        // This is b/c destruction of shared objects may be time-consuming (f.e. closing under the hood sockets).
+        // Double-linked list is used to avoid the need to traverse the tree of shared objects.
+        //
+        CETL_DEBUG_ASSERT(!shared_obj.isReferenced(), "");
+        shared_obj.linkAsUnreferenced(unreferenced_nodes_);
+        //
+        const auto result = unref_nodes_deleter_callback_.schedule(Schedule::Once{executor_.now()});
+        CETL_DEBUG_ASSERT(result, "Should not fail b/c we never reset `unref_nodes_deleter_callback_`.");
+        (void) result;
+    }
+
+    void forgetSharedClient(detail::SharedClient& shared_client) noexcept override
     {
-        releaseSharedNode(shared_client, shared_client_nodes_);
+        forgetSharedNode(shared_client);
     }
 
-    void releasePublisherImpl(detail::PublisherImpl* const publisher_impl) noexcept override
+    void forgetPublisherImpl(detail::PublisherImpl& publisher_impl) noexcept override
     {
-        releaseSharedNode(publisher_impl, publisher_impl_nodes_);
+        forgetSharedNode(publisher_impl);
     }
 
-    void releaseSubscriberImpl(detail::SubscriberImpl* const subscriber_impl) noexcept override
+    void forgetSubscriberImpl(detail::SubscriberImpl& subscriber_impl) noexcept override
     {
-        releaseSharedNode(subscriber_impl, subscriber_impl_nodes_);
+        forgetSharedNode(subscriber_impl);
     }
 
     // MARK: Data members:
@@ -662,6 +734,8 @@ class Presentation final : private detail::IPresentationDelegate
     cavl::Tree<detail::SharedClient>   shared_client_nodes_;
     cavl::Tree<detail::PublisherImpl>  publisher_impl_nodes_;
     cavl::Tree<detail::SubscriberImpl> subscriber_impl_nodes_;
+    detail::UnRefNode                  unreferenced_nodes_;
+    IExecutor::Callback::Any           unref_nodes_deleter_callback_;
 
 };  // Presentation
 

include/libcyphal/presentation/presentation_delegate.hpp

@@ -6,6 +6,8 @@
 #ifndef LIBCYPHAL_PRESENTATION_DELEGATE_HPP_INCLUDED
 #define LIBCYPHAL_PRESENTATION_DELEGATE_HPP_INCLUDED
 
+#include "shared_object.hpp"
+
 #include <cetl/pf17/cetlpf.hpp>
 
 #include <type_traits>
@@ -84,9 +86,10 @@ class IPresentationDelegate
 
     virtual cetl::pmr::memory_resource& memory() const noexcept = 0;
 
-    virtual void releaseSharedClient(SharedClient* shared_client) noexcept       = 0;
-    virtual void releasePublisherImpl(PublisherImpl* publisher_impl) noexcept    = 0;
-    virtual void releaseSubscriberImpl(SubscriberImpl* subscriber_impl) noexcept = 0;
+    virtual void markSharedObjAsUnreferenced(SharedObject& shared_obj) noexcept = 0;
+    virtual void forgetSharedClient(SharedClient& shared_client) noexcept       = 0;
+    virtual void forgetPublisherImpl(PublisherImpl& publisher_impl) noexcept    = 0;
+    virtual void forgetSubscriberImpl(SubscriberImpl& subscriber_impl) noexcept = 0;
 
 protected:
     IPresentationDelegate()  = default;

include/libcyphal/presentation/publisher_impl.hpp

@@ -36,6 +36,9 @@ namespace detail
 class PublisherImpl final : public cavl::Node<PublisherImpl>, public SharedObject
 {
 public:
+    using Node::remove;
+    using Node::isLinked;
+
     PublisherImpl(IPresentationDelegate& delegate, UniquePtr<transport::IMessageTxSession> msg_tx_session)
         : delegate_{delegate}
         , msg_tx_session_{std::move(msg_tx_session)}
@@ -70,19 +73,22 @@ class PublisherImpl final : public cavl::Node<PublisherImpl>, public SharedObjec
     {
         if (SharedObject::release())
         {
-            delegate_.releasePublisherImpl(this);
+            delegate_.markSharedObjAsUnreferenced(*this);
             return true;
         }
 
         return false;
     }
 
+private:
+    // MARK: SharedObject
+
     void destroy() noexcept override
     {
+        delegate_.forgetPublisherImpl(*this);
         destroyWithPmr(this, delegate_.memory());
     }
 
-private:
     // MARK: Data members:
 
     IPresentationDelegate&                        delegate_;

include/libcyphal/presentation/shared_object.hpp

@@ -25,9 +25,49 @@ namespace presentation
 namespace detail
 {
 
+/// @brief Defines double-linked list of unreferenced nodes.
+///
+struct UnRefNode
+{
+    void linkAsUnreferenced(UnRefNode& origin)
+    {
+        CETL_DEBUG_ASSERT((prev_node != nullptr) == (next_node != nullptr), "Should be both or none.");
+
+        // Already linked?
+        if ((nullptr == prev_node) && (nullptr == next_node))
+        {
+            // Link to the end of the list, so that the object is destroyed in the order of unreferencing.
+            //
+            next_node                   = &origin;
+            prev_node                   = origin.prev_node;
+            origin.prev_node->next_node = this;
+            origin.prev_node            = this;
+        }
+    }
+
+    void unlinkIfReferenced()
+    {
+        CETL_DEBUG_ASSERT((prev_node != nullptr) == (next_node != nullptr), "Should be both or none.");
+
+        // Already unlinked?
+        if ((nullptr != prev_node) && (nullptr != next_node))
+        {
+            prev_node->next_node = next_node;
+            next_node->prev_node = prev_node;
+
+            next_node = nullptr;
+            prev_node = nullptr;
+        }
+    }
+
+    UnRefNode* prev_node{nullptr};
+    UnRefNode* next_node{nullptr};
+
+};  // UnRefNode
+
 /// @brief Defines the base class for all classes that need to be shared (using reference count).
 ///
-class SharedObject
+class SharedObject : public UnRefNode
 {
 public:
     SharedObject()          = default;
@@ -38,6 +78,13 @@ class SharedObject
     SharedObject& operator=(const SharedObject&)     = delete;
     SharedObject& operator=(SharedObject&&) noexcept = delete;
 
+    /// @brief Gets boolean indicating whether the object is referenced at least once.
+    ///
+    bool isReferenced() const noexcept
+    {
+        return ref_count_ > 0;
+    }
+
     /// @brief Increments the reference count.
     ///
     void retain() noexcept