net: adopt a shared_ptr<CNode> connection-lifetime model#3076
Open
PrestackI wants to merge 2 commits into
Open
net: adopt a shared_ptr<CNode> connection-lifetime model#3076PrestackI wants to merge 2 commits into
PrestackI wants to merge 2 commits into
Conversation
CNode lifetime was managed by a manual AddRef/Release refcount (CNode::nRefCount) with the ThreadSocketHandler2 reaper deleting a node only once its refcount hit zero and its per-node locks cleared. Now that CConnman owns the node list, retire that by-convention refcount in favour of ownership via std::shared_ptr. - m_nodes becomes std::vector<std::shared_ptr<CNode>>; it holds the sole persistent owning reference. - ConnectNode returns std::shared_ptr<CNode> so OpenNetworkConnection and the addnode "onetry" RPC keep the node alive while they finish wiring it up, preserving the protection the old pre-publish AddRef() gave. - The socket-handler and message-handler service passes snapshot m_nodes by copying the shared_ptr vector; the copies keep nodes alive for the pass and are released when the snapshot leaves scope, replacing the AddRef/Release loops. - The reaper moves a disconnected node's shared_ptr into vNodesDisconnected and deletes it once use_count() == 1 (only the disconnected pool references it) and the per-node locks are free, replacing the explicit `delete pnode`. The former empty-buffer auto-disconnect path is dropped: every connected node held a permanent creation reference, so the GetRefCount() <= 0 gate it required never held, making it dead code. Disconnection has always been driven by fDisconnect alone, which is preserved. Closes gridcoin-community#3066.
When a ctest case fails in the distro/native build path, ctest buffers the test's stdout into build/Testing/Temporary/LastTest.log and prints nothing to the console, so functional-test flakes (e.g. rpc_multisig.py) show only "1/3 Test #1: functional_tests ...***Failed" with no failing script name, traceback, or combined node logs. test_runner.py already runs with --ci --quiet --combinedlogslen=4000, so the detail exists — it's just swallowed. Add --output-on-failure to all four ctest invocations (native, depends, win64, macos), mirroring what the dedicated functional job already does (cmake_functional.yml). The flag is additive: no change on success, full captured output on failure.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3066.
Background
The #2558 stack moved the connection list into
CConnmanbut deliberately kept theinherited manual lifetime model:
CNodelifetime was managed byAddRef()/Release()on an atomic
nRefCount, and theThreadSocketHandler2reaper moved disconnectednodes into a local
std::list<CNode*>anddelete pnodeonce the refcount and theper-node locks cleared. Now that
CConnmanowns the storage, this adopts ownership viastd::shared_ptr<CNode>and retires the by-convention refcount foot-gun.Change
m_nodesbecomesstd::vector<std::shared_ptr<CNode>>— it holds the sole persistentowning reference.
ConnectNodereturnsstd::shared_ptr<CNode>soOpenNetworkConnectionand the addnode"onetry" RPC keep the node alive while they finish wiring it up, preserving the
protection the old pre-publish
AddRef()provided.m_nodesby copying theshared_ptr vector; the copies keep nodes alive for the pass and are released when the
snapshot leaves scope, replacing the
AddRef/Releaseloops.it once
use_count() == 1(only the pool references it) and the per-node locks are free,replacing the explicit
delete pnode.Behavior-preserving note
The former empty-buffer auto-disconnect path is dropped: every connected node held a
permanent creation reference, so the
GetRefCount() <= 0gate that path required neverheld, making it dead code. Disconnection has always been driven by
fDisconnectalone,which is preserved.
Risk / testing
MEDIUM — touches the hot connect/accept/reap path. Validated by a clean full local build
and all four fork cmake_* checks + Functional Tests green. The borrowing API
(
FindNode/ForEachNode/etc.) keeps rawCNode*parameters; no caller stores aCNode*past lock release and there are no
CNode*-keyed maps, so the change stays contained tosrc/net.h+src/net.cpp(and oneConnectNodecaller inrpc/net.cpp).