Make LOCK, LOCK2, TRY_LOCK work with CWaitableCriticalSection #11640

ryanofsky commented at 9:44 PM on November 8, 2017: member

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

Also add unit test for DEBUG_LOCKORDER code.

ryanofsky force-pushed on Nov 8, 2017

fanquake added the label Refactoring on Nov 8, 2017

in src/sync.cpp:107 in f4ea65a606 outdated

  94 | @@ -95,7 +95,7 @@ static void potential_deadlock_detected(const std::pair<void*, void*>& mismatch,
  95 |          }
  96 |          LogPrintf(" %s\n", i.second.ToString());
  97 |      }
  98 | -    assert(false);
  99 | +    throw std::logic_error("potential deadlock detected");

TheBlueMatt commented at 5:45 PM on November 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 :/).

ryanofsky commented at 6:04 PM on November 9, 2017:

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

TheBlueMatt commented at 6:09 PM on November 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.

ryanofsky commented at 9:42 PM on December 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 commented at 5:52 PM on November 9, 2017: member

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

TheBlueMatt commented at 5:52 PM on November 9, 2017: member

Fails travis due to new lock checking in clang.

ryanofsky force-pushed on Nov 9, 2017

ryanofsky force-pushed on Nov 27, 2017

ryanofsky commented at 9:42 PM on December 1, 2017: member

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

Good catch, added this.

in src/sync.cpp:98 in e8cbbb348f outdated

  94 | @@ -95,7 +95,8 @@ static void potential_deadlock_detected(const std::pair<void*, void*>& mismatch,
  95 |          }
  96 |          LogPrintf(" %s\n", i.second.ToString());
  97 |      }
  98 | -    assert(false);
  99 | +    if (g_debug_lockorder_abort) abort();

TheBlueMatt commented at 4:47 PM on December 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).

ryanofsky commented at 3:58 PM on December 7, 2017:

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

in src/sync.h:84 in e8cbbb348f outdated

  77 | @@ -78,6 +78,13 @@ void LeaveCritical();
  78 |  std::string LocksHeld();
  79 |  void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs);
  80 |  void DeleteLock(void* cs);
  81 | +
  82 | +/**
  83 | + * Call abort() if a potential lock order deadlock bug is detected, instead of
  84 | + * just logging information and throwing a logic_error. Defaults to true.

TheBlueMatt commented at 4:48 PM on December 5, 2017:

something something "Only used for unit testing DEBUG_LOCKORDER."

ryanofsky commented at 4:01 PM on December 7, 2017:

Noted in 9050eff728bd1bbd23aeccfd8cf70b0c999a8e4a

in src/sync.h:102 in 0e218b400f outdated

 103 | +template <typename Mutex>
 104 | +class LockOrderMixin : public Mutex
 105 |  {
 106 |  public:
 107 | -    ~CCriticalSection() {
 108 | +    ~LockOrderMixin() {

TheBlueMatt commented at 5:34 PM on December 5, 2017:

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

ryanofsky commented at 4:01 PM on December 7, 2017:

Done in 396911e0e15129c218634c868acb747e423e88d9.

in src/sync.h:186 in 0e218b400f outdated

 190 | -#define LOCK(cs) CCriticalBlock PASTE2(criticalblock, __COUNTER__)(cs, #cs, __FILE__, __LINE__)
 191 | -#define LOCK2(cs1, cs2) CCriticalBlock criticalblock1(cs1, #cs1, __FILE__, __LINE__), criticalblock2(cs2, #cs2, __FILE__, __LINE__)
 192 | -#define TRY_LOCK(cs, name) CCriticalBlock name(cs, #cs, __FILE__, __LINE__, true)
 193 | +#define LOCK(cs) DebugLock<decltype(cs)> PASTE2(criticalblock, __COUNTER__)(cs, #cs, __FILE__, __LINE__)
 194 | +#define LOCK2(cs1, cs2)                                               \
 195 | +    DebugLock<decltype(cs1)> criticalblock1(cs1, #cs1, __FILE__, __LINE__); \

TheBlueMatt commented at 5:40 PM on December 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).

ryanofsky commented at 4:18 PM on December 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.

TheBlueMatt commented at 3:38 PM on December 8, 2017:

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

TheBlueMatt commented at 5:42 PM on December 5, 2017: member

Awesome, thanks for tackling this.

ryanofsky force-pushed on Dec 7, 2017

ryanofsky commented at 6:47 PM on December 7, 2017: member

Added 2 commits 6e3d96e0aae2040dc0d92adf02c901522bf28289 -> 396911e0e15129c218634c868acb747e423e88d9 (pr/dead.5 -> pr/dead.6, compare) Squashed 396911e0e15129c218634c868acb747e423e88d9 -> f190dd6e5ebf07a75559855d04659502f6e63123 (pr/dead.6 -> pr/dead.7)

ryanofsky commented at 7:07 PM on December 7, 2017: member

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

ryanofsky force-pushed on Dec 7, 2017

TheBlueMatt commented at 3:41 PM on December 8, 2017: member

utACK 674dcfea1d0a5314e39fc9731cb863995ff34ea1

ryanofsky force-pushed on Feb 8, 2018

ryanofsky commented at 1:04 PM on February 8, 2018: member

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

in src/httpserver.cpp:119 in b6c88a148d outdated

 114 | @@ -115,7 +115,7 @@ class WorkQueue
 115 |      /** Interrupt and exit loops */
 116 |      void Interrupt()
 117 |      {
 118 | -        std::unique_lock<std::mutex> lock(cs);

laanwj commented at 10:57 PM on March 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.

ryanofsky commented at 11:17 PM on March 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?

TheBlueMatt commented at 4:18 PM on March 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.

ryanofsky commented at 4:37 PM on March 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.

laanwj commented at 6:07 PM on March 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 commented at 6:17 PM on March 2, 2018: member

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" notifications@github.com 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: https://github.com/bitcoin/bitcoin/pull/11640#discussion_r171920098

ryanofsky commented at 11:08 PM on March 2, 2018: member

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 (https://github.com/bitcoin/bitcoin/pull/11640#discussion_r171920098). 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 commented at 8:13 AM on March 3, 2018: member

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 force-pushed on Apr 10, 2018

ryanofsky commented at 12:46 PM on April 10, 2018: member

Rebased b6c88a148db170367abec036b6c4f2e13104f9aa -> 41dea6a481781e69a1e404a7e3d806f59b47032e (pr/dead.9 -> pr/dead.10) due to conflict with #12926 Rebased 41dea6a481781e69a1e404a7e3d806f59b47032e -> c7c788496ee478064e00b592ac69ac2a23f70e11 (pr/dead.10 -> pr/dead.11) due to conflict with #12743 Rebased c7c788496ee478064e00b592ac69ac2a23f70e11 -> 626de8f3e233ae7c9ffac8b607b21c46b96ccf1e (pr/dead.11 -> pr/dead.12) due to conflict with #13033 Rebased 626de8f3e233ae7c9ffac8b607b21c46b96ccf1e -> 9c4dc597ddc66acfd58a945a5ab11f833731abba (pr/dead.12 -> pr/dead.13) due to conflict with #13423

ryanofsky force-pushed on Apr 13, 2018

ryanofsky force-pushed on Apr 26, 2018

DrahtBot closed this on Jul 22, 2018

DrahtBot commented at 11:51 PM on July 22, 2018: member

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 reopened this on Jul 22, 2018

in src/test/sync_tests.cpp:6 in 626de8f3e2 outdated

   0 | @@ -0,0 +1,52 @@
   1 | +// Copyright (c) 2012-2017 The Bitcoin Core developers
   2 | +// Distributed under the MIT software license, see the accompanying
   3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +#include "sync.h"
   6 | +#include "test/test_bitcoin.h"

MarcoFalke commented at 12:45 AM on July 23, 2018:

Please use #include <sync.h> instead.

DrahtBot added the label Needs rebase on Jul 26, 2018

Add unit test for DEBUG_LOCKORDER code 41b88e9337

MOVEONLY Move AnnotatedMixin declaration

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

ba1f095aad

Make LOCK, LOCK2, TRY_LOCK work with CWaitableCriticalSection

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.

1382913e61

Use LOCK macros for non-recursive locks

Instead of std::unique_lock.

9c4dc597dd

ryanofsky force-pushed on Aug 6, 2018

DrahtBot removed the label Needs rebase on Aug 6, 2018

laanwj commented at 1:50 PM on August 31, 2018: member

utACK 9c4dc597ddc66acfd58a945a5ab11f833731abba

laanwj merged this on Aug 31, 2018

laanwj closed this on Aug 31, 2018

laanwj referenced this in commit 385ad11040 on Aug 31, 2018

kittywhiskers referenced this in commit 3f6ba470ba on Jun 5, 2021

kittywhiskers referenced this in commit c5c3dee308 on Jun 6, 2021

UdjinM6 referenced this in commit 6c4dab2885 on Jun 6, 2021

UdjinM6 referenced this in commit a8aee57447 on Jun 11, 2021

MarcoFalke locked this on Sep 8, 2021