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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• refactor: Make CCheckQueue RAII-styled #18731 (refactor: Make CCheckQueue RAII-styled by hebasto)
• Add local thread pool to CCheckQueue #18710 (Add local thread pool to CCheckQueue by hebasto)
• Relog configuration args on debug.log rotation #16673 (Relog configuration args on debug.log rotation by LarryRuane)
• Wallet passive startup #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.

Strong concept ACK: thanks for fixing this!

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

Can you explain why?

@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.

How is this different from using Mutex and Lock?

E.g.

Mutex m;
LOCK(m);

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

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.

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?

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.

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.

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

Rebased for the ToString changes

Renamed MutexGuard to LockGuard, and dropped util_threadnames_tests changes.

ACK 5478d6c 🗾

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 5478d6c099e76fe070703cc5383cba7b91468b0f 🗾
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiM+Qv/bctxRqxjmwEq2xf4FSS7ej1ao6ni8G8tnk9+sB1QniYItq56EOxEe99u
lEB6lt7QFL2z9fD/mH6gEp5mdM0yqt16z35+MGxxVYNrBX0Lm/8lquFqHlPA5DNF
mfUp7Ro82ixyc65ZlMakjkgY5lr1rrNDPB9XWpw5jKFTvc5b2Dv+n7FxshM77BXd
1U8tgAOviw8etuhIWDSoTSVj85ubx9ld0cualiKg55prHi1gyH7zEQrOTJ4TYiS7
R8vPpnvK/nKpnbUq/jbXXT9DgV+3O0nKfSA+uipzsG0417ON83ugewko8oOQaP19
NtJWThsr1Eijk/YiPpdSpdNX2s0xqcvRTGaxEFU3Vwh80x4p3If2pPI96izs5Ye5
HyEGjZDtZRfco27eUp+YHPM8pk8Fg5pAJqdTQLfkGR3FTgR+r6PuXJ2GkSkXqHKv
Plx/qOYuSNlDJMd1dyCCeW6GiEpNh2XuszgldLIB8pjSCaq8qjSJtep+wmEIdsh/
ueOKDaHz
=Ww4X
-----END PGP SIGNATURE-----