net: Add missing cs_vNodes lock #18458
pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2003-netLock changing 5 files +31 −18-
MarcoFalke commented at 2:14 pm on March 28, 2020: memberFixes #18457
-
MarcoFalke force-pushed on Mar 28, 2020
-
DrahtBot added the label P2P on Mar 28, 2020
-
laanwj added this to the milestone 0.20.0 on Mar 28, 2020
-
in src/net.cpp:93 in 8888d9f0ac outdated
89@@ -90,6 +90,9 @@ std::map<CNetAddr, LocalServiceInfo> mapLocalHost GUARDED_BY(cs_mapLocalHost); 90 static bool vfLimited[NET_MAX] GUARDED_BY(cs_mapLocalHost) = {}; 91 std::string strSubVersion; 92 93+extern RecursiveMutex cs_main;
laanwj commented at 4:00 pm on March 28, 2020:It seems like kind of a layer violation to import these here, but, I don’t know a better solution either.
MarcoFalke commented at 4:32 pm on March 28, 2020:Agree that it is a layer violation. If I included it via the header, it would be a circular dependency according to some linter we run. But the same pattern is used in a lot of other places, seegit grep 'extern RecursiveMutex cs_main;'
. Those philosophical locking “issues” and code dependencies/layer violations can be fixed later, if needed.
sipa commented at 6:48 pm on March 28, 2020:This is pretty ugly, but I see it may be needed. Agreed with fixing it this way for now.
FWIW, IMO having an ’extern’ like this is as much a circular dependency as what you’re trying to avoid, just one that isn’t detected by the linter.
MarcoFalke commented at 7:16 pm on March 28, 2020:At least withextern
we can avoid including the full header and littering the namespace here ;)
MarcoFalke commented at 1:36 pm on March 29, 2020:Fixed -
practicalswift commented at 12:17 pm on March 29, 2020: contributor
Concept ACK
Very nice to see a
NO_THREAD_SAFETY_ANALYSIS
annotation go :)FWIW:
0$ git grep -E "^[^/]*NO_THREAD_SAFETY_ANALYSIS" -- ":(exclude)src/leveldb/" ":(exclude)src/threadsafety.h" 1src/net.h: void Stop() NO_THREAD_SAFETY_ANALYSIS; 2src/wallet/wallet.h: CAmount GetAvailableCredit(bool fUseCache = true, const isminefilter& filter = ISMINE_SPENDABLE) const NO_THREAD_SAFETY_ANALYSIS; 3src/wallet/wallet.h: std::set<uint256> GetConflicts() const NO_THREAD_SAFETY_ANALYSIS; 4src/wallet/wallet.h: int GetDepthInMainChain() const NO_THREAD_SAFETY_ANALYSIS;
-
MarcoFalke force-pushed on Mar 29, 2020
-
MarcoFalke commented at 1:36 pm on March 29, 2020: member
-
MarcoFalke force-pushed on Mar 29, 2020
-
MarcoFalke force-pushed on Mar 29, 2020
-
promag commented at 3:07 pm on March 29, 2020: member
Concept ACK.
How about split
CConnman::Stop
? InShutdown(NodeContext& node)
we would then have:0if (node.connman) { 1 node.connman->BeforeStop(); 2 LOCK2(::cs_main, ::g_cs_orphans); 3 node.connman->Stop(); 4}
You could also add annotation to
Stop
. -
net: Add missing cs_vNodes lock fa369651c5
-
MarcoFalke force-pushed on Mar 29, 2020
-
MarcoFalke commented at 3:51 pm on March 29, 2020: member
Took suggestion by @promag
You could also add annotation to Stop.
No, that would be incorrect. The locks are not needed for the correct functionality of the connection manager itself. They are only needed when the connection manager is used in the context of a full node that also runs net processing and a scheduler. I think init.cpp is the best place to document this, since that is the place where the full node gets pieced together.
-
promag commented at 4:01 pm on March 29, 2020: memberCode review ACK fa369651c5523f393e0bfcfa328b8e27006711aa.
-
DrahtBot commented at 7:34 pm on March 29, 2020: member
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #14053 (Add address-based index (attempt 4?) by marcinja)
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.
-
MarcoFalke commented at 4:54 pm on April 2, 2020: memberWould be nice to get one or two more ACKs on this
-
promag commented at 11:29 pm on April 2, 2020: memberACK ACK, you are welcome.
-
laanwj commented at 6:54 pm on April 6, 2020: memberACK fa369651c5523f393e0bfcfa328b8e27006711aa
-
laanwj merged this on Apr 6, 2020
-
laanwj closed this on Apr 6, 2020
-
MarcoFalke deleted the branch on Apr 6, 2020
-
sidhujag referenced this in commit 81f3ca7b68 on Apr 8, 2020
-
jasonbcox referenced this in commit 781244d1ff on Nov 18, 2020
-
DrahtBot locked this on Feb 15, 2022