LBRYCommunity/lbrycrd
1
0
Fork 0
Code Issues 29 Pull requests 1 Projects Releases 32 Packages Wiki Activity

Merge #11640: Make LOCK, LOCK2, TRY_LOCK work with CWaitableCriticalSection

Browse source 
9c4dc597dd Use LOCK macros for non-recursive locks (Russell Yanofsky)
1382913e61 Make LOCK, LOCK2, TRY_LOCK work with CWaitableCriticalSection (Russell Yanofsky)
ba1f095aad MOVEONLY Move AnnotatedMixin declaration (Russell Yanofsky)
41b88e9337 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
This commit is contained in:
Wladimir J. van der Laan 2018-08-31 15:49:26 +02:00
commit 385ad11040
No known key found for this signature in database
GPG key ID: 1E4AED62986CD25D
14 changed files with 143 additions and 70 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
src/Makefile.test.include
View file

@ -83,6 +83,7 @@ BITCOIN_TESTS =\
test/sigopcount_tests.cpp \
test/skiplist_tests.cpp \
test/streams_tests.cpp \
test/sync_tests.cpp \
test/timedata_tests.cpp \
test/torcontrol_tests.cpp \
test/transaction_tests.cpp \

8
src/httpserver.cpp
View file

@ -69,7 +69,7 @@ class WorkQueue
{
private:
/** Mutex protects entire object */
std::mutex cs;
CWaitableCriticalSection cs;
std::condition_variable cond;
std::deque<std::unique_ptr<WorkItem>> queue;
bool running;
@ -88,7 +88,7 @@ public:
/** Enqueue a work item */
bool Enqueue(WorkItem* item)
{
std::unique_lock<std::mutex> lock(cs);
LOCK(cs);
if (queue.size() >= maxDepth) {
return false;
}
@ -102,7 +102,7 @@ public:
while (true) {
std::unique_ptr<WorkItem> i;
{
std::unique_lock<std::mutex> lock(cs);
WAIT_LOCK(cs, lock);
while (running && queue.empty())
cond.wait(lock);
if (!running)
@ -116,7 +116,7 @@ public:
/** Interrupt and exit loops */
void Interrupt()
{
std::unique_lock<std::mutex> lock(cs);
LOCK(cs);
running = false;
cond.notify_all();
}

4
src/init.cpp
View file

@ -568,7 +568,7 @@ static void BlockNotifyGenesisWait(bool, const CBlockIndex *pBlockIndex)
{
if (pBlockIndex != nullptr) {
{
WaitableLock lock_GenesisWait(cs_GenesisWait);
LOCK(cs_GenesisWait);
fHaveGenesis = true;
}
condvar_GenesisWait.notify_all();
@ -1661,7 +1661,7 @@ bool AppInitMain()
// Wait for genesis block to be processed
{
WaitableLock lock(cs_GenesisWait);
WAIT_LOCK(cs_GenesisWait, lock);
// We previously could hang here if StartShutdown() is called prior to
// ThreadImport getting started, so instead we just wait on a timer to
// check ShutdownRequested() regularly.

4
src/net.cpp
View file

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

2
src/net.h
View file

@ -427,7 +427,7 @@ private:
bool fMsgProcWake;
std::condition_variable condMsgProc;
std::mutex mutexMsgProc;
CWaitableCriticalSection mutexMsgProc;
std::atomic<bool> flagInterruptMsgProc;
CThreadInterrupt interruptNet;

7
src/random.cpp
View file

@ -12,6 +12,7 @@
#include <wincrypt.h>
#endif
#include <logging.h> // for LogPrint()
#include <sync.h> // for WAIT_LOCK
#include <utiltime.h> // for GetTime()
#include <stdlib.h>
@ -295,7 +296,7 @@ void RandAddSeedSleep()
}
static std::mutex cs_rng_state;
static CWaitableCriticalSection cs_rng_state;
static unsigned char rng_state[32] = {0};
static uint64_t rng_counter = 0;
@ -305,7 +306,7 @@ static void AddDataToRng(void* data, size_t len) {
hasher.Write((const unsigned char*)data, len);
unsigned char buf[64];
{
std::unique_lock<std::mutex> lock(cs_rng_state);
WAIT_LOCK(cs_rng_state, lock);
hasher.Write(rng_state, sizeof(rng_state));
hasher.Write((const unsigned char*)&rng_counter, sizeof(rng_counter));
++rng_counter;
@ -337,7 +338,7 @@ void GetStrongRandBytes(unsigned char* out, int num)
// Combine with and update state
{
std::unique_lock<std::mutex> lock(cs_rng_state);
WAIT_LOCK(cs_rng_state, lock);
hasher.Write(rng_state, sizeof(rng_state));
hasher.Write((const unsigned char*)&rng_counter, sizeof(rng_counter));
++rng_counter;

8
src/rpc/blockchain.cpp
View file

@ -50,7 +50,7 @@ struct CUpdatedBlock
int height;
};
static std::mutex cs_blockchange;
static CWaitableCriticalSection cs_blockchange;
static std::condition_variable cond_blockchange;
static CUpdatedBlock latestblock;
@ -225,7 +225,7 @@ static UniValue waitfornewblock(const JSONRPCRequest& request)
CUpdatedBlock block;
{
std::unique_lock<std::mutex> lock(cs_blockchange);
WAIT_LOCK(cs_blockchange, lock);
block = latestblock;
if(timeout)
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)
CUpdatedBlock block;
{
std::unique_lock<std::mutex> lock(cs_blockchange);
WAIT_LOCK(cs_blockchange, lock);
if(timeout)
cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&hash]{return latestblock.hash == hash || !IsRPCRunning();});
else
@ -310,7 +310,7 @@ static UniValue waitforblockheight(const JSONRPCRequest& request)
CUpdatedBlock block;
{
std::unique_lock<std::mutex> lock(cs_blockchange);
WAIT_LOCK(cs_blockchange, lock);
if(timeout)
cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&height]{return latestblock.height >= height || !IsRPCRunning();});
else

2
src/rpc/mining.cpp
View file

@ -470,7 +470,7 @@ static UniValue getblocktemplate(const JSONRPCRequest& request)
{
checktxtime = std::chrono::steady_clock::now() + std::chrono::minutes(1);
WaitableLock lock(g_best_block_mutex);
WAIT_LOCK(g_best_block_mutex, lock);
while (g_best_block == hashWatchedChain && IsRPCRunning())
{
if (g_best_block_cv.wait_until(lock, checktxtime) == std::cv_status::timeout)

8
src/sync.cpp
View file

@ -100,7 +100,11 @@ static void potential_deadlock_detected(const std::pair<void*, void*>& mismatch,
}
LogPrintf(" %s\n", i.second.ToString());
}
assert(false);
if (g_debug_lockorder_abort) {
fprintf(stderr, "Assertion failed: detected inconsistent lock order at %s:%i, details in debug log.\n", __FILE__, __LINE__);
abort();
}
throw std::logic_error("potential deadlock detected");
}
static void push_lock(void* c, const CLockLocation& locklocation)
@ -189,4 +193,6 @@ void DeleteLock(void* cs)
}
}
bool g_debug_lockorder_abort = true;
#endif /* DEBUG_LOCKORDER */

105
src/sync.h
View file

@ -46,14 +46,42 @@ LEAVE_CRITICAL_SECTION(mutex); // no RAII
// //
///////////////////////////////
#ifdef DEBUG_LOCKORDER
void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false);
void LeaveCritical();
std::string LocksHeld();
void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) ASSERT_EXCLUSIVE_LOCK(cs);
void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs);
void DeleteLock(void* cs);
/**
* Template mixin that adds -Wthread-safety locking
* annotations to a subset of the mutex API.
* Call abort() if a potential lock order deadlock bug is detected, instead of
* just logging information and throwing a logic_error. Defaults to true, and
* set to false in DEBUG_LOCKORDER unit tests.
*/
extern bool g_debug_lockorder_abort;
#else
void static inline EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false) {}
void static inline LeaveCritical() {}
void static inline AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) ASSERT_EXCLUSIVE_LOCK(cs) {}
void static inline AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) {}
void static inline DeleteLock(void* cs) {}
#endif
#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs)
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
/**
* Template mixin that adds -Wthread-safety locking annotations and lock order
* checking to a subset of the mutex API.
*/
template <typename PARENT>
class LOCKABLE AnnotatedMixin : public PARENT
{
public:
~AnnotatedMixin() {
DeleteLock((void*)this);
}
void lock() EXCLUSIVE_LOCK_FUNCTION()
{
PARENT::lock();
@ -68,36 +96,15 @@ public:
{
return PARENT::try_lock();
}
};
#ifdef DEBUG_LOCKORDER
void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false);
void LeaveCritical();
std::string LocksHeld();
void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) ASSERT_EXCLUSIVE_LOCK(cs);
void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs);
void DeleteLock(void* cs);
#else
void static inline EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false) {}
void static inline LeaveCritical() {}
void static inline AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) ASSERT_EXCLUSIVE_LOCK(cs) {}
void static inline AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) {}
void static inline DeleteLock(void* cs) {}
#endif
#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs)
#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
using UniqueLock = std::unique_lock<PARENT>;
};
/**
* Wrapped mutex: supports recursive locking, but no waiting
* TODO: We should move away from using the recursive lock by default.
*/
class CCriticalSection : public AnnotatedMixin<std::recursive_mutex>
{
public:
~CCriticalSection() {
DeleteLock((void*)this);
}
};
typedef AnnotatedMixin<std::recursive_mutex> CCriticalSection;
/** Wrapped mutex: supports waiting but not recursive locking */
typedef AnnotatedMixin<std::mutex> CWaitableCriticalSection;
@ -105,27 +112,23 @@ typedef AnnotatedMixin<std::mutex> CWaitableCriticalSection;
/** Just a typedef for std::condition_variable, can be wrapped later if desired */
typedef std::condition_variable CConditionVariable;
/** Just a typedef for std::unique_lock, can be wrapped later if desired */
typedef std::unique_lock<std::mutex> WaitableLock;
#ifdef DEBUG_LOCKCONTENTION
void PrintLockContention(const char* pszName, const char* pszFile, int nLine);
#endif
/** Wrapper around std::unique_lock<CCriticalSection> */
class SCOPED_LOCKABLE CCriticalBlock
/** Wrapper around std::unique_lock style lock for Mutex. */
template <typename Mutex, typename Base = typename Mutex::UniqueLock>
class SCOPED_LOCKABLE CCriticalBlock : public Base
{
private:
std::unique_lock<CCriticalSection> lock;
void Enter(const char* pszName, const char* pszFile, int nLine)
{
EnterCritical(pszName, pszFile, nLine, (void*)(lock.mutex()));
EnterCritical(pszName, pszFile, nLine, (void*)(Base::mutex()));
#ifdef DEBUG_LOCKCONTENTION
if (!lock.try_lock()) {
if (!Base::try_lock()) {
PrintLockContention(pszName, pszFile, nLine);
#endif
lock.lock();
Base::lock();
#ifdef DEBUG_LOCKCONTENTION
}
#endif
@ -133,15 +136,15 @@ private:
bool TryEnter(const char* pszName, const char* pszFile, int nLine)
{
EnterCritical(pszName, pszFile, nLine, (void*)(lock.mutex()), true);
lock.try_lock();
if (!lock.owns_lock())
EnterCritical(pszName, pszFile, nLine, (void*)(Base::mutex()), true);
Base::try_lock();
if (!Base::owns_lock())
LeaveCritical();
return lock.owns_lock();
return Base::owns_lock();
}
public:
CCriticalBlock(CCriticalSection& mutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(mutexIn) : lock(mutexIn, std::defer_lock)
CCriticalBlock(Mutex& mutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(mutexIn) : Base(mutexIn, std::defer_lock)
{
if (fTry)
TryEnter(pszName, pszFile, nLine);
@ -149,11 +152,11 @@ public:
Enter(pszName, pszFile, nLine);
}
CCriticalBlock(CCriticalSection* pmutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(pmutexIn)
CCriticalBlock(Mutex* pmutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(pmutexIn)
{
if (!pmutexIn) return;
lock = std::unique_lock<CCriticalSection>(*pmutexIn, std::defer_lock);
*static_cast<Base*>(this) = Base(*pmutexIn, std::defer_lock);
if (fTry)
TryEnter(pszName, pszFile, nLine);
else
@ -162,22 +165,28 @@ public:
~CCriticalBlock() UNLOCK_FUNCTION()
{
if (lock.owns_lock())
if (Base::owns_lock())
LeaveCritical();
}
operator bool()
{
return lock.owns_lock();
return Base::owns_lock();
}
};
template<typename MutexArg>
using DebugLock = CCriticalBlock<typename std::remove_reference<typename std::remove_pointer<MutexArg>::type>::type>;
#define PASTE(x, y) x ## y
#define PASTE2(x, y) PASTE(x, y)
#define LOCK(cs) CCriticalBlock PASTE2(criticalblock, __COUNTER__)(cs, #cs, __FILE__, __LINE__)
#define LOCK2(cs1, cs2) CCriticalBlock criticalblock1(cs1, #cs1, __FILE__, __LINE__), criticalblock2(cs2, #cs2, __FILE__, __LINE__)
#define TRY_LOCK(cs, name) CCriticalBlock name(cs, #cs, __FILE__, __LINE__, true)
#define LOCK(cs) DebugLock<decltype(cs)> PASTE2(criticalblock, __COUNTER__)(cs, #cs, __FILE__, __LINE__)
#define LOCK2(cs1, cs2) \
DebugLock<decltype(cs1)> criticalblock1(cs1, #cs1, __FILE__, __LINE__); \
DebugLock<decltype(cs2)> criticalblock2(cs2, #cs2, __FILE__, __LINE__);
#define TRY_LOCK(cs, name) DebugLock<decltype(cs)> name(cs, #cs, __FILE__, __LINE__, true)
#define WAIT_LOCK(cs, name) DebugLock<decltype(cs)> name(cs, #cs, __FILE__, __LINE__)
#define ENTER_CRITICAL_SECTION(cs) \
{ \

52
src/test/sync_tests.cpp Normal file
View file

@ -0,0 +1,52 @@
// Copyright (c) 2012-2017 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#include <sync.h>
#include <test/test_bitcoin.h>
#include <boost/test/unit_test.hpp>
namespace {
template <typename MutexType>
void TestPotentialDeadLockDetected(MutexType& mutex1, MutexType& mutex2)
{
{
LOCK2(mutex1, mutex2);
}
bool error_thrown = false;
try {
LOCK2(mutex2, mutex1);
} catch (const std::logic_error& e) {
BOOST_CHECK_EQUAL(e.what(), "potential deadlock detected");
error_thrown = true;
}
#ifdef DEBUG_LOCKORDER
BOOST_CHECK(error_thrown);
#else
BOOST_CHECK(!error_thrown);
#endif
}
} // namespace
BOOST_FIXTURE_TEST_SUITE(sync_tests, BasicTestingSetup)
BOOST_AUTO_TEST_CASE(potential_deadlock_detected)
{
#ifdef DEBUG_LOCKORDER
bool prev = g_debug_lockorder_abort;
g_debug_lockorder_abort = false;
#endif
CCriticalSection rmutex1, rmutex2;
TestPotentialDeadLockDetected(rmutex1, rmutex2);
CWaitableCriticalSection mutex1, mutex2;
TestPotentialDeadLockDetected(mutex1, mutex2);
#ifdef DEBUG_LOCKORDER
g_debug_lockorder_abort = prev;
#endif
}
BOOST_AUTO_TEST_SUITE_END()

6
src/threadinterrupt.cpp
View file

@ -5,6 +5,8 @@
#include <threadinterrupt.h>
#include <sync.h>
CThreadInterrupt::CThreadInterrupt() : flag(false) {}
CThreadInterrupt::operator bool() const
@ -20,7 +22,7 @@ void CThreadInterrupt::reset()
void CThreadInterrupt::operator()()
{
{
std::unique_lock<std::mutex> lock(mut);
LOCK(mut);
flag.store(true, std::memory_order_release);
}
cond.notify_all();
@ -28,7 +30,7 @@ void CThreadInterrupt::operator()()
bool CThreadInterrupt::sleep_for(std::chrono::milliseconds rel_time)
{
std::unique_lock<std::mutex> lock(mut);
WAIT_LOCK(mut, lock);
return !cond.wait_for(lock, rel_time, [this]() { return flag.load(std::memory_order_acquire); });
}

4
src/threadinterrupt.h
View file

@ -5,6 +5,8 @@
#ifndef BITCOIN_THREADINTERRUPT_H
#define BITCOIN_THREADINTERRUPT_H
#include <sync.h>
#include <atomic>
#include <chrono>
#include <condition_variable>
@ -28,7 +30,7 @@ public:
private:
std::condition_variable cond;
std::mutex mut;
CWaitableCriticalSection mut;
std::atomic<bool> flag;
};

2
src/validation.cpp
View file

@ -2239,7 +2239,7 @@ void static UpdateTip(const CBlockIndex *pindexNew, const CChainParams& chainPar
mempool.AddTransactionsUpdated(1);
{
WaitableLock lock(g_best_block_mutex);
LOCK(g_best_block_mutex);
g_best_block = pindexNew->GetBlockHash();
g_best_block_cv.notify_all();
}