net: remove unnecessary check of CNode::cs

vasild commented at 9:30 am on April 22, 2021: member

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.

net: remove unnecessary check of CNode::cs_vSend

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`.

9096b13a47

fanquake added the label P2P on Apr 22, 2021

MarcoFalke added the label Refactoring on Apr 22, 2021

MarcoFalke commented at 9:34 am on April 22, 2021: member

Concept ACK. Wanted to do the same

vasild commented at 9:34 am on April 22, 2021: member

#19347 removed a similar check of CNode::cs_inventory. Maybe removing also the cs_vSend check was out of scope there or was overlooked? Or I am overlooking something with this PR?

cc @jnewbery

MarcoFalke commented at 9:58 am on April 22, 2021: 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 9096b13a4764873511b65f32a005ce4738b0d81c 🏧

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 9096b13a4764873511b65f32a005ce4738b0d81c 🏧
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUh+VAv/UH8AJG+ZvfXyiflUhYdflfmarAaklCfbP8yTP6Md4F5kUVXHLqJFQCie
 863pC27Bpz33kmXJAthemyjmwYg6KocJUT/Zptd4k9AZKELqop7YNDgjHv2s0fQwU
 9XDWAgDaAjZYheXTwGniikRCQfFxIGiGoGVjosty6eRL+37CFz2m5EqUDfLIR/YB5
10lmu/S6nsit+c+iE+mCSiFh7GHsivtXqUKPKr/RXsQCJQqFUFM4/JjbOscrnJd9m0
115kickbmQlQ9+qzvFDnitsyv5GrS1cKjalV1zUyx12IowAViTLFlYdI7nL+nMXwN5
125FtVT+ozKEt+XsPQ7NGE2pVUj03itg7+VAhczw/QUHLR20FQu/w54/bipzcq0ell
13Hn5ixEgoHz+lNrJy6BMJIg+QIG3Ar+ThteSHTsa+GSHnRi+4zZgqMgtXksXJOwI8
14+e/Io2Qrl4pl1d0V+MLohMQ8gDBLla+ZYksIv/Rl64HzVJcJ9LtyWP8DbPGotPNH
15aKW8y7uz
16=UtGP
17-----END PGP SIGNATURE-----

Timestamp of file with hash 24082aa150fe7737c3faac7c37e765910fecf5202285ee81b09b4ab7c5c6f10b -

MarcoFalke commented at 10:05 am on April 22, 2021: member

tested with

 0diff --git a/src/net.cpp b/src/net.cpp
 1index 5aa267f0d7..5bcc508bff 100644
 2--- a/src/net.cpp
 3+++ b/src/net.cpp
 4@@ -1233,7 +1233,7 @@ void CConnman::DisconnectNodes()
 5                         fDelete = true;
 6                     }
 7                 }
 8-                if (fDelete) {
 9+                Assert(fDelete); {
10                     vNodesDisconnected.remove(pnode);
11                     DeleteNode(pnode);
12                 }

jnewbery commented at 11:09 am on April 22, 2021: member

utACK 9096b13a4764873511b65f32a005ce4738b0d81c

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:

 0diff --git a/src/net.cpp b/src/net.cpp
 1index f7d102a4df..8d53afa7f8 100644
 2--- a/src/net.cpp
 3+++ b/src/net.cpp
 4@@ -1219,16 +1219,15 @@ void CConnman::DisconnectNodes()
 5             }
 6         }
 7     }
 8-    {
 9-        // Delete disconnected nodes
10-        std::list<CNode*> vNodesDisconnectedCopy = vNodesDisconnected;
11-        for (CNode* pnode : vNodesDisconnectedCopy)
12-        {
13-            // Destroy the object only after other threads have stopped using it.
14-            if (pnode->GetRefCount() <= 0) {
15-                vNodesDisconnected.remove(pnode);
16-                DeleteNode(pnode);
17-            }
18+
19+    // Delete disconnected nodes
20+    for (auto it = vNodesDisconnected.begin(); it != vNodesDisconnected.end(); ) {
21+        // Destroy the object only after other threads have stopped using it.
22+        if ((*it)->GetRefCount() <= 0) {
23+            DeleteNode(*it);
24+            it = vNodesDisconnected.erase(it);
25+        } else {
26+            ++it;
27         }
28     }
29 }

jnewbery commented at 11:11 am on April 22, 2021: member

@vasild if you’re interested in cleaning up locking for these objects, then you may wish to review #21563.

MarcoFalke merged this on May 3, 2021

MarcoFalke closed this on May 3, 2021

sidhujag referenced this in commit 8998833b2c on May 3, 2021

gwillen referenced this in commit d95da2d3b1 on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

net: remove unnecessary check of CNode::cs_vSend #21750