More thread safety annotation coverage

ajtowns commented at 2:04 am on May 31, 2019: member

In a few cases we need to use std::mutex rather than the sync.h primitives. But std::lock_guard<std::mutex> doesn’t include the clang thread safety annotations unless you also use clang’s C library, which means you can’t indicate when variables should be guarded by std::mutex mutexes.

This adds an annotated version of std::lock_guard<std::mutex> to threadsafety.h to fix that, and modifies places where std::mutex is used to take advantage of the annotations.

It’s based on top of #16112, and turns the thread safety comments included there into annotations.

It also changes the RAII classes in wallet/wallet.h and rpc/blockchain.cpp to just use the atomic flag for synchronisation rather than having a mutex that doesn’t actually guard anything as well.

fanquake added the label Refactoring on May 31, 2019

fanquake added the label Utils/log/libs on May 31, 2019

DrahtBot commented at 5:21 am on May 31, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18731 (refactor: Make CCheckQueue RAII-styled by hebasto)
#18710 (Add local thread pool to CCheckQueue by hebasto)
#16673 (Relog configuration args on debug.log rotation by LarryRuane)
#15719 (Drop Chain::requestMempoolTransactions method by ryanofsky)

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.

practicalswift commented at 6:59 am on May 31, 2019: contributor

Strong concept ACK: thanks for fixing this!

promag commented at 7:07 am on May 31, 2019: member

In a few cases we need to use std::mutex rather than the sync.h primitives.

Can you explain why?

sipa commented at 10:31 am on May 31, 2019: member

@promag Our “default” mutexes are recursive mutexes (which you can enter multiple times from the same thread). They’re easier to work with as you don’t need to worry about whether to call a lock-grabbing function from either inside or outside the lock. However, recursive mutexes can’t be used for condition variable waiting, so for those, we use std::mutex (which is not recursive) directly.

Longer term we should try to get rid of recursive mutexes entirely; code is much cleaner without them.

MarcoFalke commented at 2:42 pm on May 31, 2019: member

How is this different from using Mutex and Lock?

E.g.

0Mutex m;
1LOCK(m);

ajtowns force-pushed on Jun 1, 2019

promag commented at 7:36 pm on June 2, 2019: member

@sipa thanks, I was aware of that. I wasn’t sure why “In a few cases we need to use std::mutex”.

ajtowns commented at 1:22 am on June 3, 2019: member

How is this different from using Mutex and Lock?

If you do Mutex cs; instead of std::mutex cs; then you can do LOCK(cs); or WAIT_LOCK(cs, g); instead of MutexGuard g(cs);. In that case the type of g changes from a trivial subclass of std::lock_guard to UniqueLock which is a subclass of std::unique_lock that supports DEBUG_LOCKCONTENTION in which case it uses LogPrintf to report potential deadlocks or lock contention. Even without the extra features we have, std::unique_lock is potentially higher overhead than std::lock_guard, but std::lock_guard isn’t unlockable except on destruction, so unique_lock is needed for use with condition variables.

We can’t use Mutex for locking for the logging API, because that is a circular dependency.
We use std::mutex in code that implements our specialised locking behaviour got Mutex etc.
We don’t use Mutex in support/lockedpool.h, probably because at present it is independent of bitcoin interfaces in general (this PR adds a dependency on threadsafety.h, so maybe just using Mutex would make sense at that point).
We use std::mutex instead of Mutex for protecting dir_locks in util/system.h, not sure if there’s a circular dependency there to justify it though?
We also use std::mutex in some unit tests, which I don’t think matters much.

There’s some cases where we currently use std::lock_guard<std::mutex> g(cs) where cs is a Mutex, rather than calling LOCK(cs); or WAIT_LOCK(cs, g); That saves a bit of overhead, while losing our contention/deadlock debugging info, which I’m not sure makes much sense. I’ve updated this PR to change those instances to just use LOCK(cs) instead of lock_guard.

I think that means it makes sense to:

Use Mutex and LOCK etc as the “normal” locking mechanism.
Use RecursiveMutex (aka CCriticalSection) if recursive locks are needed, or it’s old code that we haven’t worked through yet.
Use std::mutex and { MutexGuard guard(mutex); ... } for code that’s used to implement Mutex

I’ve bumped this PR to replace a couple of the lock_guard instances that referred to our Mutex/CCriticalSection classes with LOCK instead of MutexGuard.

ryanofsky commented at 12:48 pm on June 5, 2019: member

We can’t use Mutex for locking for the logging API, because that is a circular dependency.

I don’t think this is true (that we can’t allow a circular dependency), but maybe you can explain more. I think ideally, we would just use Mutex and RecursiveMutex everywhere, since they fully support LOCK macros, lock assertions, lock annotations, deadlock detection, and so on. What exactly is the need for std::mutex and MutexGuard?

ajtowns commented at 1:54 pm on June 5, 2019: member

We can’t use Mutex for locking for the logging API, because that is a circular dependency.

I don’t think this is true (that we can’t allow a circular dependency), but maybe you can explain more.

Maybe; LOCK(m); becomes a call to UniqueLock contstructor, which calls Enter() (or TryEnter()), which calls EnterCritical(), which calls push_lock() which may call potential_deadlock_detected() which then calls LogPrintf, which calls LogPrintStr which, when logging to a file, will then lock m_file_mutex. But I don’t think that actually causes a problem, because I don’t think any lock is ever taken while the m_file_mutex is held, so that shouldn’t trigger potential_deadlock_detected.

There’s also the case if you compile with -DDEBUG_LOCKCONTENTION. If two processes try to log simultaneously, the first one will lock m_file_mutex successfully, while the second one will fail at the try_lock phase of UniqueLock::Enter, which will then call PrintLockContention and hence LogPrintf, and recursively fail for the same reason until m_file_mutex is released, at which point things should clean up.

I think ideally, we would just use Mutex and RecursiveMutex everywhere, since they fully support LOCK macros, lock assertions, lock annotations, deadlock detection, and so on. What exactly is the need for std::mutex and MutexGuard?

If we can switch all the uses of std::mutex to Mutex, I don’t think there’s a need for MutexGuard.

ajtowns force-pushed on Feb 11, 2020

ajtowns commented at 7:31 am on February 11, 2020: member

Rebased; updated to only use MutexGuard for logging.h.

Remaining uses of std::mutex and lock_guard are sync.h (duh), logging.h (which is used by sync.h), support/lockedpool.h (could be changed to Mutex, but I’m not confident the overhead wouldn’t cause problems), tests/checkqueue_tests.cpp (complicated, and just a test) and leveldb.

ajtowns renamed this:
~~Add support for thread safety annotations when using std::mutex~~
More thread safety annotation coverage
on Feb 11, 2020

in src/test/checkqueue_tests.cpp:298 in 7c3979f9c5 outdated

298-    BOOST_REQUIRE(r);
299+    {
300+        LOCK(UniqueCheck::m);
301+        bool r = true;
302+        BOOST_REQUIRE_EQUAL(UniqueCheck::results.size(), COUNT);
303+        for (size_t i = 0; i < COUNT; ++i)

sipa commented at 2:23 am on March 13, 2020:

Nit: if you’re reflowing this code anyway, can you put { } around the then branch?

ajtowns commented at 8:22 am on March 13, 2020:

Done

in src/logging.h:67 in 165d3edc86 outdated

62@@ -62,9 +63,18 @@ namespace BCLog {
63     {
64     private:
65         mutable std::mutex m_cs;                   // Can not use Mutex from sync.h because in debug mode it would cause a deadlock when a potential deadlock was detected
66-        FILE* m_fileout = nullptr;                 // GUARDED_BY(m_cs)
67-        std::list<std::string> m_msgs_before_open; // GUARDED_BY(m_cs)
68-        bool m_buffering{true};                    //!< Buffer messages before logging can be started. GUARDED_BY(m_cs)
69+
70+        // so MutexGuard provides an annotated version of lock_guard for us

sipa commented at 2:27 am on March 13, 2020:

Maybe move MutexGuard to sync.h, as it’s more generally useful?

ajtowns commented at 8:22 am on March 13, 2020:

There are a bunch of places that include logging.h but not sync.h, so doing it via threadsafety.h might keep things a little more minimal? I’ve done this.

ajtowns force-pushed on Mar 13, 2020

ajtowns commented at 8:24 am on March 13, 2020: member

Rebased to master to avoid #18111 and updated with @sipa’s suggestions.

ajtowns marked this as ready for review on Mar 14, 2020

DrahtBot added the label Needs rebase on Mar 25, 2020

ajtowns force-pushed on Mar 26, 2020

ajtowns commented at 3:21 am on March 26, 2020: member

Rebased for the ToString changes

DrahtBot removed the label Needs rebase on Mar 26, 2020

DrahtBot added the label Needs rebase on Apr 17, 2020

rpc/blockchain.cpp: Remove g_utxosetscan mutex that is only protecting a single atomic variable c3cf2f5501

ajtowns force-pushed on May 6, 2020

DrahtBot removed the label Needs rebase on May 6, 2020

ajtowns commented at 11:58 am on May 6, 2020: member

Rebased

hebasto commented at 4:52 am on May 9, 2020: member

Concept ACK.

in src/rpc/blockchain.cpp:1996 in c3cf2f5501 outdated

1994             return false;
1995         }
1996-        g_scan_in_progress = true;
1997         m_could_reserve = true;
1998         return true;
1999     }

hebasto commented at 5:00 am on May 9, 2020:

c3cf2f55013c4ea1c1ef4a878fc7ff8e92f2c42d It seems this changes behavior as m_could_reserve = true; is also guarded by mutex currently.

ajtowns commented at 5:31 am on May 9, 2020:

m_could_reserve isn’t a global, and instances of CoinsViewScanReserver aren’t shared across threads either.

hebasto commented at 5:47 am on May 9, 2020:

Oh, I see that all instances of CoinsViewScanReserver are local in the scantxoutset() function.

in src/wallet/wallet.h:1289 in 960d64d326 outdated

1284@@ -1286,8 +1285,7 @@ class WalletRescanReserver
1285     bool reserve()
1286     {
1287         assert(!m_could_reserve);
1288-        std::lock_guard<std::mutex> lock(m_wallet.mutexScanning);

hebasto commented at 5:03 am on May 9, 2020:

960d64d326c501dbe22b80094171d7fe3e86bd69 It seems this changes behavior as the whole block is guarded by mutex currently.

ajtowns commented at 5:33 am on May 9, 2020:

Same reasoning as previous; except that this one sets fScanningWallet to true twice :(

hebasto commented at 8:44 am on May 16, 2020:

So why not dropping the following m_wallet.fScanningWallet = true; ?

hebasto commented at 9:29 am on May 16, 2020:

~As WalletRescanReserver.m_wallet is a reference, the object being referenced to could be shared among different threads. So not using the mutexScanning mutex here is not thread-safe, IMO.~

ajtowns commented at 7:12 am on May 19, 2020:

dropped redundant fScanningWallet = true

in src/test/util_threadnames_tests.cpp:39 in bbe87c91a8 outdated

39     auto RenameThisThread = [&](int i) {
40         util::ThreadRename(TEST_THREAD_NAME_BASE + ToString(i));
41-        std::lock_guard<std::mutex> guard(lock);
42+        LOCK(lock);
43         names.insert(util::ThreadGetInternalName());
44     };

hebasto commented at 10:46 am on May 16, 2020:

bbe87c91a8594e33542bae3d738499324cd06906

It seems nothing to capture now:

0    auto RenameThisThread = [](int i) {
1        util::ThreadRename(TEST_THREAD_NAME_BASE + ToString(i));
2        std::lock_guard<std::mutex> guard(lock);
3        LOCK(lock);
4        names.insert(util::ThreadGetInternalName());
5    };

ajtowns commented at 7:13 am on May 19, 2020:

Dropped redundant capture

hebasto approved

hebasto commented at 12:16 pm on May 16, 2020: member

ACK a05b44ba00b6b2328967d0ea85576abd6be381b7, tested on Linux Mint 19.3 and Clang 9.0 with CPPFLAGS='-DDEBUG_LOCKCONTENTION -DDEBUG_LOCKORDER'

There is the only non-blocking nit.

wallet/wallet.h: Remove mutexScanning which was only protecting a single atomic bool de7c5f41ab

net: fMsgProcWake use LOCK instead of lock_guard 8b5af3d4c1

rpc/blockchain.cpp: thread safety annotations for latestblock 479c5846f7

test/checkqueue_tests: thread safety annotations a788789948

ajtowns force-pushed on May 19, 2020

hebasto approved

hebasto commented at 7:47 am on May 19, 2020: member

re-ACK d61883d20a35a0de32c5825207d8b510a779b200, the only changes since the previous review are addressed nits:

dropped redundant capture in RenameThisThread lambda
dropped redundant fScanningWallet = true; assignment

in src/test/util_threadnames_tests.cpp:31 in d53072ec73 outdated

25@@ -26,15 +26,17 @@ const std::string TEST_THREAD_NAME_BASE = "test_thread.";
26  *
27  * @return the set of name each thread has after attempted renaming.
28  */
29+
30+static Mutex lock;
31+static std::set<std::string> names GUARDED_BY(lock);

ryanofsky commented at 3:08 pm on May 19, 2020:

In commit “test/util_threadnames_tests: add thread safety annotations” (d53072ec730d8eec5a5b72f7e65a54b141e62b19)

Assuming the reason these need to be global is lock annotations not working on local variables (didn’t check), better IMO to simply not to use lock annotations here than to introduce global variables and make it unsafe to call RenameEnMasse from different threads itself.

Other option would be to make RenameEnMasse a member function in a class with Mutex and std::set members so it could work non-globally and also be annotated

ajtowns commented at 3:47 pm on May 26, 2020:

It’s not unsafe to call RenameEnMasse from different threads with the global lock, just inefficient. I tried the class approach but it seemed like overkill, so I’ve dropped the commit instead.

ryanofsky approved

ryanofsky commented at 3:15 pm on May 19, 2020: member

Code review ACK d61883d20a35a0de32c5825207d8b510a779b200. Would call MutexGuard class LockGuard to make it an obvious replacement for std::lock_guard and use standard names for things where Mutex is the thing being locked, and Lock is the object locking and unlocking it

util/system.cpp: add thread safety annotations for dir_locks e685ca1992

logging: thread safety annotations

Adds LockGuard helper in threadsafety.h to replace lock_guard<mutex>
when LOCK(Mutex) isn't available for use.

5478d6c099

ajtowns force-pushed on May 26, 2020

ajtowns commented at 3:48 pm on May 26, 2020: member

Renamed MutexGuard to LockGuard, and dropped util_threadnames_tests changes.

hebasto approved

hebasto commented at 4:01 pm on May 26, 2020: member

re-ACK 5478d6c099e76fe070703cc5383cba7b91468b0f, only renamed s/MutexGuard/LockGuard/, and dropped the commit “test/util_threadnames_tests: add thread safety annotations” since the previous review.

ryanofsky approved

ryanofsky commented at 7:04 pm on May 27, 2020: member

Code review ACK 5478d6c099e76fe070703cc5383cba7b91468b0f. Thanks for taking suggestions! Only changes since last review are dropping thread rename test commit d53072ec730d8eec5a5b72f7e65a54b141e62b19 and renaming mutex guard to lock guard

MarcoFalke commented at 11:29 pm on May 27, 2020: member

ACK 5478d6c099e76fe070703cc5383cba7b91468b0f 🗾

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 5478d6c099e76fe070703cc5383cba7b91468b0f 🗾
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiM+Qv/bctxRqxjmwEq2xf4FSS7ej1ao6ni8G8tnk9+sB1QniYItq56EOxEe99u
 8lEB6lt7QFL2z9fD/mH6gEp5mdM0yqt16z35+MGxxVYNrBX0Lm/8lquFqHlPA5DNF
 9mfUp7Ro82ixyc65ZlMakjkgY5lr1rrNDPB9XWpw5jKFTvc5b2Dv+n7FxshM77BXd
101U8tgAOviw8etuhIWDSoTSVj85ubx9ld0cualiKg55prHi1gyH7zEQrOTJ4TYiS7
11R8vPpnvK/nKpnbUq/jbXXT9DgV+3O0nKfSA+uipzsG0417ON83ugewko8oOQaP19
12NtJWThsr1Eijk/YiPpdSpdNX2s0xqcvRTGaxEFU3Vwh80x4p3If2pPI96izs5Ye5
13HyEGjZDtZRfco27eUp+YHPM8pk8Fg5pAJqdTQLfkGR3FTgR+r6PuXJ2GkSkXqHKv
14Plx/qOYuSNlDJMd1dyCCeW6GiEpNh2XuszgldLIB8pjSCaq8qjSJtep+wmEIdsh/
15ueOKDaHz
16=Ww4X
17-----END PGP SIGNATURE-----

Timestamp of file with hash 0b593eb159203e57d289eb41d38e7938cdd641ec1ddca7c9967f9f58ff7986de -

MarcoFalke merged this on May 27, 2020

MarcoFalke closed this on May 27, 2020

sidhujag referenced this in commit 7924b5fb3e on May 28, 2020

deadalnix referenced this in commit ae1069d37c on Sep 23, 2020

kittywhiskers referenced this in commit 189a242154 on Jun 5, 2021

kittywhiskers referenced this in commit 92e9901689 on Jun 6, 2021

kittywhiskers referenced this in commit 7423caae30 on Jun 6, 2021

kittywhiskers referenced this in commit b5d7be3672 on Jun 6, 2021

UdjinM6 referenced this in commit 733337e96f on Jun 6, 2021

UdjinM6 referenced this in commit 40837387ec on Jun 6, 2021

kittywhiskers referenced this in commit 19286be77b on Jun 7, 2021

kittywhiskers referenced this in commit aacc7b8762 on Jun 8, 2021

kittywhiskers referenced this in commit d559983eeb on Jun 9, 2021

UdjinM6 referenced this in commit a8aee57447 on Jun 11, 2021

DrahtBot locked this on Feb 15, 2022

More thread safety annotation coverage #16127

Conflicts