Do not hold cs_vNodes when making ForEachNode Callbacks #10697
Conversation
|
hm this may require carefully auditing every use to determine if the code in question isn't being protected by an assumed holding of cs_vnodes. |
|
Indeed. I believe it is the case that none of them are needed, but I may be incorrect. Note that folks may want to hold off on review, @cfields indicated he may have a more complete solution to the general mess that is our manual CNode refcounting. |
|
No, please go ahead and review. I've worked on this several times now and haven't come up with anything that could be considered an improvement. I'm currently taking another look, but that shouldn't block this unless I manage to come up with something in the next day or so. |
|
Ping @theuni @TheBlueMatt, are you working on this? |
src/net.cpp
Outdated
| break; | ||
| } | ||
| } | ||
| return found != nullptr && NodeFullyConnected(found) && func(found); | ||
| bool res = found != nullptr && NodeFullyConnected(found) && func(found); |
There was a problem hiding this comment.
This still calls func while cs_vNodes is held. Is that intentional?
src/net.cpp
Outdated
| } | ||
| } | ||
| for (CNode* node : vNodes_copy) { | ||
| if (NodeFullyConnected(node)) |
There was a problem hiding this comment.
Nit: either { } around if body, or body on the same line (in multiple places).
e4f477b to
7f1ae14
Compare
|
Addressed @sipa's comments. |
7f1ae14 to
8b4a18f
Compare
8b4a18f to
50ef0b0
Compare
| return found != nullptr && NodeFullyConnected(found) && func(found); | ||
| bool res = false; | ||
| if (found != nullptr) { | ||
| res = NodeFullyConnected(found) && func(found); |
There was a problem hiding this comment.
This will leak reference count if func throws. Suggest using RAII helper to prevent this, see NodeRef in eeb85c6. NodeRef also simplifies the ForEachNode implementations there.
| } | ||
|
|
||
| void CConnman::ForEachNode(std::function<void (CNode* pnode)> func) | ||
| { | ||
| std::vector<CNode*> vNodes_copy; |
There was a problem hiding this comment.
These should just call ForEachNodeThen with a null post functions so this code doesn't have to be repeated so many times. Again see eeb85c6.
| #define LEAVE_CRITICAL_SECTION(cs) \ | ||
| { \ | ||
| (cs).unlock(); \ | ||
| LeaveCritical((void*)(&cs)); \ |
There was a problem hiding this comment.
This should just say LeaveCritical(&(cs)). cs should be parenthesized since it's a macro argument, and the cast is unneeded.
|
Superceded by #11604 |
Generally holding locks while making callbacks is a bad idea, and for my eventual per-CNodeState locks. Also makes DEBUG_LOCKORDER more strict in a way that it assumes so best to enforce the requirement that locks are released in the order they are taken.
These two commits were pulled out of #10652 as they were entirely unconnected from the rest. Note that there was already a comment on the ForEachNode commit there.