From e44e669378f2c63b09eda68797039b73047febff Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 15 Apr 2025 18:01:27 +0200 Subject: [PATCH] doc: better document NetEventsInterface and the deletion of "CNode"s Document the requirements around the `NetEventsInterface`'s methods and the lifetime of `CNode` objects in `CConnman::m_nodes`. --- src/net.h | 73 +++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 57 insertions(+), 16 deletions(-) diff --git a/src/net.h b/src/net.h index 9fdec52115e..d2b83cd7afb 100644 --- a/src/net.h +++ b/src/net.h @@ -1002,7 +1002,11 @@ private: }; /** - * Interface for message handling + * Interface for message handling. + * For a given `CNode`, the methods must be called in the following order: + * 1. `InitializeNode()` + * 2. Any number of calls to `ProcessMessages()` or `SendMessages()` + * 3. `FinalizeNode()` */ class NetEventsInterface { @@ -1010,10 +1014,19 @@ public: /** Mutex for anything that is only accessed via the msg processing thread */ static Mutex g_msgproc_mutex; - /** Initialize a peer (setup state) */ + /** + * Initialize a peer (setup state). The caller is responsible for eventually + * calling `FinalizeNode()` on this node to avoid memory leaks. + * @param[in] node Peer to initialize. + * @param[in] our_services The services that we have advertised to the peer. + */ virtual void InitializeNode(const CNode& node, ServiceFlags our_services) = 0; - /** Handle removal of a peer (clear state) */ + /** + * Handle removal of a peer (clear state). + * When this is called all socket operations with the node must have completed. + * @param[in] node Peer whose state to clear. + */ virtual void FinalizeNode(const CNode& node) = 0; /** @@ -1023,23 +1036,26 @@ public: virtual bool HasAllDesirableServiceFlags(ServiceFlags services) const = 0; /** - * Process protocol messages received from a given node - * - * @param[in] pnode The node which we have received messages from. - * @param[in] interrupt Interrupt condition for processing threads - * @return True if there is more work to be done - */ + * Process protocol messages received from a given node. + * The caller must make sure that `pnode` is not destroyed and `FinalizeNode()` + * is not called on it during the execution of this function. + * + * @param[in] pnode The node which we have received messages from. + * @param[in] interrupt Interrupt condition for processing threads + * @return True if there is more work to be done + */ virtual bool ProcessMessages(CNode* pnode, std::atomic& interrupt) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0; /** - * Send queued protocol messages to a given node. - * - * @param[in] pnode The node which we are sending messages to. - * @return True if there is more work to be done - */ + * Send queued protocol messages to a given node. + * The caller must make sure that `pnode` is not destroyed and `FinalizeNode()` + * is not called on it during the execution of this function. + * + * @param[in] pnode The node which we are sending messages to. + * @return True if there is more work to be done + */ virtual bool SendMessages(CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0; - protected: /** * Protected destructor so that instances can only be deleted by derived classes. @@ -1442,9 +1458,34 @@ private: std::vector m_added_node_params GUARDED_BY(m_added_nodes_mutex); mutable Mutex m_added_nodes_mutex; + + /** + * List of peers we are connected with. + * Before destroying objects from this list it must be ensured that + * all socket operations have completed. `m_msgproc` must be informed + * that the node is about to be destroyed by calling `m_msgproc->FinalizeNode()`. + * `m_msgproc->ProcessMessages()` and `m_msgproc->SendMessages()` must not be + * running on that node during the destruction. + * + * CNode objects keep a reference count that is used to avoid a destruction + * while the object is used. All users of elements from `m_nodes` must either + * do so while holding `m_nodes_mutex` or by incrementing the reference before + * use and decrement it after use. To make sure that a new reference is not added + * to a no-referenced CNode just before it is destroyed we use the following + * mechanism (without a help from the standard library): + * - New references are only allowed to be added to objects in `m_nodes`. + * - When an object from `m_nodes` is to be destroyed it is moved from + * `m_nodes` to `m_nodes_disconnected`. Adding new references to objects + * in `m_nodes_disconnected` is not allowed. + * - Once an object in `m_nodes_disconnected` is not referenced anymore, + * then it is safe to destroy it. + * + * So CNode destruction is: wait for no-references, `FinalizeNode()`, delete CNode. + */ std::vector m_nodes GUARDED_BY(m_nodes_mutex); - std::list m_nodes_disconnected; mutable RecursiveMutex m_nodes_mutex; + + std::list m_nodes_disconnected; std::atomic nLastNodeId{0}; unsigned int nPrevNodeCount{0};