This is a piece of #21878, chopped off to ease review.
The following pattern was duplicated in CConnman:
0lock
1create a copy of vNodes, add a reference to each one
2unlock
3... use the copy ...
4lock
5release each node from the copy
6unlock
Put that code in a RAII helper that reduces it to:
0create snapshot "snap"
1... use the copy ...
2// release happens when "snap" goes out of scope
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
1466@@ -1467,8 +1467,6 @@ void CConnman::SocketHandler()
1467 }
1468 }
1469
1470- NodesSnapshot snap{*this};
157674ef213e6329215d1b7eb85dacbc23c468db
This snap catches nodes added in the above AcceptConnection where the new snap at the beginning doesn’t. Is this intentional?
Hmm, good catch! No, that is not intentional.
So, the previous code did:
SocketEvents(), adding ready sockets from vNodes to {recv,send,error}_set.AcceptConnection(), adding new entries to vNodes.vNodes, for each node:
3.1. Check whether its socket is in one of {recv,send,error}_set. Observation - the sockets of nodes that were added in 2. will not be in any of {recv,send,error}_set, so for them all of recvSet, sendSet and errorSet will be false.
3.2. InactivityCheck() - will also be done for nodes added in 2.The new code will skip nodes added in 2. in the 3. loop. That’s the same as the old code with respect to 3.1. The only difference is that the new code will not do InactivityCheck() for newly accepted nodes. I think that is ok because it is unlikely that a newly accepted peer would be considered inactive. In the rare case where this might have happened in the old code and will not happen in the new code, the new code will do the check on the next call to SocketHandler().
What do you think? Leave it as is or somehow ensure that new and old behave in an identical way?
InactivityCheck for new nodes is pointless then maybe move // Accept new connections to the end?
Added an explicit commit (a71b3f426) to show the intention. Putting the accept at the end somehow looked strange to me.
Maybe these two actions deserve to be split in separate functions (ie split SocketHandler()): 1. checking which of the listening sockets are ready for IO and accepting connections on them and 2. checking which of the already connected sockets are ready for IO and processing them. Would require even more changes.
// Take a snapshot of current nodes before acceptation new ones - new nodes are irrelevant below or something like that.
Right, a comment is warranted. Added:
0 // Accept new connections. Done after taking a snapshot of the nodes because
1 // `AcceptConnection()` will add new entries to `vNodes` which are unwanted
2 // in the loop below because:
3 // - all of `recvSet`, `sendSet` and `errorSet` will be `false` for them
4 // - `InactivityCheck()` will either be a noop or will wrongly command to
5 // disconnect the node before we have tried sending or receiving anything
6 // to it.
644726117...92f62d9ea: add an explicit commit that shows we intentionally don’t process newly accepted nodes, as discussed in #21943 (review)
1446@@ -1451,8 +1447,10 @@ void CConnman::SocketEvents(std::set<SOCKET> &recv_set, std::set<SOCKET> &send_s
1447
1448 void CConnman::SocketHandler()
1449 {
1450+ NodesSnapshot snap{*this};
1451+
1452 std::set<SOCKET> recv_set, send_set, error_set;
1453- SocketEvents(recv_set, send_set, error_set);
1454+ SocketEvents(snap.Nodes(), recv_set, send_set, error_set);
SocketEvents() to below the Accept new connections logic? I think it’s simpler to reason about if we’re not holding the nodes snapshot across the Accept new connections logic.
recv_set that’s populated by SocketEvents().
1275@@ -1257,6 +1276,40 @@ class CConnman
1276 */
1277 std::vector<CService> m_onion_binds;
1278
1279+ /**
1280+ * RAII helper to atomically create a copy of `vNodes` and add a reference
1281+ * to each of the nodes. The nodes are released when this object is destroyed.
1282+ */
1283+ class NodesSnapshot
I’m guessing you’ve defined this class within CConnman to enable access to the private members. You could instead define the class in net.cpp as a standalone by making vNodes and cs_vNodes protected & declaring friend class NodesSnapshot here. Seems nice to keep this mechanism that’s only used internally out of the header file.
diff 👇🏽
(also includes constifying m_nodes_copy)
0diff --git a/src/net.cpp b/src/net.cpp
1index 5fd1bf754d..6040ee86be 100644
2--- a/src/net.cpp
3+++ b/src/net.cpp
4@@ -110,6 +110,39 @@ std::map<CNetAddr, LocalServiceInfo> mapLocalHost GUARDED_BY(cs_mapLocalHost);
5 static bool vfLimited[NET_MAX] GUARDED_BY(cs_mapLocalHost) = {};
6 std::string strSubVersion;
7
8+/**
9+ * RAII helper to atomically create a copy of `vNodes` and add a reference
10+ * to each of the nodes. The nodes are released when this object is destroyed.
11+ */
12+class NodesSnapshot
13+{
14+ public:
15+ explicit NodesSnapshot(const CConnman& connman) : m_connman{connman}, m_nodes_copy{m_connman.vNodes}
16+ {
17+ LOCK(m_connman.cs_vNodes);
18+ for (auto& node : m_nodes_copy) {
19+ node->AddRef();
20+ }
21+ }
22+
23+ ~NodesSnapshot()
24+ {
25+ LOCK(m_connman.cs_vNodes);
26+ for (auto& node : m_nodes_copy) {
27+ node->Release();
28+ }
29+ }
30+
31+ const std::vector<CNode*>& Nodes() const
32+ {
33+ return m_nodes_copy;
34+ }
35+
36+ private:
37+ const CConnman& m_connman;
38+ const CConnman& m_connman;
39+ const std::vector<CNode*> m_nodes_copy;
40+};
41+
42 void CConnman::AddAddrFetch(const std::string& strDest)
43 {
44 LOCK(m_addr_fetches_mutex);
45diff --git a/src/net.h b/src/net.h
46index 846c4c0ece..f39c46c166 100644
47--- a/src/net.h
48+++ b/src/net.h
49@@ -1021,6 +1021,10 @@ public:
50 /** Return true if we should disconnect the peer for failing an inactivity check. */
51 bool ShouldRunInactivityChecks(const CNode& node, std::optional<int64_t> now=std::nullopt) const;
52
53+protected:
54+ std::vector<CNode*> vNodes GUARDED_BY(cs_vNodes);
55+ mutable RecursiveMutex cs_vNodes;
56+
57 private:
58 struct ListenSocket {
59 public:
60@@ -1153,9 +1157,7 @@ private:
61 RecursiveMutex m_addr_fetches_mutex;
62 std::vector<std::string> vAddedNodes GUARDED_BY(cs_vAddedNodes);
63 mutable RecursiveMutex cs_vAddedNodes;
64- std::vector<CNode*> vNodes GUARDED_BY(cs_vNodes);
65 std::list<CNode*> vNodesDisconnected;
66- mutable RecursiveMutex cs_vNodes;
67 std::atomic<NodeId> nLastNodeId{0};
68 unsigned int nPrevNodeCount{0};
69
70@@ -1276,40 +1278,7 @@ private:
71 */
72 std::vector<CService> m_onion_binds;
73
74- /**
75- * RAII helper to atomically create a copy of `vNodes` and add a reference
76- * to each of the nodes. The nodes are released when this object is destroyed.
77- */
78- class NodesSnapshot
79- {
80- public:
81- explicit NodesSnapshot(const CConnman& connman) : m_connman{connman}
82- {
83- LOCK(m_connman.cs_vNodes);
84- m_nodes_copy = m_connman.vNodes;
85- for (auto& node : m_nodes_copy) {
86- node->AddRef();
87- }
88- }
89-
90- ~NodesSnapshot()
91- {
92- LOCK(m_connman.cs_vNodes);
93- for (auto& node : m_nodes_copy) {
94- node->Release();
95- }
96- }
97-
98- const std::vector<CNode*>& Nodes() const
99- {
100- return m_nodes_copy;
101- }
102-
103- private:
104- const CConnman& m_connman;
105- std::vector<CNode*> m_nodes_copy;
106- };
107-
108+ friend class NodesSnapshot;
109 friend struct CConnmanTest;
110 friend struct ConnmanTestMsg;
111 };
I like this. A few notes:
+ const CConnman& m_connman; line twice) and so doesn’t apply with git apply.vNodes and cs_vNodes don’t need to change to be protected. Friend classes have access to private members.NodesSnapshot can be in the unnamed namespace since it’s only needed in the net.cpp translation unit.
The m_nodes_copy{m_connman.vNodes} in the initializer list is accessing vNodes without the cs_vNodes lock. You could update it to take the lock:
0diff --git a/src/net.cpp b/src/net.cpp
1index 6040ee86be..d39aa14b20 100644
2--- a/src/net.cpp
3+++ b/src/net.cpp
4@@ -117,7 +117,9 @@ std::string strSubVersion;
5 class NodesSnapshot
6 {
7 public:
8- explicit NodesSnapshot(const CConnman& connman) : m_connman{connman}, m_nodes_copy{m_connman.vNodes}
9+ explicit NodesSnapshot(const CConnman& connman) :
10+ m_connman{connman},
11+ m_nodes_copy{WITH_LOCK(m_connman.cs_vNodes, return m_connman.vNodes;)}
12 {
13 LOCK(m_connman.cs_vNodes);
14 for (auto& node : m_nodes_copy) {
But that would introduce a race condition if another thread deleted a node between the initializer list and the ctor being run.
I think better just to assign m_nodes_copy in the ctor.
I’m guessing you’ve defined this class within CConnman to enable access to the private members.
No, I did it to minimize the scope of the newly added class.
Seems nice to keep this mechanism that’s only used internally out of the header file.
Why? It is used internally by CConnman and is defined in its private: section. If defined in net.cpp that would broaden its scope unnecessary.
Seems nice to keep this mechanism that’s only used internally out of the header file.
Why?
Because it’s good to keep header files limited to what’s required to be included into other translation units (as far as is possible). That’s beneficial for both build time and code management reasons.
I think both ways are fine, and that eventually we’ll want to virtualize both CNode’s and CConnman’s public interfaces so that they can be mocked out for testing, at which point the private methods could all be moved to the implementation file.
That’s beneficial for both build time…
I do not think it is justified to put private class members as globals in the .cpp file in order to minimize the header file so that it compiles faster.
… and code management reasons.
Unnecessary broadened scope has an adverse effect on code management.
If NodesSnapshot was defined as a standalone class in net.cpp, it could be added to the unnamed namespace so it would only be accessible within the same translation unit, but it would broaden the scope beyond CConnman to within the translation unit.
My preference for it to be in the .cpp is because I find it easier to understand responsibilities & maneuver the files when the headers capture the way the classes interact with external callers. But I totally understand the point about broadening scope, and think this ultimately comes down to a preference.
I’m going to resolve this conversation, thanks for the feedback @vasild & @jnewbery :)
@amitiuttarwar, maybe you would like the following pattern to be more widespread:
net_processing since #20811.
1446@@ -1451,8 +1447,10 @@ void CConnman::SocketEvents(std::set<SOCKET> &recv_set, std::set<SOCKET> &send_s
1447
1448 void CConnman::SocketHandler()
1449 {
1450+ NodesSnapshot snap{*this};
0 const NodesSnapshot snap{*this};
2172
2173- for (CNode* pnode : vNodesCopy)
2174 {
2175- if (pnode->fDisconnect)
2176- continue;
2177+ NodesSnapshot snap{*this};
0 const NodesSnapshot snap{*this};
1305+ return m_nodes_copy;
1306+ }
1307+
1308+ private:
1309+ const CConnman& m_connman;
1310+ std::vector<CNode*> m_nodes_copy;
0 const std::vector<CNode*> m_nodes_copy;
Can const this if you add the constructor assignment to the initializer list (done in #21943 (review) diff)
AddRef() should be done under the lock.
Why AddRef? @amitiuttarwar suggestion can be accomplished with
0 explicit NodesSnapshot(const CConnman& connman)
1+ : m_nodes_copy(WITH_LOCK(connman.cs_vNodes, return connman.vNodes))
2 {
3- LOCK(connman.cs_vNodes);
4- m_nodes_copy = connman.vNodes;
5 for (auto& node : m_nodes_copy) {
6 node->AddRef();
7 }
8@@ -1305,7 +1304,7 @@ private:
9 }
10
11 private:
12- std::vector<CNode*> m_nodes_copy;
13+ const std::vector<CNode*> m_nodes_copy;
14 };
Never mind, I see why AddRef needs to be under the lock. It’s still possible to use the WITH_LOCK approach so m_nodes_copy could be is const, but not worth it IMO.
Yeah :) AddRef() needs to be under the same lock that did the copy (without release in between) because otherwise some of the copied elements may be destroyed in the meantime and node-> to try to dereference a dangling pointer while trying to call AddRef().
It should work with something like
0explicit NodesSnapshot(const CConnman& connman)
1 : m_nodes_copy(WITH_LOCK(connman.cs_vNodes, for (auto& node : connman.vNodes) { node->AddRef(); } ; return connman.vNodes))
but that is not worth it indeed :)
1294+
1295+ ~NodesSnapshot()
1296+ {
1297+ LOCK(m_connman.cs_vNodes);
1298+ for (auto& node : m_nodes_copy) {
1299+ node->Release();
I see that you haven’t changed the behavior here, but I’m curious about the pre-existing logic:
Is it necessary for Release() to be under the lock?
It makes sense that AddRef has to be under the lock so there isn’t a race between a thread that is trying to operate on the node, and a thread that is evaluating for disconnect. However, with Release, I don’t think there is actually potential for a race.. if GetRefCount returns > 0, DisconnectNodes will delay processing it. And since nRefCount is atomic, it shouldn’t be a problem if multiple threads are trying to increment / decrement at the same time. Does this reasoning seem sound? Am I missing something?
cc @narula
I think this is correct. There’s no need to hold cs_vNodes when calling node->Release();
Removing this lock means that the m_connman member is no longer required.
Is it necessary for
Release()to be under the lock?
The lock has two effects:
Release()s happen atomically together. If removed, then other threads could observe some nodes from the snapshot released and some not yet released. I think that is ok.nRefCount-- on the atomic variable has the same effect because -- is equivalent to fetch_sub() which uses memory_order_seq_cst by default.Removed the lock and simplified NodesSnapshot, thanks!
Perhaps beyond the scope of this PR, but it may be better if Release() asserted that the count is valid:
0void Release()
1{
2 assert(nRefCount > 0);
3 nRefCount--;
4}
The count validity is asserted in GetRefCount() but it would be better to catch the malefactor in the act :).
1298+ for (auto& node : m_nodes_copy) {
1299+ node->Release();
1300+ }
1301+ }
1302+
1303+ const std::vector<CNode*>& Nodes() const
What do you think about just wrapping the m_nodes_copy begin() and end() functions:
0diff --git a/src/net.cpp b/src/net.cpp
1index 6040ee86be..5388af02e3 100644
2--- a/src/net.cpp
3+++ b/src/net.cpp
4@@ -116,6 +116,10 @@ std::string strSubVersion;
5 */
6 class NodesSnapshot
7 {
8+ private:
9+ const CConnman& m_connman;
10+ const std::vector<CNode*> m_nodes_copy;
11+
12 public:
13 explicit NodesSnapshot(const CConnman& connman) : m_connman{connman}, m_nodes_copy{m_connman.vNodes}
14 {
15@@ -133,14 +137,18 @@ class NodesSnapshot
16 }
17 }
18
19- const std::vector<CNode*>& Nodes() const
20+ using iterator = decltype(m_nodes_copy.begin());
21+
22+ iterator begin()
23 {
24- return m_nodes_copy;
25+ return m_nodes_copy.begin();
26+ }
27+
28+ iterator end()
29+ {
30+ return m_nodes_copy.end();
31 }
32
33- private:
34- const CConnman& m_connman;
35- const std::vector<CNode*> m_nodes_copy;
36 };
and then the client code can use a NodeSnapshot in range-based loops, eg:
0→ git d
1diff --git a/src/net.cpp b/src/net.cpp
2index 6040ee86be..5388af02e3 100644
3--- a/src/net.cpp
4+++ b/src/net.cpp
5@@ -1501,7 +1509,7 @@ void CConnman::SocketHandler()
6 //
7 // Service each socket
8 //
9- for (CNode* pnode : snap.Nodes()) {
10+ for (CNode* pnode : snap) {
11 if (interruptNet)
12 return;
13
I think both ways would work.
The begin/end variant seems to be a bit more code - it saves just .Nodes() from the callers but has two methods and is also less friendly to grepping and code navigation - now I can point my “IDE” to Nodes() on the line for (CNode* pnode : snap.Nodes()) { and see what it returns or its definition. Not so with the for (CNode* pnode : snap) { line.
That said, I am fine either way but have a slight preference for the .Nodes() variant.
92f62d9...00a8c94: address review suggestions
00a8c943d7...072896b7cf: rebase due to conflicts
Concept ACK, particularly if this can reduce vNodes lock contentions in these areas that are among the most frequent and long-lasting of those I’m seeing on my nodes with -debug=lock.
It looks like the CI stalled out on the last push. In any case, it rebases cleanly onto current master, the debug build is clean, and the unit/functional test suite is green for me locally. Reviewing.
1487@@ -1492,13 +1488,21 @@ void CConnman::SocketEvents(std::set<SOCKET> &recv_set, std::set<SOCKET> &send_s
1488
1489 void CConnman::SocketHandler()
1490 {
1491+ const NodesSnapshot snap{*this, false};
0d706a6 if you retouch or have to rebase
0 const NodesSnapshot snap{*this, /* shuffle=*/ false};
func(/* description */ param);
Yes. I’ve very recently started to switch to the /*named_arg=*/ format for bugprone-argument-comment added in fa57fa1a2e7. Not sure how picky clang is about it though.
= is needed for clang-tidy. I am likely going to use (/*foo_bar=*/123, from now on (I think clang-format “mirrors” the whitespace, so clang-format valid options are (/*foo_bar=*/123, /*bar_foor=*/321, or (/* foo_bar = */ 123, /* bar_foor = */ 321,
/* bar_foor= */ 321 is also ok for clang-format (re-formatting that with clang-format leaves it unmodified). I don’t care which one as long as it is consistent through the code. I will use /*foo_bar=*/123 too.
2228- if (pnode->fDisconnect)
2229- continue;
2230+ // Randomize the order in which we process messages from/to our peers.
2231+ // This prevents attacks in which an attacker exploits having multiple
2232+ // consecutive connections in the vNodes list.
2233+ const NodesSnapshot snap{*this, true};
0d706a6 if you retouch or have to rebase, I think the comment about randomizing makes more sense with a named arg for shuffle
0 const NodesSnapshot snap{*this, /* shuffle=*/ true};
func(/* description */ param);
991@@ -992,8 +992,27 @@ class CConnman
992 void NotifyNumConnectionsChanged();
993 /** Return true if the peer is inactive and should be disconnected. */
994 bool InactivityCheck(const CNode& node) const;
995- bool GenerateSelectSet(std::set<SOCKET> &recv_set, std::set<SOCKET> &send_set, std::set<SOCKET> &error_set);
996- void SocketEvents(std::set<SOCKET> &recv_set, std::set<SOCKET> &send_set, std::set<SOCKET> &error_set);
997+
998+ /**
999+ * Generate a collection of sockets that we want to be checked for readiness for IO.
bf0dc73 nit doc suggestion, s/readiness for IO/IO readiness/ or the pithier
0 * Generate a collection of sockets to check for IO readiness.
1331 {
1332 for (const ListenSocket& hListenSocket : vhListenSocket) {
1333 recv_set.insert(hListenSocket.socket);
1334 }
1335
1336- {
LOCK(cs_vNodes); mutex lock that is removed in the previous commit, so it may be a bit clearer to remove them at the same time as the lock. No strong opinion though.
Yes, no worries. Since some time now my .gitconfig has this set by default to simplify whitespace review (open to suggestions).
0[diff]
1 renames = copies
2 colorMoved = dimmed-zebra
3 colorMovedWs = allow-indentation-change
Tested ACK 072896b7cf5e69c11a2e6d9e7163df4e0472f504 rebased to current master. Reviewed, debug built, and ran unit tests on each commit. Ran a node with this branch rebased to master on mainnet for a day (edit: 3 days) with -debug=lock and 30-40 peers and seeing an impressive reduction in the frequent vNodes lock contentions with grep "lock contention cs_vNodes" ~/.bitcoin/debug.log and very few contentions with grep "connman.cs_vNodes" ~/.bitcoin/debug.log
Nice work!
git range-diff a9d0cec 072896b a4fbf1b following my earlier full review (https://github.com/bitcoin/bitcoin/pull/21943#pullrequestreview-758400122)
a4fbf1ba59...40e0bc8de2: rebase and switch to /*description=*/123 following #21943 (review).
Sorry for the noise, I hope no more changes to this PR.
1502
1503 if (interruptNet) return;
1504
1505 //
1506- // Accept new connections
1507+ // Accept new connections. Done after taking a snapshot of the nodes because
SocketHandler() entirely and put in ThreadSocketHandler() (or a dedicated function for accepting new connections). It’s difficult to get my head around creating a NodesSnapshot, receiving events for those nodes, then updating vNodes and finally processing the events for the nodes in the snapshot.
I split the accepting of the new connections into its own method, that is independent of CConnMan::SocketHandler(), does its own socket polling and can be called independently from CConnMan::SocketHandler(). However after doing that, the following realization dawned on me (tldr: I ditched that):
:bulb: We don’t want to wait separately for incoming connections because that event is “rare”, meaning most of the time we would just sleep for some time until poll(2) times out, signaling no socket is ready for IO (no new incoming connections). This would have an adverse effect on the throughput - even on busy nodes that constantly have incoming/outgoing data, we would needlessly sleep for a short while, frequently.
So, we want to poll(2) the already connected sockets and the listening ones together, in one poll(2) call.
I made some changes to CConnMan::SocketHander() to (hopefully) make it easier to read, see #21943 (comment)
1331@@ -1332,57 +1332,53 @@ bool CConnman::InactivityCheck(const CNode& node) const
1332 return false;
1333 }
1334
1335-bool CConnman::GenerateSelectSet(std::set<SOCKET> &recv_set, std::set<SOCKET> &send_set, std::set<SOCKET> &error_set)
1336+bool CConnman::GenerateSelectSet(const std::vector<CNode*>& nodes, std::set<SOCKET> &recv_set, std::set<SOCKET> &send_set, std::set<SOCKET> &error_set)
& for the 3 set arguments (to conform to style convention)?
(Same comment for SocketEvents() below, and also in net.h.)
clang-format on this line. Done now.
1214+ }
1215+ if (shuffle) {
1216+ FastRandomContext rng;
1217+ Shuffle(m_nodes_copy.begin(), m_nodes_copy.end(), rng);
1218+ }
1219+ }
I would do this a little differently:
0NodesSnapshot(const CConnman& connman)
1{
2 LOCK(connman.cs_vNodes);
3 m_nodes_copy = connman.vNodes;
4 for (auto& node : m_nodes_copy) {
5 node->AddRef();
6 }
7}
8std::vector<CNode*>& Shuffle() {
9 FastRandomContext rng;
10 ::Shuffle(m_nodes_copy.begin(), m_nodes_copy.end(), rng);
11 return m_nodes_copy;
12}
Shuffle() right after construction. I don’t see the benefit of it. It would require the snapshot variable to be non-const.
1493@@ -1498,13 +1494,21 @@ void CConnman::SocketEvents(std::set<SOCKET> &recv_set, std::set<SOCKET> &send_s
1494
1495 void CConnman::SocketHandler()
1496 {
1497+ const NodesSnapshot snap{*this, /*shuffle=*/false};
Suggestion, this may be a little more elegant than the boolean shuffle argument (see suggested changes to NodesSnapshot)
0 const NodesSnapshot snap{*this};
I think it is ok as it is now. We are not frowning upon boolean arguments, are we? If the boolean argument looks too bad, then a 2-value enum can be introduced so call sites look like:
0const NodesSnapshot snap{*this, NodesSnapshot::SHUFFLE};
1// or
2const NodesSnapshot snap{*this, NodesSnapshot::NO_SHUFFLE};
but I think this would be an overkill.
Perhaps this is outside the scope of the PR, but I’d like:
0 void Release()
1 {
2+ assert(nRefCount > 0);
3 nRefCount--;
4 }
99c1af5a8f...8589c55e27: rebase and reorganize CConnMan::SocketHandler(), following comments from above: 1 and 2.
Accepting new connections does not require the snapshot of the existing connected nodes, so make this explicit by accepting new connections after the snapshot has been destroyed, at the end of CConnMan::SocketHandler(). Move the pieces of code from CConnMan::SocketHandler() that handle existing connections and accept new ones into their own methods and call those methods from CConnMan::SocketHandler() to improve the readability.
Invalidates ACK from @jonatack
Previously invalidated ACK from @promag
1516- for (const ListenSocket& hListenSocket : vhListenSocket)
1517- {
1518- if (hListenSocket.socket != INVALID_SOCKET && recv_set.count(hListenSocket.socket) > 0)
1519- {
1520- AcceptConnection(hListenSocket);
1521+ if (interruptNet) {
3c8fcb94691584871edaa1a142bfa0ebadc01d67
I think you can ditch this?
You are right - the surrounding SocketEvents() and SocketHandlerConnected() both contain a similar check and would quit “quickly” if interruptNet becomes true.
Maybe the newly added SocketHandlerListening() deserves such a check too, once per each listening socket to be processed?
8589c55e27...db0a01e0d1: remove unnecessary interruptNet check from SocketEvents(), add such a check to SocketHandlerListening(). Thanks, @promag, #21943 (review) !
Invalidates ACK from @promag
Previously invalidated ACK from @jonatack
1520- {
1521- LOCK(cs_vNodes);
1522- vNodesCopy = vNodes;
1523- for (CNode* pnode : vNodesCopy)
1524- pnode->AddRef();
1525+ // Check for readiness the already connected sockets and the listening sockets
s/readiness/the readiness of/ ? Perhaps also clarify “ready”. Also s/none is/none are/.
1509- if (hListenSocket.socket != INVALID_SOCKET && recv_set.count(hListenSocket.socket) > 0)
1510- {
1511- AcceptConnection(hListenSocket);
1512- }
1513- }
1514+ const NodesSnapshot snap{*this, /*shuffle=*/false};
s/snap/snapshot/ would be clearer
snap is used as a short for snapshot, not in the “verb” sense. I have seen this elsewhere too. For example zfs list -t snapshot and zfs list -t snap both do the same thing.
1235+ node->AddRef();
1236+ }
1237+ }
1238+ if (shuffle) {
1239+ FastRandomContext rng;
1240+ Shuffle(m_nodes_copy.begin(), m_nodes_copy.end(), rng);
9ca6dac0 can omit localvar rng
0 if (shuffle) {
1- FastRandomContext rng;
2- Shuffle(m_nodes_copy.begin(), m_nodes_copy.end(), rng);
3+ Shuffle(m_nodes_copy.begin(), m_nodes_copy.end(), FastRandomContext());
4 }
1497- SocketEvents(recv_set, send_set, error_set);
1498-
1499- if (interruptNet) return;
1500+ std::set<SOCKET> recv_set;
1501+ std::set<SOCKET> send_set;
1502+ std::set<SOCKET> error_set;
4097d14 unneeded style change, would leave as-is (and as currently exists in both of the SocketEvents() functions)
0- std::set<SOCKET> recv_set;
1- std::set<SOCKET> send_set;
2- std::set<SOCKET> error_set;
3-
4+ std::set<SOCKET> recv_set, send_set, error_set;
Consider:
0struct SocketSets {
1 std::set<SOCKET> recv;
2 std::set<SOCKET> send;
3 std::set<SOCKET> error;
4}
@LarryRuane, something like this is coming as a subsequent change from #21878 (see commit net: introduce Sock::WaitMany() and the structures WaitEvents and WaitData introduced in that commit).
The current combination of sets stores the sockets in “one pile of sockets that are ready for read” and “one pile of sockets that are ready for write” (notice that a given socket may be in both). It is more convenient to have just one pile of sockets each one with attached ready-for-read and ready-for-write flags.
I like the idea of abstracting this but it seems out of scope of this PR.
It is more convenient to have just one pile of sockets each one with attached ready-for-read and ready-for-write flags.
Eventually it depends on the selection mechanism used isn’t it? When it’s abstracted, might as well store it in a format ready to feed to say, poll() or select() whatever is used.
net: introduce Sock::WaitMany(). We can discuss it there, I don’t have a strong opinion on how they are packed exactly. Or if this PR is stuck we can move the discussion to #21878 where WaitData is introduced.
Once this is PR is merged I will chop off another piece from #21878 into a smaller/manageable PR that will contain the commit which introduces such a packing
net: introduce Sock::WaitMany()
There are too many conflicts if I try to cherry-pick the WaitMany() stuff on top of bare master - it touches the same areas of code as the preceding commits from #21878. I chopped off those preceding commits in separate PRs instead: #23601 and #23604.
0WaitMany | net: use Sock::WaitMany() instead of CConnman::SocketEvents()
1stuff | net: introduce Sock::WaitMany()
2 | net: also wait for exceptional events in Sock::Wait()
3
4[#23601](/bitcoin-bitcoin/23601/) | net: don't check if the listening socket is valid
5
6 | scripted-diff: rename CNode::cs_hSocket to CNode::m_sock_mutex
7[#23604](/bitcoin-bitcoin/23604/) | net: use Sock in CNode
8 | fuzz: move FuzzedSock earlier in src/test/fuzz/util.h
9
10 | net: change CreateNodeFromAcceptedSocket() to take Sock
11[#21879](/bitcoin-bitcoin/21879/) | net: use Sock in CConnman::ListenSocket
12 | net: add new method Sock::Accept() that wraps accept()
1524- pnode->AddRef();
1525+ // Check for readiness the already connected sockets and the listening sockets
1526+ // in one call. If none is ready, wait for a short while and return empty sets.
1527+ SocketEvents(snap.Nodes(), recv_set, send_set, error_set);
1528+
1529+ SocketHandlerConnected(snap.Nodes(), recv_set, send_set, error_set);
4097d14 found these comments helpful, would keep
0+ // Service each socket
1 SocketHandlerConnected(snap.Nodes(), recv_set, send_set, error_set);
2 }
3
4+ // Accept new connections
5 SocketHandlerListening(recv_set);
1023+ const std::set<SOCKET>& recv_set,
1024+ const std::set<SOCKET>& send_set,
1025+ const std::set<SOCKET>& error_set);
1026+
1027+ /**
1028+ * Accept an incoming connection from listening sockets that are ready for read.
4097d14 the existing “Accept incoming connections” comment is an easier-to-understand description for me than this one.
Maybe:
0 * Accept incoming connections, i.e. one from each read-ready listening socket.
Following up on my feedback #21943 (review), as of db0a01e0d1a the commits are now out of order. The last commit
style: remove unnecessary braces
They were needed to define the scope of `LOCK(cs_vNodes)` which was
removed in the previous commit.
should be after the second commit net: keep reference to each node during socket wait (or squashed).
Almost-ACK db0a01e0d1a with some suggestions below if you retouch.
The following pattern was duplicated in CConnman:
```cpp
lock
create a copy of vNodes, add a reference to each one
unlock
... use the copy ...
lock
release each node from the copy
unlock
```
Put that code in a RAII helper that reduces it to:
```cpp
create snapshot "snap"
... use the copy ...
// release happens when "snap" goes out of scope
```
Create the snapshot of `CConnman::vNodes` to operate on earlier in
`CConnman::SocketHandler()`, before calling `CConnman::SocketEvents()`
and pass the `vNodes` copy from the snapshot to `SocketEvents()`.
This will keep the refcount of each node incremented during
`SocketEvents()` so that the `CNode` object is not destroyed before
`SocketEvents()` has finished.
Currently in `SocketEvents()` we only remember file descriptor numbers
(when not holding `CConnman::cs_vNodes`) which is safe, but we will
change this to remember pointers to `CNode::m_sock`.
They were needed to define the scope of `LOCK(cs_vNodes)` which was
removed in the previous commit. Re-indent in a separate commit to ease
review (use `--ignore-space-change`).
`CConnman::SocketHandler()` does 3 things:
1. Check sockets for readiness
2. Process ready listening sockets
3. Process ready connected sockets
Split the processing (2. and 3.) into separate methods to make the code
easier to grasp.
Also, move the processing of listening sockets after the processing of
connected sockets to make it obvious that there is no dependency and
also explicitly release the snapshot before dealing with listening
sockets - it is only necessary for the connected sockets part.
1512@@ -1513,18 +1513,12 @@ void CConnman::SocketHandler()
1513 }
1514 }
1515
1516+ const NodesSnapshot snap{*this, /*shuffle=*/false};
I guess it’s just me, but I still don’t like this boolean (but I do agree with you that it’s better than an enum). In general, I dislike making a decision at run time (in this case, the constructor evaluating the shuffle argument) that is actually being decided at compile (in this case, all the callers pass a constant bool for this argument), if possible (sometimes it’s not).
What do you think about always shuffling? Just make that part of the semantics of this object? Could there ever be any harm (other than a very slight performance hit)?
What do you think about always shuffling?
I do not have a strong opinion. My main point with the current code is to preserve behavior - it was not shuffled before, it will not be shuffled with this PR. This PR has a different purpose.
In general, I try to avoid such unnecessary behavioral changes. They may have very subtle effects.
Maybe it is ok to always shuffle and remove the boolean argument from the NodesSnapshot constructor? What do other reviewers think?