From c3cf2f55013c4ea1c1ef4a878fc7ff8e92f2c42d Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Tue, 11 Feb 2020 14:54:50 +1000 Subject: rpc/blockchain.cpp: Remove g_utxosetscan mutex that is only protecting a single atomic variable --- src/rpc/blockchain.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) (limited to 'src') diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index f7ccbae706..d43c22e4da 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1976,7 +1976,6 @@ bool FindScriptPubKey(std::atomic& scan_progress, const std::atomic& } /** RAII object to prevent concurrency issue when scanning the txout set */ -static std::mutex g_utxosetscan; static std::atomic g_scan_progress; static std::atomic g_scan_in_progress; static std::atomic g_should_abort_scan; @@ -1989,18 +1988,15 @@ public: bool reserve() { CHECK_NONFATAL(!m_could_reserve); - std::lock_guard lock(g_utxosetscan); - if (g_scan_in_progress) { + if (g_scan_in_progress.exchange(true)) { return false; } - g_scan_in_progress = true; m_could_reserve = true; return true; } ~CoinsViewScanReserver() { if (m_could_reserve) { - std::lock_guard lock(g_utxosetscan); g_scan_in_progress = false; } } -- cgit v1.2.3 From de7c5f41aba860751ef7824245e6d9d5088a1200 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Thu, 30 May 2019 14:02:41 +1000 Subject: wallet/wallet.h: Remove mutexScanning which was only protecting a single atomic bool --- src/wallet/wallet.h | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) (limited to 'src') diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 8f624c25d7..a4cb8253d3 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -631,7 +631,6 @@ private: std::atomic fScanningWallet{false}; // controlled by WalletRescanReserver std::atomic m_scanning_start{0}; std::atomic m_scanning_progress{0}; - std::mutex mutexScanning; friend class WalletRescanReserver; //! the current wallet version: clients below this version are not able to load the wallet @@ -1286,13 +1285,11 @@ public: bool reserve() { assert(!m_could_reserve); - std::lock_guard lock(m_wallet.mutexScanning); - if (m_wallet.fScanningWallet) { + if (m_wallet.fScanningWallet.exchange(true)) { return false; } m_wallet.m_scanning_start = GetTimeMillis(); m_wallet.m_scanning_progress = 0; - m_wallet.fScanningWallet = true; m_could_reserve = true; return true; } @@ -1304,7 +1301,6 @@ public: ~WalletRescanReserver() { - std::lock_guard lock(m_wallet.mutexScanning); if (m_could_reserve) { m_wallet.fScanningWallet = false; } -- cgit v1.2.3 From 8b5af3d4c1270267ad85e78f661bf8fab06f3aad Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Thu, 30 May 2019 13:44:02 +1000 Subject: net: fMsgProcWake use LOCK instead of lock_guard --- src/net.cpp | 6 +++--- src/net.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/net.cpp b/src/net.cpp index dcc613ba88..4e80e123d1 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1454,7 +1454,7 @@ void CConnman::ThreadSocketHandler() void CConnman::WakeMessageHandler() { { - std::lock_guard lock(mutexMsgProc); + LOCK(mutexMsgProc); fMsgProcWake = true; } condMsgProc.notify_one(); @@ -2057,7 +2057,7 @@ void CConnman::ThreadMessageHandler() WAIT_LOCK(mutexMsgProc, lock); if (!fMoreWork) { - condMsgProc.wait_until(lock, std::chrono::steady_clock::now() + std::chrono::milliseconds(100), [this] { return fMsgProcWake; }); + condMsgProc.wait_until(lock, std::chrono::steady_clock::now() + std::chrono::milliseconds(100), [this]() EXCLUSIVE_LOCKS_REQUIRED(mutexMsgProc) { return fMsgProcWake; }); } fMsgProcWake = false; } @@ -2366,7 +2366,7 @@ static CNetCleanup instance_of_cnetcleanup; void CConnman::Interrupt() { { - std::lock_guard lock(mutexMsgProc); + LOCK(mutexMsgProc); flagInterruptMsgProc = true; } condMsgProc.notify_all(); diff --git a/src/net.h b/src/net.h index 0d79efbba7..a4bf06e47f 100644 --- a/src/net.h +++ b/src/net.h @@ -451,7 +451,7 @@ private: const uint64_t nSeed0, nSeed1; /** flag for waking the message processor. */ - bool fMsgProcWake; + bool fMsgProcWake GUARDED_BY(mutexMsgProc); std::condition_variable condMsgProc; Mutex mutexMsgProc; -- cgit v1.2.3 From 479c5846f7477625ec275fbb8a076c7ef157172b Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Thu, 30 May 2019 13:37:31 +1000 Subject: rpc/blockchain.cpp: thread safety annotations for latestblock --- src/rpc/blockchain.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) (limited to 'src') diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index d43c22e4da..dbf8da69c1 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -51,7 +51,7 @@ struct CUpdatedBlock static Mutex cs_blockchange; static std::condition_variable cond_blockchange; -static CUpdatedBlock latestblock; +static CUpdatedBlock latestblock GUARDED_BY(cs_blockchange); CTxMemPool& EnsureMemPool() { @@ -208,7 +208,7 @@ static UniValue getbestblockhash(const JSONRPCRequest& request) void RPCNotifyBlockChange(bool ibd, const CBlockIndex * pindex) { if(pindex) { - std::lock_guard lock(cs_blockchange); + LOCK(cs_blockchange); latestblock.hash = pindex->GetBlockHash(); latestblock.height = pindex->nHeight; } @@ -243,9 +243,9 @@ static UniValue waitfornewblock(const JSONRPCRequest& request) 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(); }); + cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&block]() EXCLUSIVE_LOCKS_REQUIRED(cs_blockchange) {return latestblock.height != block.height || latestblock.hash != block.hash || !IsRPCRunning(); }); else - cond_blockchange.wait(lock, [&block]{return latestblock.height != block.height || latestblock.hash != block.hash || !IsRPCRunning(); }); + cond_blockchange.wait(lock, [&block]() EXCLUSIVE_LOCKS_REQUIRED(cs_blockchange) {return latestblock.height != block.height || latestblock.hash != block.hash || !IsRPCRunning(); }); block = latestblock; } UniValue ret(UniValue::VOBJ); @@ -285,9 +285,9 @@ static UniValue waitforblock(const JSONRPCRequest& request) { WAIT_LOCK(cs_blockchange, lock); if(timeout) - cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&hash]{return latestblock.hash == hash || !IsRPCRunning();}); + cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&hash]() EXCLUSIVE_LOCKS_REQUIRED(cs_blockchange) {return latestblock.hash == hash || !IsRPCRunning();}); else - cond_blockchange.wait(lock, [&hash]{return latestblock.hash == hash || !IsRPCRunning(); }); + cond_blockchange.wait(lock, [&hash]() EXCLUSIVE_LOCKS_REQUIRED(cs_blockchange) {return latestblock.hash == hash || !IsRPCRunning(); }); block = latestblock; } @@ -329,9 +329,9 @@ static UniValue waitforblockheight(const JSONRPCRequest& request) { WAIT_LOCK(cs_blockchange, lock); if(timeout) - cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&height]{return latestblock.height >= height || !IsRPCRunning();}); + cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&height]() EXCLUSIVE_LOCKS_REQUIRED(cs_blockchange) {return latestblock.height >= height || !IsRPCRunning();}); else - cond_blockchange.wait(lock, [&height]{return latestblock.height >= height || !IsRPCRunning(); }); + cond_blockchange.wait(lock, [&height]() EXCLUSIVE_LOCKS_REQUIRED(cs_blockchange) {return latestblock.height >= height || !IsRPCRunning(); }); block = latestblock; } UniValue ret(UniValue::VOBJ); -- cgit v1.2.3 From a7887899480db72328784009181d93904e6d479d Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Tue, 11 Feb 2020 16:00:47 +1000 Subject: test/checkqueue_tests: thread safety annotations --- src/test/checkqueue_tests.cpp | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) (limited to 'src') diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp index 0565982215..35750b2ebc 100644 --- a/src/test/checkqueue_tests.cpp +++ b/src/test/checkqueue_tests.cpp @@ -3,6 +3,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include #include #include #include @@ -57,14 +58,14 @@ struct FailingCheck { }; struct UniqueCheck { - static std::mutex m; - static std::unordered_multiset results; + static Mutex m; + static std::unordered_multiset results GUARDED_BY(m); size_t check_id; UniqueCheck(size_t check_id_in) : check_id(check_id_in){}; UniqueCheck() : check_id(0){}; bool operator()() { - std::lock_guard l(m); + LOCK(m); results.insert(check_id); return true; } @@ -127,7 +128,7 @@ struct FrozenCleanupCheck { std::mutex FrozenCleanupCheck::m{}; std::atomic FrozenCleanupCheck::nFrozen{0}; std::condition_variable FrozenCleanupCheck::cv{}; -std::mutex UniqueCheck::m; +Mutex UniqueCheck::m; std::unordered_multiset UniqueCheck::results; std::atomic FakeCheckCheckCompletion::n_calls{0}; std::atomic MemoryCheck::fake_allocated_memory{0}; @@ -290,11 +291,15 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_UniqueCheck) control.Add(vChecks); } } - bool r = true; - BOOST_REQUIRE_EQUAL(UniqueCheck::results.size(), COUNT); - for (size_t i = 0; i < COUNT; ++i) - r = r && UniqueCheck::results.count(i) == 1; - BOOST_REQUIRE(r); + { + LOCK(UniqueCheck::m); + bool r = true; + BOOST_REQUIRE_EQUAL(UniqueCheck::results.size(), COUNT); + for (size_t i = 0; i < COUNT; ++i) { + r = r && UniqueCheck::results.count(i) == 1; + } + BOOST_REQUIRE(r); + } tg.interrupt_all(); tg.join_all(); } -- cgit v1.2.3 From e685ca19928eec4e687c66f5edfcfff085a42c27 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Tue, 11 Feb 2020 16:15:37 +1000 Subject: util/system.cpp: add thread safety annotations for dir_locks --- src/util/system.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) (limited to 'src') diff --git a/src/util/system.cpp b/src/util/system.cpp index 2013b416db..bde0f097be 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -3,6 +3,7 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include #include #include @@ -75,18 +76,18 @@ const char * const BITCOIN_CONF_FILENAME = "bitcoin.conf"; ArgsManager gArgs; +/** Mutex to protect dir_locks. */ +static Mutex cs_dir_locks; /** A map that contains all the currently held directory locks. After * successful locking, these will be held here until the global destructor * cleans them up and thus automatically unlocks them, or ReleaseDirectoryLocks * is called. */ -static std::map> dir_locks; -/** Mutex to protect dir_locks. */ -static std::mutex cs_dir_locks; +static std::map> dir_locks GUARDED_BY(cs_dir_locks); bool LockDirectory(const fs::path& directory, const std::string lockfile_name, bool probe_only) { - std::lock_guard ulock(cs_dir_locks); + LOCK(cs_dir_locks); fs::path pathLockFile = directory / lockfile_name; // If a lock for this directory already exists in the map, don't try to re-lock it @@ -110,13 +111,13 @@ bool LockDirectory(const fs::path& directory, const std::string lockfile_name, b void UnlockDirectory(const fs::path& directory, const std::string& lockfile_name) { - std::lock_guard lock(cs_dir_locks); + LOCK(cs_dir_locks); dir_locks.erase((directory / lockfile_name).string()); } void ReleaseDirectoryLocks() { - std::lock_guard ulock(cs_dir_locks); + LOCK(cs_dir_locks); dir_locks.clear(); } -- cgit v1.2.3 From 5478d6c099e76fe070703cc5383cba7b91468b0f Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Tue, 11 Feb 2020 16:50:22 +1000 Subject: logging: thread safety annotations Adds LockGuard helper in threadsafety.h to replace lock_guard when LOCK(Mutex) isn't available for use. --- src/logging.cpp | 6 +++--- src/logging.h | 16 +++++++++------- src/threadsafety.h | 11 +++++++++++ 3 files changed, 23 insertions(+), 10 deletions(-) (limited to 'src') diff --git a/src/logging.cpp b/src/logging.cpp index eb9da06d9b..8ec05624ff 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -41,7 +41,7 @@ static int FileWriteStr(const std::string &str, FILE *fp) bool BCLog::Logger::StartLogging() { - std::lock_guard scoped_lock(m_cs); + LockGuard scoped_lock(m_cs); assert(m_buffering); assert(m_fileout == nullptr); @@ -80,7 +80,7 @@ bool BCLog::Logger::StartLogging() void BCLog::Logger::DisconnectTestLogger() { - std::lock_guard scoped_lock(m_cs); + LockGuard scoped_lock(m_cs); m_buffering = true; if (m_fileout != nullptr) fclose(m_fileout); m_fileout = nullptr; @@ -246,7 +246,7 @@ namespace BCLog { void BCLog::Logger::LogPrintStr(const std::string& str) { - std::lock_guard scoped_lock(m_cs); + LockGuard scoped_lock(m_cs); std::string str_prefixed = LogEscapeMessage(str); if (m_log_threadnames && m_started_new_line) { diff --git a/src/logging.h b/src/logging.h index ab07010316..c55f581916 100644 --- a/src/logging.h +++ b/src/logging.h @@ -8,6 +8,7 @@ #include #include +#include #include #include @@ -62,9 +63,10 @@ namespace BCLog { { private: mutable std::mutex m_cs; // Can not use Mutex from sync.h because in debug mode it would cause a deadlock when a potential deadlock was detected - FILE* m_fileout = nullptr; // GUARDED_BY(m_cs) - std::list m_msgs_before_open; // GUARDED_BY(m_cs) - bool m_buffering{true}; //!< Buffer messages before logging can be started. GUARDED_BY(m_cs) + + FILE* m_fileout GUARDED_BY(m_cs) = nullptr; + std::list m_msgs_before_open GUARDED_BY(m_cs); + bool m_buffering GUARDED_BY(m_cs) = true; //!< Buffer messages before logging can be started. /** * m_started_new_line is a state variable that will suppress printing of @@ -79,7 +81,7 @@ namespace BCLog { std::string LogTimestampStr(const std::string& str); /** Slots that connect to the print signal */ - std::list> m_print_callbacks /* GUARDED_BY(m_cs) */ {}; + std::list> m_print_callbacks GUARDED_BY(m_cs) {}; public: bool m_print_to_console = false; @@ -98,14 +100,14 @@ namespace BCLog { /** Returns whether logs will be written to any output */ bool Enabled() const { - std::lock_guard scoped_lock(m_cs); + LockGuard scoped_lock(m_cs); return m_buffering || m_print_to_console || m_print_to_file || !m_print_callbacks.empty(); } /** Connect a slot to the print signal and return the connection */ std::list>::iterator PushBackCallback(std::function fun) { - std::lock_guard scoped_lock(m_cs); + LockGuard scoped_lock(m_cs); m_print_callbacks.push_back(std::move(fun)); return --m_print_callbacks.end(); } @@ -113,7 +115,7 @@ namespace BCLog { /** Delete a connection */ void DeleteCallback(std::list>::iterator it) { - std::lock_guard scoped_lock(m_cs); + LockGuard scoped_lock(m_cs); m_print_callbacks.erase(it); } diff --git a/src/threadsafety.h b/src/threadsafety.h index bb988dfdfd..81f86eac3a 100644 --- a/src/threadsafety.h +++ b/src/threadsafety.h @@ -6,6 +6,8 @@ #ifndef BITCOIN_THREADSAFETY_H #define BITCOIN_THREADSAFETY_H +#include + #ifdef __clang__ // TL;DR Add GUARDED_BY(mutex) to member variables. The others are // rarely necessary. Ex: int nFoo GUARDED_BY(cs_foo); @@ -54,4 +56,13 @@ #define ASSERT_EXCLUSIVE_LOCK(...) #endif // __GNUC__ +// LockGuard provides an annotated version of lock_guard for us +// should only be used when sync.h Mutex/LOCK/etc aren't usable +class SCOPED_LOCKABLE LockGuard : public std::lock_guard +{ +public: + explicit LockGuard(std::mutex& cs) EXCLUSIVE_LOCK_FUNCTION(cs) : std::lock_guard(cs) { } + ~LockGuard() UNLOCK_FUNCTION() {}; +}; + #endif // BITCOIN_THREADSAFETY_H -- cgit v1.2.3