Skip to content

Commit ea74e10

Browse files
committed
doc: Add best practice for annotating/asserting locks
1 parent 2ee7743 commit ea74e10

File tree

1 file changed

+66
-0
lines changed

1 file changed

+66
-0
lines changed

doc/developer-notes.md

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -746,6 +746,72 @@ the upper cycle, etc.
746746
Threads and synchronization
747747
----------------------------
748748
749+
- Prefer `Mutex` type to `RecursiveMutex` one
750+
751+
- Consistently use [Clang Thread Safety Analysis](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html) annotations to
752+
get compile-time warnings about potential race conditions in code. Combine annotations in function declarations with
753+
run-time asserts in function definitions:
754+
755+
```C++
756+
// txmempool.h
757+
class CTxMemPool
758+
{
759+
public:
760+
...
761+
mutable RecursiveMutex cs;
762+
...
763+
void UpdateTransactionsFromBlock(...) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, cs);
764+
...
765+
}
766+
767+
// txmempool.cpp
768+
void CTxMemPool::UpdateTransactionsFromBlock(...)
769+
{
770+
AssertLockHeld(::cs_main);
771+
AssertLockHeld(cs);
772+
...
773+
}
774+
```
775+
776+
```C++
777+
// validation.h
778+
class ChainstateManager
779+
{
780+
public:
781+
...
782+
bool ProcessNewBlock(...) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main);
783+
...
784+
}
785+
786+
// validation.cpp
787+
bool ChainstateManager::ProcessNewBlock(...)
788+
{
789+
AssertLockNotHeld(::cs_main);
790+
...
791+
LOCK(::cs_main);
792+
...
793+
}
794+
```
795+
796+
- When Clang Thread Safety Analysis is unable to determine if a mutex is locked, use `LockAssertion` class instances:
797+
798+
```C++
799+
// net_processing.h
800+
void RelayTransaction(...) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
801+
802+
// net_processing.cpp
803+
void RelayTransaction(...)
804+
{
805+
AssertLockHeld(::cs_main);
806+
807+
connman.ForEachNode([&txid, &wtxid](CNode* pnode) {
808+
LockAssertion lock(::cs_main);
809+
...
810+
});
811+
}
812+
813+
```
814+
749815
- Build and run tests with `-DDEBUG_LOCKORDER` to verify that no potential
750816
deadlocks are introduced. As of 0.12, this is defined by default when
751817
configuring with `--enable-debug`.

0 commit comments

Comments
 (0)