diff --git a/src/Makefile.test.include b/src/Makefile.test.include index d33af3911..b197fbe9a 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -128,6 +128,7 @@ BITCOIN_TESTS =\ test/sigopcount_tests.cpp \ test/skiplist_tests.cpp \ test/streams_tests.cpp \ + test/sync_tests.cpp \ test/test_bitcoin.cpp \ test/test_bitcoin.h \ test/testutil.cpp \ diff --git a/src/sync.cpp b/src/sync.cpp index fce57f1df..f9e26299b 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -9,8 +9,13 @@ #include -#include -#include +#include +#include +#include +#include +#include +#include +#include #ifdef DEBUG_LOCKCONTENTION void PrintLockContention(const char* pszName, const char* pszFile, int nLine) @@ -55,31 +60,31 @@ private: int sourceLine; }; -typedef std::vector > LockStack; -typedef std::map, LockStack> LockOrders; -typedef std::set > InvLockOrders; +typedef std::pair LockStackItem; +typedef std::vector LockStack; +typedef std::unordered_map LockStacks; + +typedef std::pair LockPair; +typedef std::map LockOrders; +typedef std::set InvLockOrders; struct LockData { - // Very ugly hack: as the global constructs and destructors run single - // threaded, we use this boolean to know whether LockData still exists, - // as DeleteLock can get called by global CCriticalSection destructors - // after LockData disappears. - bool available; - LockData() : available(true) {} - ~LockData() { available = false; } - + LockStacks m_lock_stacks; LockOrders lockorders; InvLockOrders invlockorders; - boost::mutex dd_mutex; -} static lockdata; + std::mutex dd_mutex; +}; -boost::thread_specific_ptr lockstack; +LockData& GetLockData() { + static LockData& lock_data = *new LockData(); + return lock_data; +} -static void potential_deadlock_detected(const std::pair& mismatch, const LockStack& s1, const LockStack& s2) +static void potential_deadlock_detected(const LockPair& mismatch, const LockStack& s1, const LockStack& s2) { LogPrintf("POTENTIAL DEADLOCK DETECTED\n"); LogPrintf("Previous lock order was:\n"); - BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s2) { + for (const LockStackItem& i : s2) { if (i.first == mismatch.first) { LogPrintf(" (1)"); } @@ -89,7 +94,7 @@ static void potential_deadlock_detected(const std::pair& mismatch, LogPrintf(" %s\n", i.second.ToString()); } LogPrintf("Current lock order is:\n"); - BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s1) { + for (const LockStackItem& i : s1) { if (i.first == mismatch.first) { LogPrintf(" (1)"); } @@ -98,28 +103,31 @@ static void potential_deadlock_detected(const std::pair& 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, bool fTry) { - if (lockstack.get() == NULL) - lockstack.reset(new LockStack); + LockData& lockdata = GetLockData(); + std::lock_guard lock(lockdata.dd_mutex); - boost::unique_lock lock(lockdata.dd_mutex); + LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()]; + lock_stack.emplace_back(c, locklocation); - (*lockstack).push_back(std::make_pair(c, locklocation)); - - BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, (*lockstack)) { + for (const LockStackItem& i : lock_stack) { if (i.first == c) break; - std::pair p1 = std::make_pair(i.first, c); + const LockPair p1 = std::make_pair(i.first, c); if (lockdata.lockorders.count(p1)) continue; - lockdata.lockorders[p1] = (*lockstack); + lockdata.lockorders[p1] = lock_stack; - std::pair p2 = std::make_pair(c, i.first); + const LockPair p2 = std::make_pair(c, i.first); lockdata.invlockorders.insert(p2); if (lockdata.lockorders.count(p2)) potential_deadlock_detected(p1, lockdata.lockorders[p2], lockdata.lockorders[p1]); @@ -128,7 +136,14 @@ static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) static void pop_lock() { - (*lockstack).pop_back(); + LockData& lockdata = GetLockData(); + std::lock_guard lock(lockdata.dd_mutex); + + LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()]; + lock_stack.pop_back(); + if (lock_stack.empty()) { + lockdata.m_lock_stacks.erase(std::this_thread::get_id()); + } } void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry) @@ -143,41 +158,58 @@ void LeaveCritical() std::string LocksHeld() { + LockData& lockdata = GetLockData(); + std::lock_guard lock(lockdata.dd_mutex); + + const LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()]; + std::string result; - BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, *lockstack) + for (const LockStackItem& i : lock_stack) { result += i.second.ToString() + std::string("\n"); + } return result; } +static bool LockHeld(void* mutex) +{ + LockData& lockdata = GetLockData(); + std::lock_guard lock(lockdata.dd_mutex); + + const LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()]; + for (const LockStackItem& i : lock_stack) { + if (i.first == mutex) return true; + } + + return false; +} + void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) { - BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, *lockstack) - if (i.first == cs) - return; + if (LockHeld(cs)) return; fprintf(stderr, "Assertion failed: lock %s not held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld().c_str()); abort(); } void DeleteLock(void* cs) { - if (!lockdata.available) { - // We're already shutting down. - return; - } - boost::unique_lock lock(lockdata.dd_mutex); - std::pair item = std::make_pair(cs, (void*)0); + LockData& lockdata = GetLockData(); + std::lock_guard lock(lockdata.dd_mutex); + + const LockPair item = std::make_pair(cs, (void*)0); LockOrders::iterator it = lockdata.lockorders.lower_bound(item); while (it != lockdata.lockorders.end() && it->first.first == cs) { - std::pair invitem = std::make_pair(it->first.second, it->first.first); + const LockPair invitem = std::make_pair(it->first.second, it->first.first); lockdata.invlockorders.erase(invitem); lockdata.lockorders.erase(it++); } InvLockOrders::iterator invit = lockdata.invlockorders.lower_bound(item); while (invit != lockdata.invlockorders.end() && invit->first == cs) { - std::pair invinvitem = std::make_pair(invit->second, invit->first); + const LockPair invinvitem = std::make_pair(invit->second, invit->first); lockdata.lockorders.erase(invinvitem); lockdata.invlockorders.erase(invit++); } } +bool g_debug_lockorder_abort = true; + #endif /* DEBUG_LOCKORDER */ diff --git a/src/sync.h b/src/sync.h index 3b29050e0..c79245325 100644 --- a/src/sync.h +++ b/src/sync.h @@ -77,6 +77,13 @@ void LeaveCritical(); std::string LocksHeld(); void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs); void DeleteLock(void* cs); + +/** + * 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() {} diff --git a/src/test/sync_tests.cpp b/src/test/sync_tests.cpp new file mode 100644 index 000000000..2e0951c3a --- /dev/null +++ b/src/test/sync_tests.cpp @@ -0,0 +1,41 @@ +// 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 +#include + +#include + +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 mutex1, 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 + + #ifdef DEBUG_LOCKORDER + g_debug_lockorder_abort = prev; + #endif +} + +BOOST_AUTO_TEST_SUITE_END()