diff options
author | Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> | 2020-08-08 12:56:28 +0300 |
---|---|---|
committer | Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> | 2020-08-29 20:46:47 +0300 |
commit | ea74e10acf17903e44c85e3678853414653dd4e1 (patch) | |
tree | 464bc6f9a473e999aaf6381c8d46a32b57fd503b | |
parent | 2ee7743fe723227f2ea1b031eddb14fc6863f4c8 (diff) | |
download | bitcoin-ea74e10acf17903e44c85e3678853414653dd4e1.tar.xz |
doc: Add best practice for annotating/asserting locks
-rw-r--r-- | doc/developer-notes.md | 66 |
1 files changed, 66 insertions, 0 deletions
diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 6ae7e770e8..ef9ecbb085 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -746,6 +746,72 @@ the upper cycle, etc. Threads and synchronization ---------------------------- +- Prefer `Mutex` type to `RecursiveMutex` one + +- Consistently use [Clang Thread Safety Analysis](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html) annotations to + get compile-time warnings about potential race conditions in code. Combine annotations in function declarations with + run-time asserts in function definitions: + +```C++ +// txmempool.h +class CTxMemPool +{ +public: + ... + mutable RecursiveMutex cs; + ... + void UpdateTransactionsFromBlock(...) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, cs); + ... +} + +// txmempool.cpp +void CTxMemPool::UpdateTransactionsFromBlock(...) +{ + AssertLockHeld(::cs_main); + AssertLockHeld(cs); + ... +} +``` + +```C++ +// validation.h +class ChainstateManager +{ +public: + ... + bool ProcessNewBlock(...) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main); + ... +} + +// validation.cpp +bool ChainstateManager::ProcessNewBlock(...) +{ + AssertLockNotHeld(::cs_main); + ... + LOCK(::cs_main); + ... +} +``` + +- When Clang Thread Safety Analysis is unable to determine if a mutex is locked, use `LockAssertion` class instances: + +```C++ +// net_processing.h +void RelayTransaction(...) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + +// net_processing.cpp +void RelayTransaction(...) +{ + AssertLockHeld(::cs_main); + + connman.ForEachNode([&txid, &wtxid](CNode* pnode) { + LockAssertion lock(::cs_main); + ... + }); +} + +``` + - Build and run tests with `-DDEBUG_LOCKORDER` to verify that no potential deadlocks are introduced. As of 0.12, this is defined by default when configuring with `--enable-debug`. |