Skip to content

Commit a8aee57

Browse files
UdjinM6UdjinM6
authored
Merge pull request dashpay#4190 from kittywhiskers/tlocks
merge bitcoin#11640, bitcoin#11599, bitcoin#16112, bitcoin#16127, bitcoin#18635, bitcoin#19249: thread safety and locking improvements
2 parents d444752 + cbc6186 commit a8aee57

26 files changed

+304
-173
lines changed

‎configure.ac

+2-2
Original file line numberDiff line numberDiff line change
@@ -362,7 +362,7 @@ if test "x$enable_werror" = "xyes"; then
362362
AC_MSG_ERROR("enable-werror set but -Werror is not usable")
363363
fi
364364
AX_CHECK_COMPILE_FLAG([-Werror=vla],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=vla"],,[[$CXXFLAG_WERROR]])
365-
AX_CHECK_COMPILE_FLAG([-Werror=thread-safety-analysis],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=thread-safety-analysis"],,[[$CXXFLAG_WERROR]])
365+
AX_CHECK_COMPILE_FLAG([-Werror=thread-safety],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=thread-safety"],,[[$CXXFLAG_WERROR]])
366366
fi
367367

368368
if test "x$CXXFLAGS_overridden" = "xno"; then
@@ -371,7 +371,7 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
371371
AX_CHECK_COMPILE_FLAG([-Wformat],[CXXFLAGS="$CXXFLAGS -Wformat"],,[[$CXXFLAG_WERROR]])
372372
AX_CHECK_COMPILE_FLAG([-Wvla],[CXXFLAGS="$CXXFLAGS -Wvla"],,[[$CXXFLAG_WERROR]])
373373
AX_CHECK_COMPILE_FLAG([-Wformat-security],[CXXFLAGS="$CXXFLAGS -Wformat-security"],,[[$CXXFLAG_WERROR]])
374-
AX_CHECK_COMPILE_FLAG([-Wthread-safety-analysis],[CXXFLAGS="$CXXFLAGS -Wthread-safety-analysis"],,[[$CXXFLAG_WERROR]])
374+
AX_CHECK_COMPILE_FLAG([-Wthread-safety],[CXXFLAGS="$CXXFLAGS -Wthread-safety"],,[[$CXXFLAG_WERROR]])
375375
AX_CHECK_COMPILE_FLAG([-Wrange-loop-analysis],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wrange-loop-analysis"],,[[$CXXFLAG_WERROR]])
376376

377377
## Some compilers (gcc) ignore unknown -Wno-* options, but warn about all

‎src/Makefile.test.include

+1
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@ BITCOIN_TESTS =\
101101
test/skiplist_tests.cpp \
102102
test/streams_tests.cpp \
103103
test/subsidy_tests.cpp \
104+
test/sync_tests.cpp \
104105
test/timedata_tests.cpp \
105106
test/torcontrol_tests.cpp \
106107
test/transaction_tests.cpp \

‎src/blockencodings.cpp

+3-4
Original file line numberDiff line numberDiff line change
@@ -106,13 +106,12 @@ ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& c
106106
std::vector<bool> have_txn(txn_available.size());
107107
{
108108
LOCK(pool->cs);
109-
const std::vector<std::pair<uint256, CTxMemPool::txiter> >& vTxHashes = pool->vTxHashes;
110-
for (size_t i = 0; i < vTxHashes.size(); i++) {
111-
uint64_t shortid = cmpctblock.GetShortID(vTxHashes[i].first);
109+
for (size_t i = 0; i < pool->vTxHashes.size(); i++) {
110+
uint64_t shortid = cmpctblock.GetShortID(pool->vTxHashes[i].first);
112111
std::unordered_map<uint64_t, uint16_t>::iterator idit = shorttxids.find(shortid);
113112
if (idit != shorttxids.end()) {
114113
if (!have_txn[idit->second]) {
115-
txn_available[idit->second] = vTxHashes[i].second->GetSharedTx();
114+
txn_available[idit->second] = pool->vTxHashes[i].second->GetSharedTx();
116115
have_txn[idit->second] = true;
117116
mempool_count++;
118117
} else {

‎src/blockencodings.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ class PartiallyDownloadedBlock {
127127
protected:
128128
std::vector<CTransactionRef> txn_available;
129129
size_t prefilled_count = 0, mempool_count = 0, extra_count = 0;
130-
CTxMemPool* pool;
130+
const CTxMemPool* pool;
131131
public:
132132
CBlockHeader header;
133133
explicit PartiallyDownloadedBlock(CTxMemPool* poolIn) : pool(poolIn) {}

‎src/httpserver.cpp

+4-4
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ class WorkQueue
7474
{
7575
private:
7676
/** Mutex protects entire object */
77-
std::mutex cs;
77+
Mutex cs;
7878
std::condition_variable cond;
7979
std::deque<std::unique_ptr<WorkItem>> queue;
8080
bool running;
@@ -93,7 +93,7 @@ class WorkQueue
9393
/** Enqueue a work item */
9494
bool Enqueue(WorkItem* item)
9595
{
96-
std::unique_lock<std::mutex> lock(cs);
96+
LOCK(cs);
9797
if (queue.size() >= maxDepth) {
9898
return false;
9999
}
@@ -107,7 +107,7 @@ class WorkQueue
107107
while (true) {
108108
std::unique_ptr<WorkItem> i;
109109
{
110-
std::unique_lock<std::mutex> lock(cs);
110+
WAIT_LOCK(cs, lock);
111111
while (running && queue.empty())
112112
cond.wait(lock);
113113
if (!running)
@@ -121,7 +121,7 @@ class WorkQueue
121121
/** Interrupt and exit loops */
122122
void Interrupt()
123123
{
124-
std::unique_lock<std::mutex> lock(cs);
124+
LOCK(cs);
125125
running = false;
126126
cond.notify_all();
127127
}

‎src/init.cpp

+10-16
Original file line numberDiff line numberDiff line change
@@ -702,17 +702,17 @@ static void BlockNotifyCallback(bool initialSync, const CBlockIndex *pBlockIndex
702702
}
703703

704704
static bool fHaveGenesis = false;
705-
static CWaitableCriticalSection cs_GenesisWait;
706-
static CConditionVariable condvar_GenesisWait;
705+
static Mutex g_genesis_wait_mutex;
706+
static std::condition_variable g_genesis_wait_cv;
707707

708708
static void BlockNotifyGenesisWait(bool, const CBlockIndex *pBlockIndex)
709709
{
710710
if (pBlockIndex != nullptr) {
711711
{
712-
WaitableLock lock_GenesisWait(cs_GenesisWait);
712+
LOCK(g_genesis_wait_mutex);
713713
fHaveGenesis = true;
714714
}
715-
condvar_GenesisWait.notify_all();
715+
g_genesis_wait_cv.notify_all();
716716
}
717717
}
718718

@@ -1089,12 +1089,6 @@ void InitLogging()
10891089
{
10901090
g_logger->m_print_to_file = !gArgs.IsArgNegated("-debuglogfile");
10911091
g_logger->m_file_path = AbsPathForConfigVal(gArgs.GetArg("-debuglogfile", DEFAULT_DEBUGLOGFILE));
1092-
1093-
// Add newlines to the logfile to distinguish this execution from the last
1094-
// one; called before console logging is set up, so this is only sent to
1095-
// debug.log.
1096-
LogPrintf("\n\n\n\n\n");
1097-
10981092
g_logger->m_print_to_console = gArgs.GetBoolArg("-printtoconsole", !gArgs.GetBoolArg("-daemon", false));
10991093
g_logger->m_log_timestamps = gArgs.GetBoolArg("-logtimestamps", DEFAULT_LOGTIMESTAMPS);
11001094
g_logger->m_log_time_micros = gArgs.GetBoolArg("-logtimemicros", DEFAULT_LOGTIMEMICROS);
@@ -1709,10 +1703,10 @@ bool AppInitMain()
17091703
// and because this needs to happen before any other debug.log printing
17101704
g_logger->ShrinkDebugFile();
17111705
}
1712-
if (!g_logger->OpenDebugLog()) {
1713-
return InitError(strprintf("Could not open debug log file %s",
1714-
g_logger->m_file_path.string()));
1715-
}
1706+
}
1707+
if (!g_logger->StartLogging()) {
1708+
return InitError(strprintf("Could not open debug log file %s",
1709+
g_logger->m_file_path.string()));
17161710
}
17171711

17181712
if (!g_logger->m_log_timestamps)
@@ -2377,12 +2371,12 @@ bool AppInitMain()
23772371

23782372
// Wait for genesis block to be processed
23792373
{
2380-
WaitableLock lock(cs_GenesisWait);
2374+
WAIT_LOCK(g_genesis_wait_mutex, lock);
23812375
// We previously could hang here if StartShutdown() is called prior to
23822376
// ThreadImport getting started, so instead we just wait on a timer to
23832377
// check ShutdownRequested() regularly.
23842378
while (!fHaveGenesis && !ShutdownRequested()) {
2385-
condvar_GenesisWait.wait_for(lock, std::chrono::milliseconds(500));
2379+
g_genesis_wait_cv.wait_for(lock, std::chrono::milliseconds(500));
23862380
}
23872381
uiInterface.NotifyBlockTip.disconnect(BlockNotifyGenesisWait);
23882382
}

‎src/logging.cpp

+41-27
Original file line numberDiff line numberDiff line change
@@ -31,24 +31,38 @@ static int FileWriteStr(const std::string &str, FILE *fp)
3131
return fwrite(str.data(), 1, str.size(), fp);
3232
}
3333

34-
bool BCLog::Logger::OpenDebugLog()
34+
bool BCLog::Logger::StartLogging()
3535
{
36-
std::lock_guard<std::mutex> scoped_lock(m_file_mutex);
36+
StdLockGuard scoped_lock(m_cs);
3737

38+
assert(m_buffering);
3839
assert(m_fileout == nullptr);
39-
assert(!m_file_path.empty());
4040

41-
m_fileout = fsbridge::fopen(m_file_path, "a");
42-
if (!m_fileout) {
43-
return false;
41+
if (m_print_to_file) {
42+
assert(!m_file_path.empty());
43+
m_fileout = fsbridge::fopen(m_file_path, "a");
44+
if (!m_fileout) {
45+
return false;
46+
}
47+
48+
setbuf(m_fileout, nullptr); // unbuffered
49+
50+
// Add newlines to the logfile to distinguish this execution from the
51+
// last one.
52+
FileWriteStr("\n\n\n\n\n", m_fileout);
4453
}
4554

46-
setbuf(m_fileout, nullptr); // unbuffered
4755
// dump buffered messages from before we opened the log
56+
m_buffering = false;
4857
while (!m_msgs_before_open.empty()) {
49-
FileWriteStr(m_msgs_before_open.front(), m_fileout);
58+
const std::string& s = m_msgs_before_open.front();
59+
60+
if (m_print_to_file) FileWriteStr(s, m_fileout);
61+
if (m_print_to_console) fwrite(s.data(), 1, s.size(), stdout);
62+
5063
m_msgs_before_open.pop_front();
5164
}
65+
if (m_print_to_console) fflush(stdout);
5266

5367
return true;
5468
}
@@ -247,8 +261,9 @@ std::string BCLog::Logger::LogThreadNameStr(const std::string &str)
247261
return strThreadLogged;
248262
}
249263

250-
void BCLog::Logger::LogPrintStr(const std::string &str)
264+
void BCLog::Logger::LogPrintStr(const std::string& str)
251265
{
266+
StdLockGuard scoped_lock(m_cs);
252267
std::string strThreadLogged = LogThreadNameStr(str);
253268
std::string strTimestamped = LogTimestampStr(strThreadLogged);
254269

@@ -257,32 +272,31 @@ void BCLog::Logger::LogPrintStr(const std::string &str)
257272
else
258273
m_started_new_line = false;
259274

275+
if (m_buffering) {
276+
// buffer if we haven't started logging yet
277+
m_msgs_before_open.push_back(strTimestamped);
278+
return;
279+
}
280+
260281
if (m_print_to_console) {
261282
// print to console
262283
fwrite(strTimestamped.data(), 1, strTimestamped.size(), stdout);
263284
fflush(stdout);
264285
}
265286
if (m_print_to_file) {
266-
std::lock_guard<std::mutex> scoped_lock(m_file_mutex);
267-
268-
// buffer if we haven't opened the log yet
269-
if (m_fileout == nullptr) {
270-
m_msgs_before_open.push_back(strTimestamped);
271-
}
272-
else
273-
{
274-
// reopen the log file, if requested
275-
if (m_reopen_file) {
276-
m_reopen_file = false;
277-
m_fileout = fsbridge::freopen(m_file_path, "a", m_fileout);
278-
if (!m_fileout) {
279-
return;
280-
}
281-
setbuf(m_fileout, nullptr); // unbuffered
287+
assert(m_fileout != nullptr);
288+
289+
// reopen the log file, if requested
290+
if (m_reopen_file) {
291+
m_reopen_file = false;
292+
FILE* new_fileout = fsbridge::fopen(m_file_path, "a");
293+
if (new_fileout) {
294+
setbuf(new_fileout, nullptr); // unbuffered
295+
fclose(m_fileout);
296+
m_fileout = new_fileout;
282297
}
283-
284-
FileWriteStr(strTimestamped, m_fileout);
285298
}
299+
FileWriteStr(strTimestamped, m_fileout);
286300
}
287301
}
288302

‎src/logging.h

+15-6
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
#include <fs.h>
1010
#include <tinyformat.h>
11+
#include <threadsafety.h>
1112

1213
#include <atomic>
1314
#include <cstdint>
@@ -82,9 +83,11 @@ namespace BCLog {
8283
class Logger
8384
{
8485
private:
85-
FILE* m_fileout = nullptr;
86-
std::mutex m_file_mutex;
87-
std::list<std::string> m_msgs_before_open;
86+
mutable StdMutex m_cs; // Can not use Mutex from sync.h because in debug mode it would cause a deadlock when a potential deadlock was detected
87+
88+
FILE* m_fileout GUARDED_BY(m_cs) = nullptr;
89+
std::list<std::string> m_msgs_before_open GUARDED_BY(m_cs);
90+
bool m_buffering GUARDED_BY(m_cs) = true; //!< Buffer messages before logging can be started.
8891

8992
/**
9093
* m_started_new_line is a state variable that will suppress printing of
@@ -111,12 +114,18 @@ namespace BCLog {
111114
std::atomic<bool> m_reopen_file{false};
112115

113116
/** Send a string to the log output */
114-
void LogPrintStr(const std::string &str);
117+
void LogPrintStr(const std::string& str);
115118

116119
/** Returns whether logs will be written to any output */
117-
bool Enabled() const { return m_print_to_console || m_print_to_file; }
120+
bool Enabled() const
121+
{
122+
StdLockGuard scoped_lock(m_cs);
123+
return m_buffering || m_print_to_console || m_print_to_file;
124+
}
125+
126+
/** Start logging (and flush all buffered messages) */
127+
bool StartLogging();
118128

119-
bool OpenDebugLog();
120129
void ShrinkDebugFile();
121130

122131
uint64_t GetCategoryMask() const { return m_categories.load(); }

‎src/net.cpp

+5-5
Original file line numberDiff line numberDiff line change
@@ -2040,7 +2040,7 @@ void CConnman::ThreadSocketHandler()
20402040
void CConnman::WakeMessageHandler()
20412041
{
20422042
{
2043-
std::lock_guard<std::mutex> lock(mutexMsgProc);
2043+
LOCK(mutexMsgProc);
20442044
fMsgProcWake = true;
20452045
}
20462046
condMsgProc.notify_one();
@@ -2862,9 +2862,9 @@ void CConnman::ThreadMessageHandler()
28622862

28632863
ReleaseNodeVector(vNodesCopy);
28642864

2865-
std::unique_lock<std::mutex> lock(mutexMsgProc);
2865+
WAIT_LOCK(mutexMsgProc, lock);
28662866
if (!fMoreWork) {
2867-
condMsgProc.wait_until(lock, std::chrono::steady_clock::now() + std::chrono::milliseconds(100), [this] { return fMsgProcWake; });
2867+
condMsgProc.wait_until(lock, std::chrono::steady_clock::now() + std::chrono::milliseconds(100), [this]() EXCLUSIVE_LOCKS_REQUIRED(mutexMsgProc) { return fMsgProcWake; });
28682868
}
28692869
fMsgProcWake = false;
28702870
}
@@ -3198,7 +3198,7 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
31983198
flagInterruptMsgProc = false;
31993199

32003200
{
3201-
std::unique_lock<std::mutex> lock(mutexMsgProc);
3201+
LOCK(mutexMsgProc);
32023202
fMsgProcWake = false;
32033203
}
32043204

@@ -3303,7 +3303,7 @@ void CExplicitNetCleanup::callCleanup()
33033303
void CConnman::Interrupt()
33043304
{
33053305
{
3306-
std::lock_guard<std::mutex> lock(mutexMsgProc);
3306+
LOCK(mutexMsgProc);
33073307
flagInterruptMsgProc = true;
33083308
}
33093309
condMsgProc.notify_all();

‎src/net.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -617,10 +617,10 @@ friend class CNode;
617617
const uint64_t nSeed0, nSeed1;
618618

619619
/** flag for waking the message processor. */
620-
bool fMsgProcWake;
620+
bool fMsgProcWake GUARDED_BY(mutexMsgProc);
621621

622622
std::condition_variable condMsgProc;
623-
std::mutex mutexMsgProc;
623+
Mutex mutexMsgProc;
624624
std::atomic<bool> flagInterruptMsgProc;
625625

626626
CThreadInterrupt interruptNet;

‎src/random.cpp

+4-3
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 Mutex 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;

0 commit comments

Comments
 (0)