net: replace manual reference counting of CNode with shared_ptr#32015
net: replace manual reference counting of CNode with shared_ptr#32015vasild wants to merge 7 commits into
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32015. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
cc @willcl-ark, @dergoegge, @theuni |
|
cc @hodlinator, you might be interested in this as well |
|
https://github.com/bitcoin/bitcoin/actions/runs/13718139296/job/38367326902?pr=32015#step:7:1603: Run p2p_headers_presync with args ['/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/src/test/fuzz/fuzz', PosixPath('/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_corpora/p2p_headers_presync')]net_processing.cpp:295 SetTxRelay: Assertion `!m_tx_relay' failed.
Error processing input "/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_corpora/p2p_headers_presync/c12f87158cda99b4fb2e1fe7312af392a1f4634a"
net_processing.cpp:295 SetTxRelay: Assertion `!m_tx_relay' failed.
Error processing input "/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_corpora/p2p_headers_presync/c12f87158cda99b4fb2e1fe7312af392a1f4634a" |
| // after m_nodes_mutex has been released. Destructing a node involves | ||
| // calling m_msgproc->FinalizeNode() which acquires cs_main. Lock order | ||
| // should be cs_main, m_nodes_mutex. | ||
| disconnected_nodes.push_back(node); |
There was a problem hiding this comment.
Now that ~CNode is responsible for calling FinalizeNode, the guarantees around which thread does the actual cleanup are lost. It's basically just whichever thread ends up holding the last instance of a CNode. This seems bug prone.
E.g. there are no guarantees (afaict) that disconnected_nodes will actually be the last owner.
There was a problem hiding this comment.
The idea is that it does not matter which thread will destroy the CNode objects.
there are no guarantees (afaict) that
disconnected_nodeswill actually be the last owner.
That is right, but there is no need for such a guarantee. disconnected_nodes is to guarantee that the objects will not be destroyed while holding m_nodes_mutex.
There was a problem hiding this comment.
In thread #32015 (comment):
Maybe one could have an additional vector<shared_ptr<CNode>> member in CConnman beyond m_nodes that always keeps the shared_ptr::use_count() above 0. Then use removal from that vector (when use_count() == 1) for controlled deletion with enforced lock order, more like how the code was before?
There was a problem hiding this comment.
I do not think that is necessary. I mean - if we would assume that the code is going to be modified in a careless way to break the logic, in other words to destroy CNode objects while holding m_nodes_mutex, then we might as well assume that the code could be modified in a way that breaks the manual reference counting. IMO the manual reference counting is more fragile and error prone.
Further, if the code is changed to destroy CNode objects while holding m_nodes_mutex, then 83 functional tests fail. I think it is safe to assume that such a thing will not go unnoticed.
There was a problem hiding this comment.
In thread #32015 (comment):
Started proving out my suggestion and landed on something closer to master.
- Resurrect
m_nodes_disconnected(now holdingshared_ptr) along with clearly defined lifetimes. - Resurrect
DeleteNode(), but make it take an r-valueshared_ptrand assert thatuse_count() == 1. - Avoid adding
~CNode()andCNode::m_destruct_cbalong with sending it in everywhere in the constructor. This means we don't need to touch 5 of the test files.
master...hodlinator:bitcoin:pr/32015_suggestion
Passes unit tests and functional tests.
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
|
hodlinator
left a comment
There was a problem hiding this comment.
Concept ACK 36d932cebbb235a79e90575960646e86a23d39a8
| // after m_nodes_mutex has been released. Destructing a node involves | ||
| // calling m_msgproc->FinalizeNode() which acquires cs_main. Lock order | ||
| // should be cs_main, m_nodes_mutex. | ||
| disconnected_nodes.push_back(node); |
There was a problem hiding this comment.
In thread #32015 (comment):
Maybe one could have an additional vector<shared_ptr<CNode>> member in CConnman beyond m_nodes that always keeps the shared_ptr::use_count() above 0. Then use removal from that vector (when use_count() == 1) for controlled deletion with enforced lock order, more like how the code was before?
36d932c to
af622d0
Compare
|
|
|
Instead of passing around shared pointers to mutable data (aka global variables in disguise), would it be possible to increase the level of abstraction and pass around values with regular semantics? Every C++ programmer should rewatch https://youtu.be/W2tWOdzgXHA from time to time. |
I remembered that as "the seminal
Guess one could have an |
|
After reviewing the code line by line, I realized that the only places where |
It is also called in the RAII helper
It's probably just there to have it "covered". |
Got it. |
0cd86a5 to
8218daa
Compare
|
|
8218daa to
d026d54
Compare
|
This is now a little bit simplified after #32326 was merged - no need to touch |
This is a non-functional change - the manual reference counting, disconnection, finalization and deletion are unchanged. `DeleteNode()` used to call `FinalizeNode()` + `delete` the raw pointer. Expand `DeleteNode()` at the call sites - call `FinalizeNode()` at the call sites and instead of raw `delete` either get the container vector go out of scope or explicitly call `clear()` on it.
…aries The other copies have a purpose for locking, these were just an awkward convenience for deleting while iterating. Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Subsequent commits will allow this to be shared with the message handling thread. Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Rather than relying on refcounting to abstract away our teardown procedure, make the rules explicit: - FinalizeNode() is only called when ProcessMessages() and SendMessages() is not running. This is enforced by finalizing only at the top of the message handler loop. - FinalizeNode() may only be called on nodes which have been disconnected, by virtue of finding them in m_nodes_disconnected. This enforces the order: disconnect -> finalize -> delete. This order is the current behavior but is not strictly necessary, and the restriction will be removed in a later commit. - Nodes are only deleted after they've been disconnected and finalized. This is enforced by deleting them from m_nodes_disconnected and only after calling FinalizeNode(). - Callers of ForNode() and ForEachNode() are currently protected from finalization or deletion due to their locking of m_nodes_mutex. Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Snapshots of `m_nodes` are not needed anymore because finalization and deletion of nodes only happens in `ThreadMessageHandler()` and only a after nodes have been disconnected and moved from `m_nodes` to `m_nodes_disconnected`. The manual reference counting becomes unused, remove it too. Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>
The shared_ptrs now ensure that the CNodes will not be deleted for the duration of the callbacks. Disconnection/Finalization may happen while the shared_ptr is being held, though it won't be deleted for the duration of the callback. The majority of uses are in ProcessMessages and SendMessages, during which disconnection may occur but FinalizeNode will not be called. Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
This keeps us from waiting for the condvar timeout (currently 100ms) before finalizing a node that has been disconnected on the socket handler thread. Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
d026d54 to
abc29c4
Compare
|
|
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
This has been in need of a rebase for a while. Can you push an update? |
|
⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
@vasild are you still working on this? |
|
Whenever I need to use a pointer, I increment a reference count to make sure nobody deletes it while I use it. When I am done with it, I decrement the reference counter. Whenever the counter drops to zero, the object is deleted. This is what happens in I think such a simplification is worth the effort to rebase, resolve conflicts and give some love to this PR. However, it is also an energy drain. Last status seems to be that I incorporated the feedback and then it was suggested it might be better to do another bigger change. That was one year ago. @theuni, would you be ok with this in 2026? If yes, then I will bring it up to date. If no, then I will close it because it will go nowhere anyway. |
Before this change the code used to count references to
CNodeobjects manually viaCNode::nRefCount. UnneededCNodes were scheduled for deletion by putting them inCConnman::m_nodes_disconnectedand were deleted after their reference count reached zero. Deleting consists of callingPeerManager::FinalizeNode()and destroying theCNodeobject.Replace this scheme with
std::shared_ptr. This simplifies the code and removes:CNode::nRefCountCNode::GetRefCount()CNode::AddRef()CNode::Release()CConnman::m_nodes_disconnectedCConnman::NodesSnapshotNow creating a snapshot of
CConnman::m_nodesis done by simply copying it (under the mutex).Call
PeerManager::FinalizeNode()from the destructor ofCNode, which is called when the reference count reaches 0.This removes brittle code and replaces it by a code from the standard library which is more robust. All the below are possible with the manual reference counting and not possible with
shared_ptr:There is zero learning curve for the average C++ programmer who knows how
shared_ptrworks. Not so much with the manual reference counting because one has to learn aboutAddRef(),Release(), check and maintain where are they called and their relationship withm_nodes_disconnected.This has some history in #28222 and #10738. See below #32015 (comment) and #32015 (comment).
Possible followup: change
ProcessMessages()andSendMessages()to takeCNode&: #32015 (comment)