Do not hold cs_vNodes when making ForEachNode Callbacks #10697

1. 
   TheBlueMatt commented at 9:19 pm on June 28, 2017: member

   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.

2. 
   gmaxwell commented at 10:37 pm on June 28, 2017: contributor

   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.
3. 
   TheBlueMatt commented at 10:48 pm on June 28, 2017: member

   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.
4. fanquake added the label P2P on Jun 29, 2017
5. fanquake added the label Refactoring on Jun 29, 2017
6. 
   theuni commented at 2:51 am on June 29, 2017: member

   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.
7. 
   promag commented at 8:27 am on October 28, 2017: member

   Ping @theuni @TheBlueMatt, are you working on this?
8. in src/net.cpp:2866 in e4f477b6b0 outdated

   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);
   

   ---

   ---

   

   sipa commented at 9:26 am on October 28, 2017:

   This still calls func while cs_vNodes is held. Is that intentional?
9. in src/net.cpp:2883 in e4f477b6b0 outdated

   2877+            node->AddRef();
   2878+            vNodes_copy.push_back(node);
   2879+        }
   2880+    }
   2881+    for (CNode* node : vNodes_copy) {
   2882+        if (NodeFullyConnected(node))
   

   ---

   ---

   

   sipa commented at 9:27 am on October 28, 2017:

   Nit: either { } around if body, or body on the same line (in multiple places).
10. TheBlueMatt force-pushed on Oct 31, 2017
11. 
    TheBlueMatt commented at 3:51 pm on October 31, 2017: member

    Addressed @sipa’s comments.
12. TheBlueMatt force-pushed on Oct 31, 2017
13. Dont let ForEachNode hold cs_vNodes while making callbacks cd6d27c944
14. Improve LeaveCritical strictness 50ef0b0fbd
15. TheBlueMatt force-pushed on Oct 31, 2017
16. in src/net.cpp:2852 in cd6d27c944 outdated

    2853         }
    2854     }
    2855-    return found != nullptr && NodeFullyConnected(found) && func(found);
    2856+    bool res = false;
    2857+    if (found != nullptr) {
    2858+        res = NodeFullyConnected(found) && func(found);
    

    ---

    ---

    

    ryanofsky commented at 4:13 pm on November 3, 2017:

    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.
17. in src/net.cpp:2860 in cd6d27c944 outdated

    2861+    return res;
    2862 }
    2863 
    2864+void CConnman::ForEachNode(std::function<void (CNode* pnode)> func)
    2865+{
    2866+    std::vector<CNode*> vNodes_copy;
    

    ---

    ---

    

    ryanofsky commented at 4:14 pm on November 3, 2017:

    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.
18. in src/sync.h:188 in 50ef0b0fbd

    187-        (cs).unlock();             \
    188-        LeaveCritical();           \
    189+#define LEAVE_CRITICAL_SECTION(cs)   \
    190+    {                                \
    191+        (cs).unlock();               \
    192+        LeaveCritical((void*)(&cs)); \
    

    ---

    ---

    

    ryanofsky commented at 4:18 pm on November 3, 2017:

    This should just say LeaveCritical(&(cs)). cs should be parenthesized since it’s a macro argument, and the cast is unneeded.
19. 
    ryanofsky commented at 4:23 pm on November 3, 2017: member

    utACK 50ef0b0fbd9e066286a33c44b879746a79be8e34
20. 
    TheBlueMatt commented at 0:31 am on November 4, 2017: member

    Superceded by #11604
21. TheBlueMatt closed this on Nov 4, 2017

    ---

22. DrahtBot locked this on Sep 8, 2021

Contributors
TheBlueMatt gmaxwell theuni promag sipa ryanofsky

Labels
Refactoring P2P