Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make LOCK, LOCK2, TRY_LOCK work with CWaitableCriticalSection #11640

Merged
merged 4 commits into from Aug 31, 2018
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -81,6 +81,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 \
@@ -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 @@ class WorkQueue
/** 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 @@ class WorkQueue
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 @@ class WorkQueue
/** Interrupt and exit loops */
void Interrupt()
{
std::unique_lock<std::mutex> lock(cs);

This comment has been minimized.

Copy link
@laanwj

laanwj Mar 1, 2018

Member

I don't like replacing the standard C++11 lock usage here, and in other places, with the macros. The original code seems easier to understand at least.

Edit: so I get that this helps with the lock dependency checking, but I wonder if there isn't a way that can be done, with keeping the code closer to the original C++11 syntax.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 1, 2018

Author Contributor

I don't like replacing the standard C++11 lock usage here, and in other places, with the macros

It doesn't matter to me which style is used, but the macros provide lock order checking which @TheBlueMatt seemed to think was important. It would be helpful if there could be a guideline for when to use LOCK macros vs standard locks. Is the guideline that you'd recommend just to avoid macros when locks are used with condition variables, or is it broader or narrower than that?

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Mar 2, 2018

Contributor

We're in a not-great state right now where we have automated lock-checking code, but a bunch of places dont bother to use it. In practice most of those places are really limited use and "obviously-correct" (tm), but that only makes it much easier to break in the future, and making exceptions like that are kinda bad. I would strongly prefer we have a consistent syntax across the codebase that uses our lock-checking stuff vs moving back to an ad-hoc world.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 2, 2018

Author Contributor

I would strongly prefer we have a consistent syntax across the codebase that uses our lock-checking stuff vs moving back to an ad-hoc world.

This PR switches code to consistently use LOCK syntax, which Wladimir objects to in some cases (not clear which ones).

@laanwj @TheBlueMatt, it'd be good to resolve the apparent disagreement about when to use lock_guard and when to use LOCK, if possible. But if there can't be any agreement, I could revert the changes in the PR to keep preexisting styles.

This comment has been minimized.

Copy link
@laanwj

laanwj Mar 2, 2018

Member

I agree the lock checking is important, but I dislike the non-standard macro syntax as it hides what is happening. So I wondered if there is an official way to 'decorate' the C++11 locking primitives, as I doubt we're the only project to do checking like this. If not, this change is fine with me...

LOCK(cs);
running = false;
cond.notify_all();
}
@@ -566,7 +566,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();
@@ -1660,7 +1660,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.
@@ -2064,7 +2064,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; });
}
@@ -2346,7 +2346,7 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
flagInterruptMsgProc = false;

{
std::unique_lock<std::mutex> lock(mutexMsgProc);
LOCK(mutexMsgProc);
fMsgProcWake = false;
}

@@ -425,7 +425,7 @@ class CConnman
bool fMsgProcWake;

std::condition_variable condMsgProc;
std::mutex mutexMsgProc;
CWaitableCriticalSection mutexMsgProc;
std::atomic<bool> flagInterruptMsgProc;

CThreadInterrupt interruptNet;
@@ -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;
@@ -49,7 +49,7 @@ struct CUpdatedBlock
int height;
};

static std::mutex cs_blockchange;
static CWaitableCriticalSection cs_blockchange;
static std::condition_variable cond_blockchange;
static CUpdatedBlock latestblock;

@@ -224,7 +224,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(); });
@@ -266,7 +266,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
@@ -309,7 +309,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
@@ -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)
@@ -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 */
@@ -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,92 +96,67 @@ class LOCKABLE AnnotatedMixin : public PARENT
{
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;

/** 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
}

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);
else
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 @@ class SCOPED_LOCKABLE CCriticalBlock

~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__); \

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Dec 5, 2017

Contributor

Isnt it considered bad practice to turn a #define into two statements (asking, I dont know, I've just always seen people bend over to avoid it).

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Dec 7, 2017

Author Contributor

It's bad practice generally because it can break usages like if (cond) MY_MACRO();, and the normal workaround is to wrap the statements in a do {...} while(0). But the concern doesn't apply to variable declarations like this.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Dec 8, 2017

Contributor

Fair enough, I just missed why you changed it, but see it now.

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) \
{ \
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.