net: remove unnecessary check of CNode::cs_vSend#21750
Merged
maflcko merged 1 commit intobitcoin:masterfrom May 3, 2021
Merged
net: remove unnecessary check of CNode::cs_vSend#21750maflcko merged 1 commit intobitcoin:masterfrom
maflcko merged 1 commit intobitcoin:masterfrom
Conversation
It is not possible to have a node in `CConnman::vNodesDisconnected` and its reference count to be incremented - all `CNode::AddRef()` are done either before the node is added to `CConnman::vNodes` or while holding `CConnman::cs_vNodes` and the object being in `CConnman::vNodes`. So, the object being in `CConnman::vNodesDisconnected` and its reference count being zero means that it is not and will not start to be used by other threads. So, the lock of `CNode::cs_vSend` in `CConnman::DisconnectNodes()` will always succeed and is not necessary. Indeed all locks of `CNode::cs_vSend` are done either when the reference count is >0 or under the protection of `CConnman::cs_vNodes` and the node being in `CConnman::vNodes`.
Member
|
Concept ACK. Wanted to do the same |
Contributor
Author
Member
|
Checked that cs_vSend is only locked when the node is in vNodes, so any node in the disconnect pool can't fail to take the lock. review ACK 9096b13 🏧 Show signature and timestampSignature: Timestamp of file with hash |
Member
|
tested with diff --git a/src/net.cpp b/src/net.cpp
index 5aa267f0d7..5bcc508bff 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1233,7 +1233,7 @@ void CConnman::DisconnectNodes()
fDelete = true;
}
}
- if (fDelete) {
+ Assert(fDelete); {
vNodesDisconnected.remove(pnode);
DeleteNode(pnode);
} |
Contributor
|
utACK 9096b13 Logic in commit message is sound. No other thread can be holding this lock if refcount is zero. Copying the list and then iterating through the copy seems unnecessary. Perhaps for a follow up: Detailsdiff --git a/src/net.cpp b/src/net.cpp
index f7d102a4df..8d53afa7f8 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1219,16 +1219,15 @@ void CConnman::DisconnectNodes()
}
}
}
- {
- // Delete disconnected nodes
- std::list<CNode*> vNodesDisconnectedCopy = vNodesDisconnected;
- for (CNode* pnode : vNodesDisconnectedCopy)
- {
- // Destroy the object only after other threads have stopped using it.
- if (pnode->GetRefCount() <= 0) {
- vNodesDisconnected.remove(pnode);
- DeleteNode(pnode);
- }
+
+ // Delete disconnected nodes
+ for (auto it = vNodesDisconnected.begin(); it != vNodesDisconnected.end(); ) {
+ // Destroy the object only after other threads have stopped using it.
+ if ((*it)->GetRefCount() <= 0) {
+ DeleteNode(*it);
+ it = vNodesDisconnected.erase(it);
+ } else {
+ ++it;
}
}
} |
Contributor
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 3, 2021
9096b13 net: remove unnecessary check of CNode::cs_vSend (Vasil Dimov) Pull request description: It is not possible to have a node in `CConnman::vNodesDisconnected` and its reference count to be incremented - all `CNode::AddRef()` are done either before the node is added to `CConnman::vNodes` or while holding `CConnman::cs_vNodes` and the object being in `CConnman::vNodes`. So, the object being in `CConnman::vNodesDisconnected` and its reference count being zero means that it is not and will not start to be used by other threads. So, the lock of `CNode::cs_vSend` in `CConnman::DisconnectNodes()` will always succeed and is not necessary. Indeed all locks of `CNode::cs_vSend` are done either when the reference count is >0 or under the protection of `CConnman::cs_vNodes` and the node being in `CConnman::vNodes`. ACKs for top commit: MarcoFalke: review ACK 9096b13 🏧 jnewbery: utACK 9096b13 Tree-SHA512: 910899cdcdc8934642eb0c40fcece8c3b01b7e20a0b023966b9d6972db6a885cb3a9a04e9562bae14d5833967e45e2ecb3687b94d495060c3da4b1f2afb0ac8f
gwillen
pushed a commit
to ElementsProject/elements
that referenced
this pull request
Jun 1, 2022
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
It is not possible to have a node in
CConnman::vNodesDisconnectedandits reference count to be incremented - all
CNode::AddRef()are doneeither before the node is added to
CConnman::vNodesor while holdingCConnman::cs_vNodesand the object being inCConnman::vNodes.So, the object being in
CConnman::vNodesDisconnectedand its referencecount being zero means that it is not and will not start to be used by
other threads.
So, the lock of
CNode::cs_vSendinCConnman::DisconnectNodes()willalways succeed and is not necessary.
Indeed all locks of
CNode::cs_vSendare done either when the referencecount is >0 or under the protection of
CConnman::cs_vNodesand thenode being in
CConnman::vNodes.