net: drop custom CNode refcounting in favor of smart pointers #10738

1. 
   theuni commented at 0:00 am on July 4, 2017: member

   This is a more involved version of #10697, which instead completely gets rid of our nasty AddRef() and Release() in favor of automatic lifetime management.

   Special care must be taken, though, to only delete CNodes from a single thread, and to control which thread that is. Eventually, this should move to the message processing thread, so that it’s not possible to hit cs_main from the main network thread.

   In order to do this safely, this PR introduces 2 new generic smart pointer types: strong_ptr and decay_ptr. They provide a functionality that I’ve wanted for a long time: the ability to safely decay a shared_ptr to a unique_ptr. That sounds somewhat nonsensical at first, but it’s useful to be able to make copies of a pointer for a while, stop, wait until only one remains, then delete with guaranteed safety.

   Please read shared_ptr.h and check out the tests before groaning. I think this is a very cool (and completely safe) pattern.

   This functionality could potentially be accomplished with a shared_ptr and polling ptr.unique(), but that’s inherently racy because a new copy could be created simultaneously. Even moving to a local instance and calling .unique() on that one is not safe, as a weak_ptr could be upgraded simultaneously.

   Instead, a strong_ptr is created which acts like a unique_ptr but allows shared_ptrs to be “loaned” out. Once a strong_ptr is moved into a decay_ptr, the strong_ptr is reset and no new loans may be created. The decay_ptr tracks the lifetime of the loaned copies, and knows whether or not they have all expired. This can be queried, with no race concerns, with decay_ptr::decay().

   Additionally, if the loaned shared_ptrs for some reason outlive the strong_ptr/decay_ptr, they are safely deleted once the last loaned shared_ptr expires. So there is no risk of leaks.

   In order to make review easier, these changes were made in a few stages:

   1. Where possible, make functions agnostic to the type of pointer being used
   2. Switch to shared_ptr but keep existing refcounting on top
   3. Switch to strong_ptr
   4. Drop existing refcounting and now-unnecessary locking.
2. net: replace specific FindNodes with a generic one

   Once FindNode returns a shared_ptr, it becomes much more useful.

   e1dbd84efe
3. net: use range-based-for in a few places

   No need to mess with iterators

   e832634b1d
4. net: turn CNodes into shared_ptrs

   Note that this is an interim commit for easier review; it is strictly worse
   than bare pointers.
   
   The actual benefits will come in the next commits. The main one being that we
   can drop our clunky (and unsafe) refcounting.

   59cd96a173
5. net: add strong_ptr and decay_ptr

   See strong_ptr.h for more details.

   2d720aaa62
6. net: switch CNode to strong_ptr/decay_ptr

   It's necessary to control which thread deletes CNodes, because net_processing
   requires notification so that it may clean up its resources as well.
   
   More generally we need assurance that, when deleting a CNode, there's nothing
   else attempting to access it.
   
   shared_ptr and decay_ptr enable us to do this safely. vNodes now holds
   strong_ptrs rather than shared_ptrs, and moves them to a container of
   decay_ptrs once disconnected. At that point, we can wait until there are no
   remaining users before deletion.

   457bc99c57
7. net: remove CNode's refcounting in favor of strong_ptr 8317948f67
8. net: create GetNodesCopy and use it in a few places afd1a552d2
9. net: clean up vNodes usage in the socket handler

   - Make a single, quick copy at the top of the loop
   - Disconnect outside of the lock

   afd31d7706
10. 
    theuni commented at 0:03 am on July 4, 2017: member

    Note that #10697 (comment) applies here as well.
11. 
    theuni commented at 0:45 am on July 4, 2017: member

    Test failure looks unrelated and matches another one here: https://travis-ci.org/bitcoin/bitcoin/jobs/248773151. Kicking off a new build.
12. theuni force-pushed on Jul 4, 2017
13. fanquake added the label P2P on Jul 4, 2017
14. net: drop some cs_vNodes locks

    These are all unnecessary now that a shared_ptr is held.

    6dc671a220
15. 
    laanwj commented at 10:22 am on August 23, 2017: member

    Needs rebase.
16. laanwj assigned laanwj on Aug 23, 2017
17. fanquake added this to the "In progress" column in a project

    ---

18. in src/strong_ptr.h:51 in 6dc671a220

    46+        {
    47+            strong_ptr<int> str(make_strong<int>(100));
    48+            std::thread(func, str.get_shared());
    49+            decay_ptr<int> dec(std::move(strong));
    50+            // The original strong_ptr is now reset
    51+            while (!dec.decayed()) {
    

    ---

    ---

    

    ryanofsky commented at 7:09 pm on December 20, 2017:

    I don’t understand how make_strong/strong_ptr/decay_ptr provide any benefit in this example. Why wouldn’t you just use regular shared pointers, and write this loop as while (str.use_count() > 1)?

    ---

    

    theuni commented at 10:07 pm on December 20, 2017:

    a weak_ptr can be created from a shared_ptr without bumping its refcount. That weak_ptr can lock() in a separate thread just after checking use_count() here. shared_ptr.unique() (shared_ptr.use_count() == 1) was deprecated in c++17 for that reason.

    Once moved to a decay_ptr, decayed() is a trustworthy unique().

    ---

    

    laanwj commented at 2:19 pm on February 6, 2018:

    I don’t think this is a particularly good example, as it encourages busy waiting :-) But for the application in our net code it’s a clever solution.
19. in src/strong_ptr.h:156 in 6dc671a220

    151+    void reset()
    152+    {
    153+        m_data.reset();
    154+        m_shared.reset();
    155+    }
    156+    void reset(std::nullptr_t)
    

    ---

    ---

    

    laanwj commented at 2:26 pm on February 6, 2018:

    What is the advantage of having a specific overload for nullptr_t?

    ---

    

    theuni commented at 10:04 pm on February 9, 2018:

    Heh, I have no idea why I added that. Will remove.
20. in src/net.h:331 in e1dbd84efe outdated

    327@@ -333,6 +328,19 @@ class CConnman
    328     // Whether the node should be passed out in ForEach* callbacks
    329     static bool NodeFullyConnected(const CNode* pnode);
    330 
    331+    template <typename Callable>
    

    ---

    ---

    

    laanwj commented at 2:31 pm on February 6, 2018:

    Nice! I like how this cleans up the searching functions. Maybe add a comment (for doxygen) that this returns the first (arbitrary) node that matches a certain predicate, or nullptr otherwise.
21. in src/net.h:332 in afd1a552d2 outdated

    328@@ -329,6 +329,19 @@ class CConnman
    329     // Whether the node should be passed out in ForEach* callbacks
    330     static bool NodeFullyConnected(const CNode* pnode);
    331 
    332+    std::vector<std::shared_ptr<CNode>> GetNodesCopy() const
    

    ---

    ---

    

    laanwj commented at 2:40 pm on February 6, 2018:

    Creating a copy - increasing all refcounts in the process, just to drop them again at the end of the function - feels like a lot of overhead, how many lock operations does that take internally? Can we somehow benchmark that this is more efficient than holding cs_vnodes for the entire time? (or at least, not much slower, not all the cases are performance critical and it does clean up the code)

    ---

    

    theuni commented at 9:49 pm on February 9, 2018:

    Performance isn’t the intention, this was done in order to avoid keeping cs_vNodes locked during the ForEachNode callbacks. Though I agree and also really dislike the overhead.
22. 
    laanwj commented at 2:44 pm on February 6, 2018: member

    utACK. I was able to review everything except some of the C++11 magic in strong.h (commit 2d720aaa626d3866ee3664f1ce9c3d654811e418), but the tests for that look convincing to me. In my opinion this is a big improvement from how things are currently handled.
23. 
    jtimon commented at 3:36 pm on February 10, 2018: contributor

    The title sounds great because is not “reinventing the wheel” like our own ref_count data structure. But strong_pointer feels like reinventing the wheel again…
24. laanwj unassigned laanwj on Apr 24, 2018
25. MarcoFalke added the label Needs rebase on Jun 6, 2018
26. 
    DrahtBot commented at 11:20 pm on November 8, 2018: contributor
27. DrahtBot added the label Up for grabs on Nov 8, 2018
28. DrahtBot closed this on Nov 8, 2018

    ---

29. dongcarl moved this from the "In progress" to the "Backlog" column in a project

    ---

30. laanwj removed the label Needs rebase on Oct 24, 2019
31. bitcoin locked this on Dec 16, 2021
32. MarcoFalke removed the label Up for grabs on Aug 14, 2023

Contributors
theuni laanwj ryanofsky jtimon DrahtBot

Labels
P2P