Preserve the `LockData` initial state if "potential deadlock detected" exception thrown by hebasto · Pull Request #19340 · bitcoin/bitcoin

hebasto · 2020-06-20T20:26:35Z

On master (e3fa3c7) if the push_lock() throws the "potential deadlock detected" exception (via the potential_deadlock_detected() call), the LockData instance internal state differs from one when the push_lock() was called. This non-well behaviour makes (at least) testing brittle.

This PR preserves the LockData instance initial state if push_lock() throws an exception, and improves the sync_tests unit test.

DrahtBot · 2020-06-20T22:03:56Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

sync: detect double lock from the same thread #19337 (sync: detect double lock from the same thread by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

vasild

It would be good if the offending entry is still in the lock stack by the time potential_deadlock_detected() is called because it prints the stack and we want to see the offender in the debug log.

Also, I think that it would be best to undo all mods before abort()/throw. We also inserted into lockdata.lockorders and in lockdata.invlockorders. Maybe something like:

diff --git i/src/sync.cpp w/src/sync.cpp
index 3098e402c..b21154d6e 100644
--- i/src/sync.cpp
+++ w/src/sync.cpp
@@ -124,17 +124,12 @@ static void potential_deadlock_detected(const LockPair& mismatch, const LockStac
         }
         if (i.first == mismatch.second) {
             LogPrintf(" (2)"); /* Continued */
         }
         LogPrintf(" %s\n", i.second.ToString());
     }
-    if (g_debug_lockorder_abort) {
-        tfm::format(std::cerr, "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 double_lock_detected(const void* mutex, LockStack& lock_stack)
 {
     LogPrintf("DOUBLE LOCK DETECTED\n");
     LogPrintf("Lock order:\n");
@@ -186,14 +181,28 @@ static void push_lock(MutexType* c, const CLockLocation& locklocation)
         if (lockdata.lockorders.count(p1))
             continue;
         lockdata.lockorders.emplace(p1, lock_stack);
 
         const LockPair p2 = std::make_pair(c, i.first);
         lockdata.invlockorders.insert(p2);
-        if (lockdata.lockorders.count(p2))
+        if (lockdata.lockorders.count(p2)) {
             potential_deadlock_detected(p1, lockdata.lockorders[p2], lockdata.lockorders[p1]);
+
+            lockdata.invlockorders.erase(p2);
+            lockdata.lockorders.erase(p1);
+            lock_stack.pop_back();
+
+            if (g_debug_lockorder_abort) {
+                tfm::format(std::cerr,
+                    "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 pop_lock()
 {
     LockData& lockdata = GetLockData();

src/sync.cpp

vasild · 2020-06-22T09:27:48Z

src/sync.cpp

@@ -131,7 +131,7 @@ static void potential_deadlock_detected(const LockPair& mismatch, const LockStac
    throw std::logic_error("potential deadlock detected");


I would suggest to unconditionally remove the entry from the stack here, after printing the stack and before abort()/throw. This cleanup is warranted from all code paths that call EnterCritical().

Suggested change

throw std::logic_error("potential deadlock detected");

// Undo the insertion from push_lock(). We will not lock the mutex.

s2.pop_back();

throw std::logic_error("potential deadlock detected");

(this should be before the if, 4 lines above, but github does not allow me to put the suggestion there)

hebasto · 2020-06-22T09:56:54Z

@vasild

It would be good if the offending entry is still in the lock stack by the time potential_deadlock_detected() is called because it prints the stack and we want to see the offender in the debug log.

It prints from LockOrders instances, not from the lock stack of the current thread, no?

vasild · 2020-06-22T10:59:51Z

You are right! I did not realize that the LockStack objects are copied.

Anyway - I think, for EnterCritical() to be a good citizen, if it throws, it should leave its internal structures in the state they were before it was called.

hebasto · 2020-06-22T11:46:23Z

Updated c810cd4 -> 236e760 (pr19340.01 -> pr19340.02, diff):

addressed @vasild's comment:

operator[] has a side effect that it would insert a new element in m_lock_stacks if the thread that called it does not own any locks.

@vasild

Anyway - I think, for EnterCritical() to be a good citizen, if it throws, it should leave its internal structures in the state they were before it was called.

I'd leave it as is since it seems unrelated to this PR goal.

hebasto · 2020-06-22T18:53:21Z

Reworked after the discussion with @vasild on IRC.

~~Going to update the OP soon.~~ The OP has been updated.

vasild

ACK f88222a

JeremyRubin · 2020-07-09T21:05:10Z

I'm not sure I fully get what's going on here, but the change looks fine to me. Can you explain how making the copy of the lockstack actually helps? It looks like a race condition to even copy it?

vasild · 2020-07-10T07:58:07Z

@JeremyRubin thanks for asking! The key here is the added lock_stack.pop_back(), not the copying. The copying is needed because without it the prints from potential_deadlock_detected() would not be complete (because we already removed one element). There is no race because that code is operating under lockdata.dd_mutex.

Let me elaborate with an example:

Thread t1 acquires lockA
Thread t2 acquires lockB
Thread t1 tries to acquire lockB
- EnterCritical() adds lockB to the lock stack of t1 and succeeds
- The lock stacks are: t1: lockA, lockB, t2: lockB
- Now t1 hangs inside the OS mutex lock() function, waiting for lockB to be released.
Thread t2 tries to acquire lockA
- EnterCritical() adds lockA to the lock stack of t2 and it is really upset because some threads first (try to) acquire lockA and then lockB and some other threads try in reverse order. It is going to either abort() or throw, lets assume it is going to throw.
- The lock stacks are: t1: lockA, lockB, t2: lockB, lockA
- EnterCritical() prints both lock stacks and throws.
- The caller (in t2) catches the exception and continues executing (this is only happening from tests).

Notice that at the end t2's lock stack is lockB, lockA but it actually only owns lockB -- this is the problem being fixed by this PR. This only matters in tests which catch the exception and continue executing. The other code ends up with abort() and the whole program terminates immediately.

maflcko

ACK f88222a

just some questions. Feel free to ignore.

maflcko · 2020-07-31T06:12:09Z

src/sync.cpp


        const LockPair p1 = std::make_pair(i.first, c);
+        const LockPair p2 = std::make_pair(c, i.first);
+        if (lockdata.lockorders.count(p2)) {


Any reason this is now being done before if (lockdata.lockorders.count(p1))continue?

src/sync.h

hebasto · 2020-08-02T13:47:37Z

Updated f88222a -> cab80f4 (pr19340.03 -> pr19340.04, diff):

addressed @MarcoFalke's comment:

Any reason this is now being done before if (lockdata.lockorders.count(p1))continue?

addressed @MarcoFalke's comment:

Could remove the confusing static's here?

Rebased cab80f4 -> 63e9e40 (pr19340.04 -> pr19340.05) to fix Travis "lint" job errors.

vasild

ACK 63e9e40

maflcko · 2020-08-04T14:59:28Z

re-ACK 63e9e40

…ial deadlock detected" exception thrown 63e9e40 test: Add LockStackEmpty() (Hennadii Stepanov) 42b2a95 test: Repeat deadlock tests (Hennadii Stepanov) 1f96be2 Preserve initial state if push_lock() throws exception (Hennadii Stepanov) Pull request description: On master (e3fa3c7) if the `push_lock()` throws the "potential deadlock detected" exception (via the `potential_deadlock_detected()` call), the `LockData` instance internal state differs from one when the `push_lock()` was called. This non-well behaviour makes (at least) testing brittle. This PR preserves the `LockData` instance initial state if `push_lock()` throws an exception, and improves the `sync_tests` unit test. ACKs for top commit: MarcoFalke: re-ACK 63e9e40 vasild: ACK 63e9e40 Tree-SHA512: 7679182154ce5f079b44b790faf76eb5f553328dea70a326ff6b600db70e2f9ae015a33a104ca070cb660318280cb79b6b42e37ea5166f26f9e627ba721fcdec

…exception thrown Summary: > Preserve initial state if push_lock() throws exception > test: Repeat deadlock tests > test: Add LockStackEmpty() This is a backport of [[bitcoin/bitcoin#19340 | core#19340]] Test Plan: With TSAN: `ninja check check-functional` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D10059

…l deadlock detected" exception thrown

hebasto mentioned this pull request Jun 20, 2020

sync: detect double lock from the same thread #19337

Merged

DrahtBot added the Tests label Jun 20, 2020

vasild reviewed Jun 22, 2020

View reviewed changes

hebasto force-pushed the 200620-stack branch from c810cd4 to 236e760 Compare June 22, 2020 11:41

hebasto force-pushed the 200620-stack branch from 236e760 to f88222a Compare June 22, 2020 18:51

DrahtBot mentioned this pull request Jun 22, 2020

Fix mistakenly swapped "previous" and "current" lock orders #19353

Merged

hebasto changed the title ~~Fix lock stack after "potential deadlock detected" exception~~ Preserve the LockData initial state if "potential deadlock detected" exception thrown Jun 22, 2020

vasild approved these changes Jun 23, 2020

View reviewed changes

This was referenced Jul 28, 2020

Remove mempool global #19556

Merged

Disable fee estimation in blocksonly mode (by removing the fee estimates global) #18766

Merged

maflcko approved these changes Jul 31, 2020

View reviewed changes

hebasto force-pushed the 200620-stack branch from f88222a to cab80f4 Compare August 2, 2020 13:34

hebasto added 3 commits August 2, 2020 16:42

Preserve initial state if push_lock() throws exception

1f96be2

test: Repeat deadlock tests

42b2a95

test: Add LockStackEmpty()

63e9e40

hebasto force-pushed the 200620-stack branch from cab80f4 to 63e9e40 Compare August 2, 2020 13:43

vasild approved these changes Aug 4, 2020

View reviewed changes

maflcko merged commit 0f16212 into bitcoin:master Aug 4, 2020

hebasto deleted the 200620-stack branch August 4, 2020 15:11

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 12, 2021

merge bitcoin#19340: Preserve the LockData initial state if "potentia…

790a4f9

…l deadlock detected" exception thrown

pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021

merge bitcoin#19340: Preserve the LockData initial state if "potentia…

3f992e3

…l deadlock detected" exception thrown

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

		@@ -131,7 +131,7 @@ static void potential_deadlock_detected(const LockPair& mismatch, const LockStac
		throw std::logic_error("potential deadlock detected");

Conversation

hebasto commented Jun 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild Jun 22, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jun 22, 2020

Uh oh!

vasild commented Jun 22, 2020

Uh oh!

hebasto commented Jun 22, 2020

Uh oh!

hebasto commented Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

JeremyRubin commented Jul 9, 2020

Uh oh!

vasild commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto Aug 2, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Aug 2, 2020

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hebasto commented Jun 20, 2020 •

edited

Loading

DrahtBot commented Jun 20, 2020 •

edited

Loading

hebasto commented Jun 22, 2020 •

edited

Loading

vasild commented Jul 10, 2020 •

edited

Loading