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.
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?
2856 | @@ -2857,12 +2857,96 @@ bool CConnman::ForNode(NodeId id, std::function<bool(CNode* pnode)> func) 2857 | for (auto&& pnode : vNodes) { 2858 | if(pnode->GetId() == id) { 2859 | found = pnode; 2860 | + found->AddRef(); 2861 | break; 2862 | } 2863 | } 2864 | - return found != nullptr && NodeFullyConnected(found) && func(found); 2865 | + bool res = found != nullptr && NodeFullyConnected(found) && func(found);
This still calls func while cs_vNodes is held. Is that intentional?
2877 | + node->AddRef(); 2878 | + vNodes_copy.push_back(node); 2879 | + } 2880 | + } 2881 | + for (CNode* node : vNodes_copy) { 2882 | + if (NodeFullyConnected(node))
Nit: either { } around if body, or body on the same line (in multiple places).
Addressed @sipa's comments.
2853 | } 2854 | } 2855 | - return found != nullptr && NodeFullyConnected(found) && func(found); 2856 | + bool res = false; 2857 | + if (found != nullptr) { 2858 | + res = NodeFullyConnected(found) && func(found);
This will leak reference count if func throws. Suggest using RAII helper to prevent this, see NodeRef in eeb85c63cf880bc7344413b266c3af793d5cb8d2. NodeRef also simplifies the ForEachNode implementations there.
2861 | + return res; 2862 | } 2863 | 2864 | +void CConnman::ForEachNode(std::function<void (CNode* pnode)> func) 2865 | +{ 2866 | + std::vector<CNode*> vNodes_copy;
These should just call ForEachNodeThen with a null post functions so this code doesn't have to be repeated so many times. Again see eeb85c63cf880bc7344413b266c3af793d5cb8d2.
187 | - (cs).unlock(); \ 188 | - LeaveCritical(); \ 189 | +#define LEAVE_CRITICAL_SECTION(cs) \ 190 | + { \ 191 | + (cs).unlock(); \ 192 | + LeaveCritical((void*)(&cs)); \
This should just say LeaveCritical(&(cs)). cs should be parenthesized since it's a macro argument, and the cast is unneeded.
utACK 50ef0b0fbd9e066286a33c44b879746a79be8e34
Superceded by #11604