net: drop custom CNode refcounting in favor of smart pointers #10738
Conversation
Once FindNode returns a shared_ptr, it becomes much more useful.
No need to mess with iterators
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.
See strong_ptr.h for more details.
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.
- Make a single, quick copy at the top of the loop - Disconnect outside of the lock
|
Note that #10697 (comment) applies here as well. |
|
Test failure looks unrelated and matches another one here: https://travis-ci.org/bitcoin/bitcoin/jobs/248773151. Kicking off a new build. |
These are all unnecessary now that a shared_ptr is held.
|
Needs rebase. |
| std::thread(func, str.get_shared()); | ||
| decay_ptr<int> dec(std::move(strong)); | ||
| // The original strong_ptr is now reset | ||
| while (!dec.decayed()) { |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
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.
| m_data.reset(); | ||
| m_shared.reset(); | ||
| } | ||
| void reset(std::nullptr_t) |
There was a problem hiding this comment.
What is the advantage of having a specific overload for nullptr_t?
There was a problem hiding this comment.
Heh, I have no idea why I added that. Will remove.
src/net.h
Outdated
| @@ -333,6 +328,19 @@ class CConnman | |||
| // Whether the node should be passed out in ForEach* callbacks | |||
| static bool NodeFullyConnected(const CNode* pnode); | |||
|
|
|||
| template <typename Callable> | |||
There was a problem hiding this comment.
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.
| @@ -329,6 +329,19 @@ class CConnman | |||
| // Whether the node should be passed out in ForEach* callbacks | |||
| static bool NodeFullyConnected(const CNode* pnode); | |||
|
|
|||
| std::vector<std::shared_ptr<CNode>> GetNodesCopy() const | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
|
The title sounds great because is not "reinventing the wheel" like our own ref_count data structure. |
| There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened. |
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: