refactor: Make CAddrMan::cs non-recursive #19238

It would be nice to implement that TODO comment because it would make it possible to avoid any mutex requirements from size().

I did so in https://github.com/vasild/bitcoin/commit/f25d8a7c6de0e45b037f91138e1b4a72e7a1653f. If you like it you can replace the commit refactor: Prevent double lock in CAddrMan::size() with that one. In this branch https://github.com/vasild/bitcoin/tree/200610-addrman-mx that is done and also the subsequent commits are adjusted.

Or, if you don't like the caching, then maybe just delete the TODO comment.

The commit you suggested does change CAddrMan::size() behavior. So I'd prefer to keep this PR as clean refactoring with no behavior changes.

ok

Here and in StdMutex - I find this very confusing because it would end up with m being the same as !m. It is a huge hack imposed by clang. I would suggest to add some comment to explain what is that. Something like Required by the clang thread safety instrumentation so that we can declare that a mutex should not be held when a function is called - EXCLUSIVE_LOCKS_REQUIRED(!m) (aka "negative capabilities").

The example in https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#mutex-h also has some comment.

PS I wonder what would EXCLUSIVE_LOCKS_REQUIRED(!!m) do. :eyes:

Done in #19249.

The pattern AssertLockNotHeld(m); LOCK(m); seems too verbose. Following that, should we add AssertLockNotHeld() before every LOCK() in the source code?

What about adding AssertLockNotHeld() inside LOCK()?

What happens if we try to lock a mutex which we already own? https://en.cppreference.com/w/cpp/thread/mutex/lock says:

If lock is called by a thread that already owns the mutex, the behavior is undefined

The following test case

void f(Mutex& m)
{
    LOCK(m);
}

BOOST_AUTO_TEST_CASE(double_lock)
{
    Mutex m;
    LOCK(m);
    f(m);
}

throws an exception std::__1::system_error: mutex lock failed: Resource deadlock avoided by the OS itself (from inside std::mutex::lock()). So, we don't have a protection in our code and we need one because want to avoid the undefined behavior (can't rely on the OS to always throw an exception).

The following succeeds (why not also exception?):

void f(std::mutex& m) {
    std::unique_lock<std::mutex> lock(m);
}

int main(int, char**) {
    std::mutex m;
    std::unique_lock<std::mutex> lock(m);
    f(m);
    return 0;
}

Following that, should we add AssertLockNotHeld() before every LOCK() in the source code?

Only for Mutex instances, not for RecursiveMutex ones.

What about adding AssertLockNotHeld() inside LOCK()?

Not now because the RecursiveMutex instance could be an argument of LOCK().

What about checking, inside LOCK(), whether the mutex being locked is instance of Mutex (and not RecursiveMutex) and only asserting that we don't own it in that case:

diff --git i/src/sync.h w/src/sync.h
index 60e5a87ae..36c348898 100644
--- i/src/sync.h
+++ w/src/sync.h
@@ -10,12 +10,13 @@
 #include <util/macros.h>
 
 #include <condition_variable>
 #include <mutex>
 #include <string>
 #include <thread>
+#include <type_traits>
 
 ////////////////////////////////////////////////
 //                                            //
 // THE SIMPLE DEFINITION, EXCLUDING DEBUG CODE //
 //                                            //
 ////////////////////////////////////////////////
@@ -145,12 +146,15 @@ private:
         return Base::owns_lock();
     }
 
 public:
     UniqueLock(Mutex& mutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(mutexIn) : Base(mutexIn, std::defer_lock)
     {
+        if (std::is_base_of<::Mutex, Mutex>::value) {
+            AssertLockNotHeldInternal(pszName, pszFile, nLine, &mutexIn);
+        }
         if (fTry)
             TryEnter(pszName, pszFile, nLine);
         else
             Enter(pszName, pszFile, nLine);
     }
 
diff --git i/src/test/sync_tests.cpp w/src/test/sync_tests.cpp
index 5c6c2ee38..a66d55519 100644
--- i/src/test/sync_tests.cpp
+++ w/src/test/sync_tests.cpp
@@ -46,7 +46,27 @@ BOOST_AUTO_TEST_CASE(potential_deadlock_detected)
 
     #ifdef DEBUG_LOCKORDER
     g_debug_lockorder_abort = prev;
     #endif
 }
 
+template <typename M>
+void lock(M& m)
+{
+    LOCK(m);
+}
+
+BOOST_AUTO_TEST_CASE(double_lock_mutex)
+{
+    Mutex m;
+    LOCK(m);
+    lock(m);
+}
+
+BOOST_AUTO_TEST_CASE(double_lock_recursive_mutex)
+{
+    RecursiveMutex m;
+    LOCK(m);
+    lock(m);
+}
+
 BOOST_AUTO_TEST_SUITE_END()

It works - the test double_lock_mutex fails because of the newly added check while the double_lock_recursive_mutex succeeds.

The double colon in ::Mutex is required because the class template type template <typename Mutex, ...> UniqueLock ... shadows the global typedef ... Mutex;.

@vasild Mind submitting a PR that it could be reviewed before this one?

Currently, it is assumed the following patterns:

• for public interface methods:

# foo.h
class Foo
{
public: void foo() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
}
# foo.cpp
void Foo::foo()
{
    AssertLockNotHeld(m_mutex);
    LOCK(m_mutex);
    // do magic
}

• for internal methods that are called from critical sections:

# foo.h
class Foo
{
pivate: void foo_cs() EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
}
# foo.cpp
void Foo::foo_cs()
{
    AssertLockHeld(m_mutex);
    // do magic
}

The common places are thread safety annotations and lock assertions. It seems placing AssertLockNotHeld() into internals of the LOCK() breaks uniformity, no?

Mind submitting a PR that it could be reviewed before this one?

Yes, I think that deserves a separate PR. I have to consider this a little bit before opening a PR. Maybe it would be possible to optimize it - the patch above would lock lockdata.dd_mutex and dig into the stack of locks for the current thread 2 times - once from the newly added AssertLockNotHeld() and a second time from push_lock(). It should be possible to do everything in push_lock() and avoid adding the call to AssertLockNotHeld().

...breaks uniformity, no?

I don't see it that way - LOCK() already implies that we don't own the mutex.

Here is a PR that would detect a double lock. With the check from inside LOCK() it makes unnecessary to add AssertLockNotHeld() calls before each LOCK(): https://github.com/bitcoin/bitcoin/pull/19337

Can you consolidate all the private members and protected members to be next to each other? Multiple private and protected access specifiers make this harder to read than is necessary.

Yeah, class declaration is easier to read if there is just one instance of public:, protected: and private: (in that order).

@jnewbery @vasild

Done in #22025.

I don't think we need to expose this as a public method just for the addrman tests. Just release the lock in that test before calling Attempt():

--- a/src/test/addrman_tests.cpp
+++ b/src/test/addrman_tests.cpp
@@ -73,13 +73,12 @@ public:
     // Simulates connection failure so that we can test eviction of offline nodes
     void SimConnFail(const CService& addr)
     {
-         LOCK(cs);
-         int64_t nLastSuccess = 1;
-         Good_(addr, true, nLastSuccess); // Set last good connection in the deep past.
+         // Set last good connection in the deep past.
+         WITH_LOCK(cs, {Good_(addr, true, /* nLastSuccess= */ 1);});
 
          bool count_failure = false;
          int64_t nLastTry = GetAdjustedTime()-61;
-         AttemptWithLockHeld(addr, count_failure, nLastTry);
+         Attempt(addr, count_failure, nLastTry);

The unit test is single threaded, so there's no need to hold the mutex between Good_() and Attempt().

Thanks! Done in bc3694a49a93b9ae6f8cacfd0d5b1def5f09b004.

Make this (and other Check() functions) const?

This change is not trivial due to the non-const std::map::operator[]:

addrman.cpp:433:20: error: no viable overloaded operator[] for type 'const std::map<CNetAddr, int>'
        if (mapAddr[info] != n)
            ~~~~~~~^~~~~

It'd be good to get rid of the Clear() functions entirely:

• the call to Clear() in deserialize could be replaced with an assert that size() is 0 (we should never deserialize into a populated CAddrman)
• the calls in the tests can be replaced by instantiating new CAddrmans. Clearing then reusing an existing CAddrman is not a realistic usage pattern.

If you don't want to do that in this PR, it feels like this function should be called Clear_() and be defined in the cpp file, for consistency with the other functions.

@jnewbery

It'd be good to get rid of the Clear() functions entirely:

I totally agree with you.

• the call to Clear() in deserialize could be replaced with an assert that size() is 0 (we should never deserialize into a populated CAddrman)

Going to implement this suggestion.

• the calls in the tests can be replaced by instantiating new CAddrmans. Clearing then reusing an existing CAddrman is not a realistic usage pattern.

This part is not trivial, and probably deserves its own pr.

Thanks! Updated.

It feels like this is a good opportunity to make the access patterns consistent here:

@@ -689,13 +689,10 @@ public:
     //! Randomly select an address in tried that another address is attempting to evict.
     CAddrInfo SelectTriedCollision()
     {
-        CAddrInfo ret;
-        {
-            LOCK(cs);
-            CheckWithLockHeld();
-            ret = SelectTriedCollision_();
-            CheckWithLockHeld();
-        }
+        LOCK(cs);
+        CheckWithLockHeld();
+        CAddrInfo ret = SelectTriedCollision_();
+        CheckWithLockHeld();
         return ret;
     }
 
@@ -704,26 +701,21 @@ public:
      */
     CAddrInfo Select(bool newOnly = false)
     {
-        CAddrInfo addrRet;
-        {
-            LOCK(cs);
-            CheckWithLockHeld();
-            addrRet = Select_(newOnly);
-            CheckWithLockHeld();
-        }
+        LOCK(cs);
+        CheckWithLockHeld();
+        CAddrInfo addrRet = Select_(newOnly);
+        CheckWithLockHeld();
         return addrRet;
     }
 
     //! Return a bunch of addresses, selected at random.
     std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct)
     {
-        Check();
+        LOCK(cs);
+        CheckWithLockHeld();
         std::vector<CAddress> vAddr;
-        {
-            LOCK(cs);
-            GetAddr_(vAddr, max_addresses, max_pct);
-        }
-        Check();
+        GetAddr_(vAddr, max_addresses, max_pct);
+        CheckWithLockHeld();
         return vAddr;
     }

(i.e. for all of these functions take the lock, run Check(), run the inner function, run Check() again, release lock/return)

:+1: This would make the Check() function unused, so it can be removed.

Thanks! Done in e21c0dd57009197dfeedbc4ed4d30454b7a05cf6.

I was expecting to see EXCLUSIVE_LOCKS_REQUIRED(!cs) added to these functions that lock cs to get compile time detection of double-locking. Any reason not to have them?

Of course! Done in 7f01942d2d3d9d66a8932d6a1fe08281d9b52273.

It is a matter of taste. Personally, I find the "classic" easier to read:

const int err = Check_();
if (err != 0) {

It is also more gdb-friendly because one can set breakpoint on the first or on the second line.

Thanks! Updated.