The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
LOCKS_EXCLUDED isn’t widely used in the code base is that it doesn’t propagate up the call stack, so has a rather limited use case. Though, the new syntax claims to do that.
557+ size_t size() const EXCLUSIVE_LOCKS_REQUIRED(!m_addrman_mutex)
558 {
559- LOCK(cs); // TODO: Cache this in an atomic to avoid this overhead
560- return vRandom.size();
561+ LOCK(m_addrman_mutex); // TODO: Cache this in an atomic to avoid this overhead
562+ return sizeNonLockerHelper();
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.
102@@ -103,6 +103,9 @@ class LOCKABLE AnnotatedMixin : public PARENT
103 }
104
105 using UniqueLock = std::unique_lock<PARENT>;
106+#ifdef __clang__
107+ const AnnotatedMixin& operator!() const { return *this; }
108+#endif // __clang__
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:
Looks good, some suggestions below.
The last two commits:
0Add means to handle negative capabilities in thread safety annotations
1refactor: Add negative thread safety annotations to CAddrMan
are somewhat unrelated and maybe deserve a separate PR if they cause some controversy or if another reviewer asks to move them away to ease review. Since I already reviewed everything I am fine with them as they are.
Updated d9572933f1fb0201b259d9cb7b40873bdbad7b6f -> b6712ece8ada3b1c9230f1264da81cbf11fe4595 (pr19238.02 -> pr19238.04):
Updated b6712ece8ada3b1c9230f1264da81cbf11fe4595 -> 9cd0ed858c96b1052580e2044069c5cb9434628e (pr19238.04 -> pr19238.05):
659+ void ResolveCollisions() EXCLUSIVE_LOCKS_REQUIRED(!m_addrman_mutex)
660 {
661- LOCK(cs);
662- Check();
663+ AssertLockNotHeld(m_addrman_mutex);
664+ LOCK(m_addrman_mutex);
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
0void f(Mutex& m)
1{
2 LOCK(m);
3}
4
5BOOST_AUTO_TEST_CASE(double_lock)
6{
7 Mutex m;
8 LOCK(m);
9 f(m);
10}
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?):
0void f(std::mutex& m) {
1 std::unique_lock<std::mutex> lock(m);
2}
3
4int main(int, char**) {
5 std::mutex m;
6 std::unique_lock<std::mutex> lock(m);
7 f(m);
8 return 0;
9}
Following that, should we add
AssertLockNotHeld()before everyLOCK()in the source code?
Only for Mutex instances, not for RecursiveMutex ones.
What about adding
AssertLockNotHeld()insideLOCK()?
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:
0diff --git i/src/sync.h w/src/sync.h
1index 60e5a87ae..36c348898 100644
2--- i/src/sync.h
3+++ w/src/sync.h
4@@ -10,12 +10,13 @@
5 #include <util/macros.h>
6
7 #include <condition_variable>
8 #include <mutex>
9 #include <string>
10 #include <thread>
11+#include <type_traits>
12
13 ////////////////////////////////////////////////
14 // //
15 // THE SIMPLE DEFINITION, EXCLUDING DEBUG CODE //
16 // //
17 ////////////////////////////////////////////////
18@@ -145,12 +146,15 @@ private:
19 return Base::owns_lock();
20 }
21
22 public:
23 UniqueLock(Mutex& mutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(mutexIn) : Base(mutexIn, std::defer_lock)
24 {
25+ if (std::is_base_of<::Mutex, Mutex>::value) {
26+ AssertLockNotHeldInternal(pszName, pszFile, nLine, &mutexIn);
27+ }
28 if (fTry)
29 TryEnter(pszName, pszFile, nLine);
30 else
31 Enter(pszName, pszFile, nLine);
32 }
33
34diff --git i/src/test/sync_tests.cpp w/src/test/sync_tests.cpp
35index 5c6c2ee38..a66d55519 100644
36--- i/src/test/sync_tests.cpp
37+++ w/src/test/sync_tests.cpp
38@@ -46,7 +46,27 @@ BOOST_AUTO_TEST_CASE(potential_deadlock_detected)
39
40 #ifdef DEBUG_LOCKORDER
41 g_debug_lockorder_abort = prev;
42 #endif
43 }
44
45+template <typename M>
46+void lock(M& m)
47+{
48+ LOCK(m);
49+}
50+
51+BOOST_AUTO_TEST_CASE(double_lock_mutex)
52+{
53+ Mutex m;
54+ LOCK(m);
55+ lock(m);
56+}
57+
58+BOOST_AUTO_TEST_CASE(double_lock_recursive_mutex)
59+{
60+ RecursiveMutex m;
61+ LOCK(m);
62+ lock(m);
63+}
64+
65 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;.
Currently, it is assumed the following patterns:
0# foo.h
1class Foo
2{
3public: void foo() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
4}
5# foo.cpp
6void Foo::foo()
7{
8 AssertLockNotHeld(m_mutex);
9 LOCK(m_mutex);
10 // do magic
11}
0# foo.h
1class Foo
2{
3pivate: void foo_cs() EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
4}
5# foo.cpp
6void Foo::foo_cs()
7{
8 AssertLockHeld(m_mutex);
9 // do magic
10}
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.
LOCK() it makes unnecessary to add AssertLockNotHeld() calls before each LOCK(): https://github.com/bitcoin/bitcoin/pull/19337
_cs suffix to the name of the function that is called from within the critical section already guarded by the locked instance of the Mutex?
btw, how about convention to add the
_cssuffix to the name of the function that is called from within the critical section already guarded by the locked instance of theMutex?
I like it, and it is shorter than ...NonLockHelper(), but what about the CamelCaseConvention? Also, it would get fuzzy if more than one mutex is involved and a method requires one to be held and the other not…
Updated 9cd0ed858c96b1052580e2044069c5cb9434628e -> d985aeeac48114f1078980888303f34ffdb78e74 (pr19238.05 -> pr19238.06):
Updated d985aeeac48114f1078980888303f34ffdb78e74 -> f1246cb12f573c4a8b32d75d6ea377eeec3b43e7 (pr19238.06 -> pr19238.07):
268 uint256 nKey;
269
270 //! Source of random numbers for randomization in inner loops
271 FastRandomContext insecure_rand;
272
273+private:
public:, protected: and private: (in that order).
296@@ -262,7 +297,7 @@ friend class CAddrManTest;
297 bool Add_(const CAddress &addr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs);
298
299 //! Mark an entry as attempted to connect.
300- void Attempt_(const CService &addr, bool fCountFailure, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs);
301+ void AttemptWithLockHeld(const CService &addr, bool fCountFailure, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs);
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():
0--- a/src/test/addrman_tests.cpp
1+++ b/src/test/addrman_tests.cpp
2@@ -73,13 +73,12 @@ public:
3 // Simulates connection failure so that we can test eviction of offline nodes
4 void SimConnFail(const CService& addr)
5 {
6- LOCK(cs);
7- int64_t nLastSuccess = 1;
8- Good_(addr, true, nLastSuccess); // Set last good connection in the deep past.
9+ // Set last good connection in the deep past.
10+ WITH_LOCK(cs, {Good_(addr, true, /* nLastSuccess= */ 1);});
11
12 bool count_failure = false;
13 int64_t nLastTry = GetAdjustedTime()-61;
14- AttemptWithLockHeld(addr, count_failure, nLastTry);
15+ Attempt(addr, count_failure, nLastTry);
The unit test is single threaded, so there’s no need to hold the mutex between Good_() and Attempt().
228@@ -229,13 +229,48 @@ friend class CAddrManTest;
229 //! Holds addrs inserted into tried table that collide with existing entries. Test-before-evict discipline used to resolve these collisions.
230 std::set<int> m_tried_collisions;
231
232+ void CheckWithLockHeld() EXCLUSIVE_LOCKS_REQUIRED(cs)
Check() functions) const?
This change is not trivial due to the non-const std::map::operator[]:
0addrman.cpp:433:20: error: no viable overloaded operator[] for type 'const std::map<CNetAddr, int>'
1 if (mapAddr[info] != n)
2 ~~~~~~~^~~~~
237+ LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err);
238+ }
239+#endif // DEBUG_ADDRMAN
240+ }
241+
242+ void ClearWithLockHeld() EXCLUSIVE_LOCKS_REQUIRED(cs)
It’d be good to get rid of the Clear() functions entirely:
Clear() in deserialize could be replaced with an assert that size() is 0 (we should never deserialize into a populated CAddrman)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.
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 populatedCAddrman)
Going to implement this suggestion.
- the calls in the tests can be replaced by instantiating new
CAddrmans. Clearing then reusing an existingCAddrmanis not a realistic usage pattern.
This part is not trivial, and probably deserves its own pr.
691@@ -679,9 +692,9 @@ friend class CAddrManTest;
692 CAddrInfo ret;
It feels like this is a good opportunity to make the access patterns consistent here:
0@@ -689,13 +689,10 @@ public:
1 //! Randomly select an address in tried that another address is attempting to evict.
2 CAddrInfo SelectTriedCollision()
3 {
4- CAddrInfo ret;
5- {
6- LOCK(cs);
7- CheckWithLockHeld();
8- ret = SelectTriedCollision_();
9- CheckWithLockHeld();
10- }
11+ LOCK(cs);
12+ CheckWithLockHeld();
13+ CAddrInfo ret = SelectTriedCollision_();
14+ CheckWithLockHeld();
15 return ret;
16 }
17
18@@ -704,26 +701,21 @@ public:
19 */
20 CAddrInfo Select(bool newOnly = false)
21 {
22- CAddrInfo addrRet;
23- {
24- LOCK(cs);
25- CheckWithLockHeld();
26- addrRet = Select_(newOnly);
27- CheckWithLockHeld();
28- }
29+ LOCK(cs);
30+ CheckWithLockHeld();
31+ CAddrInfo addrRet = Select_(newOnly);
32+ CheckWithLockHeld();
33 return addrRet;
34 }
35
36 //! Return a bunch of addresses, selected at random.
37 std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct)
38 {
39- Check();
40+ LOCK(cs);
41+ CheckWithLockHeld();
42 std::vector<CAddress> vAddr;
43- {
44- LOCK(cs);
45- GetAddr_(vAddr, max_addresses, max_pct);
46- }
47- Check();
48+ GetAddr_(vAddr, max_addresses, max_pct);
49+ CheckWithLockHeld();
50 return vAddr;
51 }
(i.e. for all of these functions take the lock, run Check(), run the inner function, run Check() again, release lock/return)
Check() function unused, so it can be removed.
CAddrMan::Check() is basically useless, since it’s enabled by a compile-time option which I expect nobody uses. That PR would make it a runtime check, so that we could enable it for the unit, functional and fuzz tests. It’s obviously much more likely to catch bugs if it’s being run by those tests!
@hebasto Whilst you’re looking at this code, do you mind taking a look at #20233. Without that PR,
CAddrMan::Check()is basically useless, since it’s enabled by a compile-time option which I expect nobody uses. That PR would make it a runtime check, so that we could enable it for the unit, functional and fuzz tests. It’s obviously much more likely to catch bugs if it’s being run by those tests!
Sure!
624@@ -608,23 +625,19 @@ friend class CAddrManTest;
625 void Check()
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?
Thank you for your reviews!
Updated 0c0fb742ad7b686209efb627a8ec43d7f96c3754 -> 148a7c9454adc3e311867f3566e01d261b362f71 (pr19238.08 -> pr19238.09):
722+ void Check()
723+ EXCLUSIVE_LOCKS_REQUIRED(cs)
724+ {
725+#ifdef DEBUG_ADDRMAN
726+ AssertLockHeld(cs);
727+ if (const int err = Check_(); err != 0) {
It is a matter of taste. Personally, I find the “classic” easier to read:
0const int err = Check_();
1if (err != 0) {
It is also more gdb-friendly because one can set breakpoint on the first or on the second line.
0test/fuzz/addrman.cpp:110:30: error: 'Check' is a private member of 'CAddrMan'
1 (void)/*const_*/addr_man.Check();
2 ^
3./addrman.h:722:10: note: declared private here
4 void Check()
5 ^
61 error generated.
The unit test is single threaded, so there's no need to hold the mutex
between Good() and Attempt().
This change avoids recursive locking in the CAddrMan::Attempt function.
Co-authored-by: John Newbery <john@johnnewbery.com>
Change in the addrman.h header is move-only.
This change improves readability, and follows Developer Notes.
Co-authored-by: John Newbery <john@johnnewbery.com>
Co-authored-by: John Newbery <john@johnnewbery.com>
Rebased af9550561d7fa8cfa8da910db3e46993f848a0f5 -> ae98aec9c0521cdcec76459c8200bd45ff6a1485 (pr19238.12 -> pr19238.13) due to the silent merge conflict with #21941.