Release `LockData::dd_mutex` before calling `*_detected` functions #26781

hebasto · 2022-12-31T15:38:49Z

Both double_lock_detected() and potential_deadlock_detected() functions call LogPrintf() which in turn implies locking of the Logger::m_cs mutex. To avoid a deadlock, the latter must not have the Mutex type (see #16112).

With this PR the mentioned restriction has been lifted, and it is possible now to use our regular Mutex type for the Logger::m_cs mutex instead of a dedicated StdMutex type (not introducing that change here, as its diff is much bigger than a few lines, and the currently proposed diff seems valuable by itself).

Note for reviewers: Make sure the code is configured and built with CPPFLAGS=-DDEBUG_LOCKORDER.

DrahtBot · 2022-12-31T15:38:51Z

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/26781.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	vasild, ryanofsky

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

src/sync.cpp

vasild

ACK 20647b8

DrahtBot · 2023-08-08T17:13:33Z

There hasn't been much activity lately. What is the status here?

Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

hebasto · 2023-08-08T19:16:03Z

cc @ryanofsky

Both `double_lock_detected()` and `potential_deadlock_detected()` functions call `LogPrintf()` which in turn implies locking of the `Logger::m_cs` mutex. To avoid a deadlock the latter must not have the `Mutex` type. With this change the mentioned restriction has been lifted, and it is possible now to use our regular `Mutex` type for the `Logger::m_cs` mutex instead of a dedicated `StdMutex` type.

hebasto · 2024-02-12T14:36:17Z

Rebased to refresh the CI.

vasild

ACK 38f93fe

hebasto · 2024-03-08T10:55:32Z

@maflcko @ryanofsky

Do you mind having a look into this PR please?

maflcko · 2024-03-11T13:04:25Z

Sure, seems fine, but I don't see the benefit, if the logger is kept as-is?

ryanofsky

Code review ACK 38f93fe. This seems like a good change, but I think it would be better if it had another commit getting rid of StdMutex/StdLockGuard. Otherwise nothing is really taking advantage of the change.

ryanofsky · 2024-03-11T14:17:25Z

src/sync.cpp

@@ -172,6 +172,7 @@ static void push_lock(MutexType* c, const CLockLocation& locklocation)
            // same thread as that results in an undefined behavior.
            auto lock_stack_copy = lock_stack;
            lock_stack.pop_back();
+            lock.unlock();


In commit "Release LockData::dd_mutex before calling *_detected functions" (08cfe27)

Would be good to have a comment saying why this is necessary. If I understand correctly it could say something like // Release dd_mutex before calling double_lock_detected to avoid deadlocks, so no other lock is ever acquired after dd_mutex, and lock order is consistent.

ryanofsky · 2024-03-11T14:20:17Z

src/sync.cpp

@@ -182,9 +183,11 @@ static void push_lock(MutexType* c, const CLockLocation& locklocation)

        const LockPair p2 = std::make_pair(c, i.first);
        if (lockdata.lockorders.count(p2)) {
-            auto lock_stack_copy = lock_stack;
+            auto lock_stack_current = lock_stack;


In commit "Release LockData::dd_mutex before calling *_detected functions" (08cfe27)

Previous name lock_stack_copy seems like it explains the purpose for this variable better than lock_stack_current. Also lock_stack_copy is the name used above and it would be nicer if double_lock_detected and potential_deadlock_detected cases were handled consistently I think, unless I am missing something.

achow101 · 2024-06-17T21:22:27Z

@hebasto Are you planning on addressing review feedback here?

I think it would be better if it had another commit getting rid of StdMutex/StdLockGuard. Otherwise nothing is really taking advantage of the change.

I agree. The PR description sounded like this was going to be the ultimate change, but it does not appear to have actually been done.

vasild · 2024-06-21T08:44:12Z

I re-reviewed this again and retain my ACK because IMO it is an improvement to master even without further changes because it reduces the scope where the mutex is held. I don't see the fact that it can go further as a blocker for this.

I would be happy to review changing StdMutex m_cs to Mutex m_cs here or in another PR.

The PR description sounded like this was going to be the ultimate change

well, fwiw, the description contains "not introducing that change here".

DrahtBot · 2025-02-15T01:48:30Z

🤔 There hasn't been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

vasild

ACK 38f93fe

(repeat my earlier comment, DrahtBot was confused)

DrahtBot · 2025-05-22T00:46:47Z

🤔 There hasn't been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

vasild reviewed Jan 4, 2023

View reviewed changes

src/sync.cpp Outdated Show resolved Hide resolved

hebasto force-pushed the 221231-lock branch from 6fe1365 to 20647b8 Compare January 5, 2023 15:10

vasild approved these changes Jan 5, 2023

View reviewed changes

achow101 requested a review from ryanofsky April 25, 2023 15:49

DrahtBot added CI failed and removed CI failed labels Oct 21, 2023

DrahtBot added CI failed and removed CI failed labels Dec 19, 2023

DrahtBot added the CI failed label Jan 13, 2024

hebasto force-pushed the 221231-lock branch from 20647b8 to 6092ea4 Compare February 12, 2024 14:35

hebasto added 2 commits February 12, 2024 14:35

refactor: Add [[noreturn]] attribute to *_detected() functions

38f93fe

hebasto force-pushed the 221231-lock branch from 6092ea4 to 38f93fe Compare February 12, 2024 14:35

vasild approved these changes Feb 12, 2024

View reviewed changes

DrahtBot removed the CI failed label Feb 12, 2024

ryanofsky reviewed Mar 11, 2024

View reviewed changes

DrahtBot added CI failed and removed CI failed labels Sep 12, 2024

DrahtBot added CI failed and removed CI failed labels Sep 28, 2024

hebasto marked this pull request as draft October 11, 2024 19:04

DrahtBot added the CI failed label Oct 25, 2024

DrahtBot added CI failed and removed CI failed labels Oct 29, 2024

DrahtBot removed the CI failed label Nov 9, 2024

DrahtBot added the CI failed label Nov 17, 2024

vasild approved these changes Feb 21, 2025

View reviewed changes

DrahtBot mentioned this pull request Jul 30, 2025

refactor: unify container presence checks #33094

Closed

Release LockData::dd_mutex before calling *_detected functions #26781

Are you sure you want to change the base?

Release LockData::dd_mutex before calling *_detected functions #26781

Uh oh!

Conversation

hebasto commented Dec 31, 2022

Uh oh!

DrahtBot commented Dec 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Aug 8, 2023

Uh oh!

hebasto commented Aug 8, 2023

Uh oh!

hebasto commented Feb 12, 2024

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Mar 8, 2024

Uh oh!

maflcko commented Mar 11, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 17, 2024

Uh oh!

vasild commented Jun 21, 2024

Uh oh!

DrahtBot commented Feb 15, 2025

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented May 22, 2025

Uh oh!

Uh oh!

Release `LockData::dd_mutex` before calling `*_detected` functions #26781

Release `LockData::dd_mutex` before calling `*_detected` functions #26781

DrahtBot commented Dec 31, 2022 •

edited

Loading