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

Conversation

@ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Nov 8, 2017

Make LOCK macros work with non-recursive mutexes, and use wherever possible for better deadlock detection.

Also add unit test for DEBUG_LOCKORDER code.

Copy link
Contributor

@TheBlueMatt TheBlueMatt left a comment

Dont you need a similar DeleteLock() call as CCriticalSection?

src/sync.cpp Outdated
@@ -95,7 +95,7 @@ static void potential_deadlock_detected(const std::pair<void*, void*>& mismatch,
}
LogPrintf(" %s\n", i.second.ToString());
}
assert(false);
throw std::logic_error("potential deadlock detected");
Copy link
Contributor

@TheBlueMatt TheBlueMatt Nov 9, 2017

I believe we would sometimes incorrectly catch this - we still have some generic catch blocks lying around in places :(. I wouldn't mind having a policy of only catching in tests, but until then, I think this should, sadly, remain an assert (unless you want to go to the hassle of adding some #define that is only set in test_bitcoin and compiling this unit differently for it :/).

Copy link
Contributor Author

@ryanofsky ryanofsky Nov 9, 2017

What exactly is the problem? DEBUG_LOCKORDER isn't enabled unless you are doing a special build anyway.

Copy link
Contributor

@TheBlueMatt TheBlueMatt Nov 9, 2017

I presume it wouldn't actually fail in some cases where a lockorder inversion is hit as the exception here would be caught. Precisely because its (essentially) only enabled in travis we should just keep an assert.

Copy link
Contributor Author

@ryanofsky ryanofsky Dec 1, 2017

I presume it wouldn't actually fail in some cases where a lockorder inversion is hit as the exception here would be caught. Precisely because its (essentially) only enabled in travis we should just keep an assert.

Makes sense, added back an optional abort (on by default), so it will still be guaranteed to fail travis, even in the presence of code that would catch std::logic_error exceptions.

@TheBlueMatt
Copy link
Contributor

@TheBlueMatt TheBlueMatt commented Nov 9, 2017

Fails travis due to new lock checking in clang.

@ryanofsky
Copy link
Contributor Author

@ryanofsky ryanofsky commented Dec 1, 2017

Dont you need a similar DeleteLock() call as CCriticalSection?

Good catch, added this.

Copy link
Contributor

@TheBlueMatt TheBlueMatt left a comment

Awesome, thanks for tackling this.

src/sync.cpp Outdated
@@ -95,7 +95,8 @@ static void potential_deadlock_detected(const std::pair<void*, void*>& mismatch,
}
LogPrintf(" %s\n", i.second.ToString());
}
assert(false);
if (g_debug_lockorder_abort) abort();
Copy link
Contributor

@TheBlueMatt TheBlueMatt Dec 5, 2017

I believe abort() doesnt give the same useful stderr print as assert(false) does (specifically giving you a line number so you know at least where the crash was).

Copy link
Contributor Author

@ryanofsky ryanofsky Dec 7, 2017

Addressed in 9050eff. (I added fprintf() before the abort following the pattern from AssertLockHeldInternal/AssertLockNotHeldInternal. I can switch to assert though if you prefer that.)

src/sync.h Outdated

/**
* Call abort() if a potential lock order deadlock bug is detected, instead of
* just logging information and throwing a logic_error. Defaults to true.
Copy link
Contributor

@TheBlueMatt TheBlueMatt Dec 5, 2017

something something "Only used for unit testing DEBUG_LOCKORDER."

Copy link
Contributor Author

@ryanofsky ryanofsky Dec 7, 2017

Noted in 9050eff

src/sync.h Outdated
{
public:
~CCriticalSection() {
~LockOrderMixin() {
Copy link
Contributor

@TheBlueMatt TheBlueMatt Dec 5, 2017

Maybe just move this into AnnotatedMixin instead of making everything two classes on top of each other?

Copy link
Contributor Author

@ryanofsky ryanofsky Dec 7, 2017

Done in 396911e.

#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__); \
Copy link
Contributor

@TheBlueMatt TheBlueMatt Dec 5, 2017

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).

Copy link
Contributor Author

@ryanofsky ryanofsky Dec 7, 2017

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.

Copy link
Contributor

@TheBlueMatt TheBlueMatt Dec 8, 2017

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

Copy link
Contributor Author

@ryanofsky ryanofsky left a comment

Added 2 commits 6e3d96e -> 396911e (pr/dead.5 -> pr/dead.6, compare)
Squashed 396911e -> f190dd6 (pr/dead.6 -> pr/dead.7)

src/sync.cpp Outdated
@@ -95,7 +95,8 @@ static void potential_deadlock_detected(const std::pair<void*, void*>& mismatch,
}
LogPrintf(" %s\n", i.second.ToString());
}
assert(false);
if (g_debug_lockorder_abort) abort();
Copy link
Contributor Author

@ryanofsky ryanofsky Dec 7, 2017

Addressed in 9050eff. (I added fprintf() before the abort following the pattern from AssertLockHeldInternal/AssertLockNotHeldInternal. I can switch to assert though if you prefer that.)

src/sync.h Outdated

/**
* Call abort() if a potential lock order deadlock bug is detected, instead of
* just logging information and throwing a logic_error. Defaults to true.
Copy link
Contributor Author

@ryanofsky ryanofsky Dec 7, 2017

Noted in 9050eff

src/sync.h Outdated
{
public:
~CCriticalSection() {
~LockOrderMixin() {
Copy link
Contributor Author

@ryanofsky ryanofsky Dec 7, 2017

Done in 396911e.

#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__); \
Copy link
Contributor Author

@ryanofsky ryanofsky Dec 7, 2017

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.

@ryanofsky
Copy link
Contributor Author

@ryanofsky ryanofsky commented Dec 7, 2017

Rebased f190dd6 -> 674dcfe (pr/dead.7 -> pr/dead.8) due to new changes conflicting with AssertLockNotHeld declaration from #10286

@TheBlueMatt
Copy link
Contributor

@TheBlueMatt TheBlueMatt commented Dec 8, 2017

utACK 674dcfe

@ryanofsky
Copy link
Contributor Author

@ryanofsky ryanofsky commented Feb 8, 2018

Rebased 674dcfe -> b6c88a1 (pr/dead.8 -> pr/dead.9) due to trivial conflicts with #12366 and #12367.

@@ -115,7 +115,7 @@ class WorkQueue
/** Interrupt and exit loops */
void Interrupt()
{
std::unique_lock<std::mutex> lock(cs);
Copy link
Member

@laanwj laanwj Mar 1, 2018

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.

Copy link
Contributor Author

@ryanofsky ryanofsky Mar 1, 2018

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?

Copy link
Contributor

@TheBlueMatt TheBlueMatt Mar 2, 2018

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.

Copy link
Contributor Author

@ryanofsky ryanofsky Mar 2, 2018

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.

Copy link
Member

@laanwj laanwj Mar 2, 2018

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...

@TheBlueMatt
Copy link
Contributor

@TheBlueMatt TheBlueMatt commented Mar 2, 2018

@ryanofsky
Copy link
Contributor Author

@ryanofsky ryanofsky commented Mar 2, 2018

Yes, I tend to agree with that point. I think just explicitly using our wrapper classes instead of the LOCK macros would suffice there.

Matt, maybe I'm misunderstanding, but are you suggesting that I write something like:

DebugLock<CWaitableCriticalSection> lock(cs, "cs", __FILE__, __LINE__);

to replace current master code:

std::unique_lock<std::mutex> lock(cs);

instead of current PR code:

LOCK(cs);

If this is the case, I guess I'd prefer to keep the current PR code, since it just uses one style (LOCK) uniformly and it seems Wladimir no longer objects (#11640 (comment)). In the future, if we want to avoid using LOCK in some cases without replacing it everywhere, having a documented developer guideline would probably be helpful.

@laanwj
Copy link
Member

@laanwj laanwj commented Mar 3, 2018

Yes, I agree, my comment was somehow out of scope. Let's leave changing the style to something in the future. Or maybe just documenting how the various OS/C++ synchronization primitives map to "bitcoin core style" and vice versa.

@ryanofsky
Copy link
Contributor Author

@ryanofsky ryanofsky commented Apr 10, 2018

Rebased b6c88a1 -> 41dea6a (pr/dead.9 -> pr/dead.10) due to conflict with #12926
Rebased 41dea6a -> c7c7884 (pr/dead.10 -> pr/dead.11) due to conflict with #12743
Rebased c7c7884 -> 626de8f (pr/dead.11 -> pr/dead.12) due to conflict with #13033
Rebased 626de8f -> 9c4dc59 (pr/dead.12 -> pr/dead.13) due to conflict with #13423

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Jul 22, 2018

The last travis run for this pull request was 87 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

@DrahtBot DrahtBot reopened this Jul 22, 2018
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include "sync.h"
#include "test/test_bitcoin.h"
Copy link
Member

@MarcoFalke MarcoFalke Jul 23, 2018

Please use #include <sync.h> instead.

ryanofsky added 4 commits Aug 3, 2018
Move AnnotatedMixin closer to where it's used, and after the DEBUG_LOCKORDER
function declarations so it can call them.
They should also work with any other mutex type which std::unique_lock
supports.

There is no change in behavior for current code that calls these macros with
CCriticalSection mutexes.
Instead of std::unique_lock.
@laanwj
Copy link
Member

@laanwj laanwj commented Aug 31, 2018

utACK 9c4dc59

@laanwj laanwj merged commit 9c4dc59 into bitcoin:master Aug 31, 2018
1 check passed
laanwj added a commit that referenced this issue Aug 31, 2018
…ection

9c4dc59 Use LOCK macros for non-recursive locks (Russell Yanofsky)
1382913 Make LOCK, LOCK2, TRY_LOCK work with CWaitableCriticalSection (Russell Yanofsky)
ba1f095 MOVEONLY Move AnnotatedMixin declaration (Russell Yanofsky)
41b88e9 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
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Jun 5, 2021
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Jun 6, 2021
UdjinM6 added a commit to UdjinM6/dash that referenced this issue Jun 6, 2021
UdjinM6 added a commit to dashpay/dash that referenced this issue Jun 11, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants