Skip to content

Commit 385ad11

Browse files
committed
Merge #11640: Make LOCK, LOCK2, TRY_LOCK work with CWaitableCriticalSection
9c4dc59 Use LOCK macros for non-recursive locks (Russell Yanofsky) 1382913 Make LOCK, LOCK2, TRY_LOCK work with CWaitableCriticalSection (Russell Yanofsky) ba1f095 MOVEONLY Move AnnotatedMixin declaration (Russell Yanofsky) 41b88e9 Add unit test for DEBUG_LOCKORDER code (Russell Yanofsky) Pull request description: Make LOCK macros work with non-recursive mutexes, and use wherever possible for better deadlock detection. Also add unit test for DEBUG_LOCKORDER code. Tree-SHA512: 64ef209307f28ecd0813a283f15c6406138c6ffe7f6cbbd084161044db60e2c099a7d0d2edcd1c5e7770a115e9b931b486e86c9a777bdc96d2e8a9f4dc192942
2 parents b012bbe + 9c4dc59 commit 385ad11

14 files changed

+143
-70
lines changed

src/Makefile.test.include

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ BITCOIN_TESTS =\
8383
test/sigopcount_tests.cpp \
8484
test/skiplist_tests.cpp \
8585
test/streams_tests.cpp \
86+
test/sync_tests.cpp \
8687
test/timedata_tests.cpp \
8788
test/torcontrol_tests.cpp \
8889
test/transaction_tests.cpp \

src/httpserver.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ class WorkQueue
6969
{
7070
private:
7171
/** Mutex protects entire object */
72-
std::mutex cs;
72+
CWaitableCriticalSection cs;
7373
std::condition_variable cond;
7474
std::deque<std::unique_ptr<WorkItem>> queue;
7575
bool running;
@@ -88,7 +88,7 @@ class WorkQueue
8888
/** Enqueue a work item */
8989
bool Enqueue(WorkItem* item)
9090
{
91-
std::unique_lock<std::mutex> lock(cs);
91+
LOCK(cs);
9292
if (queue.size() >= maxDepth) {
9393
return false;
9494
}
@@ -102,7 +102,7 @@ class WorkQueue
102102
while (true) {
103103
std::unique_ptr<WorkItem> i;
104104
{
105-
std::unique_lock<std::mutex> lock(cs);
105+
WAIT_LOCK(cs, lock);
106106
while (running && queue.empty())
107107
cond.wait(lock);
108108
if (!running)
@@ -116,7 +116,7 @@ class WorkQueue
116116
/** Interrupt and exit loops */
117117
void Interrupt()
118118
{
119-
std::unique_lock<std::mutex> lock(cs);
119+
LOCK(cs);
120120
running = false;
121121
cond.notify_all();
122122
}

src/init.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -568,7 +568,7 @@ static void BlockNotifyGenesisWait(bool, const CBlockIndex *pBlockIndex)
568568
{
569569
if (pBlockIndex != nullptr) {
570570
{
571-
WaitableLock lock_GenesisWait(cs_GenesisWait);
571+
LOCK(cs_GenesisWait);
572572
fHaveGenesis = true;
573573
}
574574
condvar_GenesisWait.notify_all();
@@ -1661,7 +1661,7 @@ bool AppInitMain()
16611661

16621662
// Wait for genesis block to be processed
16631663
{
1664-
WaitableLock lock(cs_GenesisWait);
1664+
WAIT_LOCK(cs_GenesisWait, lock);
16651665
// We previously could hang here if StartShutdown() is called prior to
16661666
// ThreadImport getting started, so instead we just wait on a timer to
16671667
// check ShutdownRequested() regularly.

src/net.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2065,7 +2065,7 @@ void CConnman::ThreadMessageHandler()
20652065
pnode->Release();
20662066
}
20672067

2068-
std::unique_lock<std::mutex> lock(mutexMsgProc);
2068+
WAIT_LOCK(mutexMsgProc, lock);
20692069
if (!fMoreWork) {
20702070
condMsgProc.wait_until(lock, std::chrono::steady_clock::now() + std::chrono::milliseconds(100), [this] { return fMsgProcWake; });
20712071
}
@@ -2347,7 +2347,7 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
23472347
flagInterruptMsgProc = false;
23482348

23492349
{
2350-
std::unique_lock<std::mutex> lock(mutexMsgProc);
2350+
LOCK(mutexMsgProc);
23512351
fMsgProcWake = false;
23522352
}
23532353

src/net.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -427,7 +427,7 @@ class CConnman
427427
bool fMsgProcWake;
428428

429429
std::condition_variable condMsgProc;
430-
std::mutex mutexMsgProc;
430+
CWaitableCriticalSection mutexMsgProc;
431431
std::atomic<bool> flagInterruptMsgProc;
432432

433433
CThreadInterrupt interruptNet;

src/random.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
#include <wincrypt.h>
1313
#endif
1414
#include <logging.h> // for LogPrint()
15+
#include <sync.h> // for WAIT_LOCK
1516
#include <utiltime.h> // for GetTime()
1617

1718
#include <stdlib.h>
@@ -295,7 +296,7 @@ void RandAddSeedSleep()
295296
}
296297

297298

298-
static std::mutex cs_rng_state;
299+
static CWaitableCriticalSection cs_rng_state;
299300
static unsigned char rng_state[32] = {0};
300301
static uint64_t rng_counter = 0;
301302

@@ -305,7 +306,7 @@ static void AddDataToRng(void* data, size_t len) {
305306
hasher.Write((const unsigned char*)data, len);
306307
unsigned char buf[64];
307308
{
308-
std::unique_lock<std::mutex> lock(cs_rng_state);
309+
WAIT_LOCK(cs_rng_state, lock);
309310
hasher.Write(rng_state, sizeof(rng_state));
310311
hasher.Write((const unsigned char*)&rng_counter, sizeof(rng_counter));
311312
++rng_counter;
@@ -337,7 +338,7 @@ void GetStrongRandBytes(unsigned char* out, int num)
337338

338339
// Combine with and update state
339340
{
340-
std::unique_lock<std::mutex> lock(cs_rng_state);
341+
WAIT_LOCK(cs_rng_state, lock);
341342
hasher.Write(rng_state, sizeof(rng_state));
342343
hasher.Write((const unsigned char*)&rng_counter, sizeof(rng_counter));
343344
++rng_counter;

src/rpc/blockchain.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ struct CUpdatedBlock
5050
int height;
5151
};
5252

53-
static std::mutex cs_blockchange;
53+
static CWaitableCriticalSection cs_blockchange;
5454
static std::condition_variable cond_blockchange;
5555
static CUpdatedBlock latestblock;
5656

@@ -225,7 +225,7 @@ static UniValue waitfornewblock(const JSONRPCRequest& request)
225225

226226
CUpdatedBlock block;
227227
{
228-
std::unique_lock<std::mutex> lock(cs_blockchange);
228+
WAIT_LOCK(cs_blockchange, lock);
229229
block = latestblock;
230230
if(timeout)
231231
cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&block]{return latestblock.height != block.height || latestblock.hash != block.hash || !IsRPCRunning(); });
@@ -267,7 +267,7 @@ static UniValue waitforblock(const JSONRPCRequest& request)
267267

268268
CUpdatedBlock block;
269269
{
270-
std::unique_lock<std::mutex> lock(cs_blockchange);
270+
WAIT_LOCK(cs_blockchange, lock);
271271
if(timeout)
272272
cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&hash]{return latestblock.hash == hash || !IsRPCRunning();});
273273
else
@@ -310,7 +310,7 @@ static UniValue waitforblockheight(const JSONRPCRequest& request)
310310

311311
CUpdatedBlock block;
312312
{
313-
std::unique_lock<std::mutex> lock(cs_blockchange);
313+
WAIT_LOCK(cs_blockchange, lock);
314314
if(timeout)
315315
cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&height]{return latestblock.height >= height || !IsRPCRunning();});
316316
else

src/rpc/mining.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -470,7 +470,7 @@ static UniValue getblocktemplate(const JSONRPCRequest& request)
470470
{
471471
checktxtime = std::chrono::steady_clock::now() + std::chrono::minutes(1);
472472

473-
WaitableLock lock(g_best_block_mutex);
473+
WAIT_LOCK(g_best_block_mutex, lock);
474474
while (g_best_block == hashWatchedChain && IsRPCRunning())
475475
{
476476
if (g_best_block_cv.wait_until(lock, checktxtime) == std::cv_status::timeout)

src/sync.cpp

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,11 @@ static void potential_deadlock_detected(const std::pair<void*, void*>& mismatch,
100100
}
101101
LogPrintf(" %s\n", i.second.ToString());
102102
}
103-
assert(false);
103+
if (g_debug_lockorder_abort) {
104+
fprintf(stderr, "Assertion failed: detected inconsistent lock order at %s:%i, details in debug log.\n", __FILE__, __LINE__);
105+
abort();
106+
}
107+
throw std::logic_error("potential deadlock detected");
104108
}
105109

106110
static void push_lock(void* c, const CLockLocation& locklocation)
@@ -189,4 +193,6 @@ void DeleteLock(void* cs)
189193
}
190194
}
191195

196+
bool g_debug_lockorder_abort = true;
197+
192198
#endif /* DEBUG_LOCKORDER */

src/sync.h

Lines changed: 57 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -46,14 +46,42 @@ LEAVE_CRITICAL_SECTION(mutex); // no RAII
4646
// //
4747
///////////////////////////////
4848

49+
#ifdef DEBUG_LOCKORDER
50+
void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false);
51+
void LeaveCritical();
52+
std::string LocksHeld();
53+
void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) ASSERT_EXCLUSIVE_LOCK(cs);
54+
void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs);
55+
void DeleteLock(void* cs);
56+
4957
/**
50-
* Template mixin that adds -Wthread-safety locking
51-
* annotations to a subset of the mutex API.
58+
* Call abort() if a potential lock order deadlock bug is detected, instead of
59+
* just logging information and throwing a logic_error. Defaults to true, and
60+
* set to false in DEBUG_LOCKORDER unit tests.
61+
*/
62+
extern bool g_debug_lockorder_abort;
63+
#else
64+
void static inline EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false) {}
65+
void static inline LeaveCritical() {}
66+
void static inline AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) ASSERT_EXCLUSIVE_LOCK(cs) {}
67+
void static inline AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) {}
68+
void static inline DeleteLock(void* cs) {}
69+
#endif
70+
#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs)
71+
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
72+
73+
/**
74+
* Template mixin that adds -Wthread-safety locking annotations and lock order
75+
* checking to a subset of the mutex API.
5276
*/
5377
template <typename PARENT>
5478
class LOCKABLE AnnotatedMixin : public PARENT
5579
{
5680
public:
81+
~AnnotatedMixin() {
82+
DeleteLock((void*)this);
83+
}
84+
5785
void lock() EXCLUSIVE_LOCK_FUNCTION()
5886
{
5987
PARENT::lock();
@@ -68,92 +96,67 @@ class LOCKABLE AnnotatedMixin : public PARENT
6896
{
6997
return PARENT::try_lock();
7098
}
71-
};
7299

73-
#ifdef DEBUG_LOCKORDER
74-
void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false);
75-
void LeaveCritical();
76-
std::string LocksHeld();
77-
void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) ASSERT_EXCLUSIVE_LOCK(cs);
78-
void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs);
79-
void DeleteLock(void* cs);
80-
#else
81-
void static inline EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false) {}
82-
void static inline LeaveCritical() {}
83-
void static inline AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) ASSERT_EXCLUSIVE_LOCK(cs) {}
84-
void static inline AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) {}
85-
void static inline DeleteLock(void* cs) {}
86-
#endif
87-
#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs)
88-
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
100+
using UniqueLock = std::unique_lock<PARENT>;
101+
};
89102

90103
/**
91104
* Wrapped mutex: supports recursive locking, but no waiting
92105
* TODO: We should move away from using the recursive lock by default.
93106
*/
94-
class CCriticalSection : public AnnotatedMixin<std::recursive_mutex>
95-
{
96-
public:
97-
~CCriticalSection() {
98-
DeleteLock((void*)this);
99-
}
100-
};
107+
typedef AnnotatedMixin<std::recursive_mutex> CCriticalSection;
101108

102109
/** Wrapped mutex: supports waiting but not recursive locking */
103110
typedef AnnotatedMixin<std::mutex> CWaitableCriticalSection;
104111

105112
/** Just a typedef for std::condition_variable, can be wrapped later if desired */
106113
typedef std::condition_variable CConditionVariable;
107114

108-
/** Just a typedef for std::unique_lock, can be wrapped later if desired */
109-
typedef std::unique_lock<std::mutex> WaitableLock;
110-
111115
#ifdef DEBUG_LOCKCONTENTION
112116
void PrintLockContention(const char* pszName, const char* pszFile, int nLine);
113117
#endif
114118

115-
/** Wrapper around std::unique_lock<CCriticalSection> */
116-
class SCOPED_LOCKABLE CCriticalBlock
119+
/** Wrapper around std::unique_lock style lock for Mutex. */
120+
template <typename Mutex, typename Base = typename Mutex::UniqueLock>
121+
class SCOPED_LOCKABLE CCriticalBlock : public Base
117122
{
118123
private:
119-
std::unique_lock<CCriticalSection> lock;
120-
121124
void Enter(const char* pszName, const char* pszFile, int nLine)
122125
{
123-
EnterCritical(pszName, pszFile, nLine, (void*)(lock.mutex()));
126+
EnterCritical(pszName, pszFile, nLine, (void*)(Base::mutex()));
124127
#ifdef DEBUG_LOCKCONTENTION
125-
if (!lock.try_lock()) {
128+
if (!Base::try_lock()) {
126129
PrintLockContention(pszName, pszFile, nLine);
127130
#endif
128-
lock.lock();
131+
Base::lock();
129132
#ifdef DEBUG_LOCKCONTENTION
130133
}
131134
#endif
132135
}
133136

134137
bool TryEnter(const char* pszName, const char* pszFile, int nLine)
135138
{
136-
EnterCritical(pszName, pszFile, nLine, (void*)(lock.mutex()), true);
137-
lock.try_lock();
138-
if (!lock.owns_lock())
139+
EnterCritical(pszName, pszFile, nLine, (void*)(Base::mutex()), true);
140+
Base::try_lock();
141+
if (!Base::owns_lock())
139142
LeaveCritical();
140-
return lock.owns_lock();
143+
return Base::owns_lock();
141144
}
142145

143146
public:
144-
CCriticalBlock(CCriticalSection& mutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(mutexIn) : lock(mutexIn, std::defer_lock)
147+
CCriticalBlock(Mutex& mutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(mutexIn) : Base(mutexIn, std::defer_lock)
145148
{
146149
if (fTry)
147150
TryEnter(pszName, pszFile, nLine);
148151
else
149152
Enter(pszName, pszFile, nLine);
150153
}
151154

152-
CCriticalBlock(CCriticalSection* pmutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(pmutexIn)
155+
CCriticalBlock(Mutex* pmutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(pmutexIn)
153156
{
154157
if (!pmutexIn) return;
155158

156-
lock = std::unique_lock<CCriticalSection>(*pmutexIn, std::defer_lock);
159+
*static_cast<Base*>(this) = Base(*pmutexIn, std::defer_lock);
157160
if (fTry)
158161
TryEnter(pszName, pszFile, nLine);
159162
else
@@ -162,22 +165,28 @@ class SCOPED_LOCKABLE CCriticalBlock
162165

163166
~CCriticalBlock() UNLOCK_FUNCTION()
164167
{
165-
if (lock.owns_lock())
168+
if (Base::owns_lock())
166169
LeaveCritical();
167170
}
168171

169172
operator bool()
170173
{
171-
return lock.owns_lock();
174+
return Base::owns_lock();
172175
}
173176
};
174177

178+
template<typename MutexArg>
179+
using DebugLock = CCriticalBlock<typename std::remove_reference<typename std::remove_pointer<MutexArg>::type>::type>;
180+
175181
#define PASTE(x, y) x ## y
176182
#define PASTE2(x, y) PASTE(x, y)
177183

178-
#define LOCK(cs) CCriticalBlock PASTE2(criticalblock, __COUNTER__)(cs, #cs, __FILE__, __LINE__)
179-
#define LOCK2(cs1, cs2) CCriticalBlock criticalblock1(cs1, #cs1, __FILE__, __LINE__), criticalblock2(cs2, #cs2, __FILE__, __LINE__)
180-
#define TRY_LOCK(cs, name) CCriticalBlock name(cs, #cs, __FILE__, __LINE__, true)
184+
#define LOCK(cs) DebugLock<decltype(cs)> PASTE2(criticalblock, __COUNTER__)(cs, #cs, __FILE__, __LINE__)
185+
#define LOCK2(cs1, cs2) \
186+
DebugLock<decltype(cs1)> criticalblock1(cs1, #cs1, __FILE__, __LINE__); \
187+
DebugLock<decltype(cs2)> criticalblock2(cs2, #cs2, __FILE__, __LINE__);
188+
#define TRY_LOCK(cs, name) DebugLock<decltype(cs)> name(cs, #cs, __FILE__, __LINE__, true)
189+
#define WAIT_LOCK(cs, name) DebugLock<decltype(cs)> name(cs, #cs, __FILE__, __LINE__)
181190

182191
#define ENTER_CRITICAL_SECTION(cs) \
183192
{ \

0 commit comments

Comments
 (0)