Make LOCK, LOCK2, TRY_LOCK work with CWaitableCriticalSection by ryanofsky · Pull Request #11640 · bitcoin/bitcoin

ryanofsky · 2017-11-08T21:44:04Z

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

Also add unit test for DEBUG_LOCKORDER code.

TheBlueMatt

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

TheBlueMatt · 2017-11-09T17:45:26Z

src/sync.cpp

        LogPrintf(" %s\n", i.second.ToString());
    }
-    assert(false);
+    throw std::logic_error("potential deadlock detected");


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

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

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.

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 · 2017-11-09T17:52:46Z

Fails travis due to new lock checking in clang.

ryanofsky · 2017-12-01T21:42:57Z

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

Good catch, added this.

TheBlueMatt

Awesome, thanks for tackling this.

TheBlueMatt · 2017-12-05T16:47:33Z

src/sync.cpp

        LogPrintf(" %s\n", i.second.ToString());
    }
-    assert(false);
+    if (g_debug_lockorder_abort) abort();


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

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

TheBlueMatt · 2017-12-05T16:48:48Z

src/sync.h

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


something something "Only used for unit testing DEBUG_LOCKORDER."

Noted in 9050eff

TheBlueMatt · 2017-12-05T17:34:42Z

src/sync.h

 {
 public:
-    ~CCriticalSection() {
+    ~LockOrderMixin() {


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

Done in 396911e.

TheBlueMatt · 2017-12-05T17:40:28Z

src/sync.h

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


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

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.

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

ryanofsky

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

ryanofsky · 2017-12-07T15:58:51Z

src/sync.cpp

        LogPrintf(" %s\n", i.second.ToString());
    }
-    assert(false);
+    if (g_debug_lockorder_abort) abort();


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

ryanofsky · 2017-12-07T16:01:42Z

src/sync.h

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


Noted in 9050eff

ryanofsky · 2017-12-07T16:01:56Z

src/sync.h

 {
 public:
-    ~CCriticalSection() {
+    ~LockOrderMixin() {


Done in 396911e.

ryanofsky · 2017-12-07T16:18:15Z

src/sync.h

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


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 · 2017-12-07T19:07:02Z

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

TheBlueMatt · 2017-12-08T15:41:04Z

utACK 674dcfe

ryanofsky · 2018-02-08T13:04:53Z

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

laanwj · 2018-03-01T22:57:01Z

src/httpserver.cpp

    /** Interrupt and exit loops */
    void Interrupt()
    {
-        std::unique_lock<std::mutex> lock(cs);


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.

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?

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.

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.

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 · 2018-03-02T18:17:01Z

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

…

On March 2, 2018 6:08:09 PM UTC, "Wladimir J. van der Laan" ***@***.***> wrote: laanwj commented on this pull request. > @@ -115,7 +115,7 @@ class WorkQueue /** Interrupt and exit loops */ void Interrupt() { - std::unique_lock<std::mutex> lock(cs); 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... -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #11640 (comment)

ryanofsky · 2018-03-02T23:08:00Z

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 · 2018-03-03T08:13:48Z

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 · 2018-04-10T12:46:10Z

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 · 2018-07-22T23:51:01Z

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.

maflcko · 2018-07-23T00:45:59Z

src/test/sync_tests.cpp

+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include "sync.h"
+#include "test/test_bitcoin.h"


Please use #include <sync.h> instead.

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 · 2018-08-31T13:50:34Z

utACK 9c4dc59

…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

…iticalSection

merge bitcoin#11640, bitcoin#11599, bitcoin#16112, bitcoin#16127, bitcoin#18635, bitcoin#19249: thread safety and locking improvements

ryanofsky force-pushed the pr/dead branch from 5938e54 to f4ea65a Compare November 8, 2017 21:46

fanquake added the Refactoring label Nov 8, 2017

TheBlueMatt reviewed Nov 9, 2017

View reviewed changes

ryanofsky force-pushed the pr/dead branch from f4ea65a to 1e71f3c Compare November 9, 2017 18:59

TheBlueMatt mentioned this pull request Nov 9, 2017

scripted-diff: Small locking rename #11599

Merged

ryanofsky force-pushed the pr/dead branch 2 times, most recently from fa4d6b0 to 6e3d96e Compare November 27, 2017 23:55

TheBlueMatt reviewed Dec 5, 2017

View reviewed changes

ryanofsky force-pushed the pr/dead branch from 6e3d96e to f190dd6 Compare December 7, 2017 18:42

ryanofsky commented Dec 7, 2017

View reviewed changes

ryanofsky force-pushed the pr/dead branch from f190dd6 to 674dcfe Compare December 7, 2017 19:07

ryanofsky mentioned this pull request Feb 8, 2018

Make CWallet::ListCoins atomic #12333

Merged

ryanofsky force-pushed the pr/dead branch from 674dcfe to b6c88a1 Compare February 8, 2018 13:03

laanwj reviewed Mar 1, 2018

View reviewed changes

ryanofsky force-pushed the pr/dead branch from b6c88a1 to 41dea6a Compare April 10, 2018 12:45

ryanofsky force-pushed the pr/dead branch from 41dea6a to c7c7884 Compare April 13, 2018 17:19

ryanofsky force-pushed the pr/dead branch from c7c7884 to 626de8f Compare April 26, 2018 01:59

ryanofsky mentioned this pull request May 18, 2018

Consistently bounds-check vin/vout access #13268

Closed

DrahtBot mentioned this pull request Jun 19, 2018

[net] Thread safety annotations in net_processing #13423

Merged

DrahtBot closed this Jul 22, 2018

DrahtBot reopened this Jul 22, 2018

maflcko reviewed Jul 23, 2018

View reviewed changes

DrahtBot added the Needs rebase label Jul 26, 2018

ryanofsky added 4 commits August 3, 2018 07:11

Add unit test for DEBUG_LOCKORDER code

41b88e9

MOVEONLY Move AnnotatedMixin declaration

ba1f095

Move AnnotatedMixin closer to where it's used, and after the DEBUG_LOCKORDER function declarations so it can call them.

Make LOCK, LOCK2, TRY_LOCK work with CWaitableCriticalSection

1382913

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.

Use LOCK macros for non-recursive locks

9c4dc59

Instead of std::unique_lock.

ryanofsky force-pushed the pr/dead branch from 626de8f to 9c4dc59 Compare August 6, 2018 16:17

DrahtBot removed the Needs rebase label Aug 6, 2018

laanwj merged commit 9c4dc59 into bitcoin:master Aug 31, 2018

Bushstar mentioned this pull request Sep 4, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#359

Merged

maflcko mentioned this pull request Aug 23, 2019

test: boost unit tests don't handle assert very well #16700

Closed

str4d mentioned this pull request Apr 12, 2021

Sync backports zcash/zcash#5057

Merged

kwvg mentioned this pull request Jun 5, 2021

merge #11640, #11599, #16112, #16127, #18635, #19249: thread safety and locking improvements dashpay/dash#4190

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Jun 6, 2021

merge bitcoin#11640: Make LOCK, LOCK2, TRY_LOCK work with CWaitableCr…

c5c3dee

…iticalSection

UdjinM6 added a commit to dashpay/dash that referenced this pull request Jun 11, 2021

Merge pull request #4190 from kittywhiskers/tlocks

a8aee57

merge bitcoin#11640, bitcoin#11599, bitcoin#16112, bitcoin#16127, bitcoin#18635, bitcoin#19249: thread safety and locking improvements

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

ryanofsky commented Nov 8, 2017 • edited by DrahtBot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Nov 9, 2017

Uh oh!

ryanofsky commented Dec 1, 2017

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Dec 8, 2017

Uh oh!

ryanofsky commented Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Mar 2, 2018 via email

Uh oh!

ryanofsky commented Mar 2, 2018

Uh oh!

laanwj commented Mar 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ryanofsky commented Nov 8, 2017 •

edited by DrahtBot

Loading

ryanofsky commented Dec 7, 2017 •

edited

Loading

ryanofsky commented Feb 8, 2018 •

edited

Loading

laanwj Mar 1, 2018 •

edited

Loading

laanwj commented Mar 3, 2018 •

edited

Loading

ryanofsky commented Apr 10, 2018 •

edited

Loading