Skip to content

sync: print proper lock order location when double lock is detected #20507

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.

Sign up for GitHub

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

Jump to bottom
Merged
maflcko merged 3 commits into from
Dec 2, 2020
Merged

sync: print proper lock order location when double lock is detected #20507

maflcko merged 3 commits into from
Dec 2, 2020

Conversation

@vasild
Copy link
Contributor

@vasild vasild commented Nov 26, 2020

Before:

Assertion failed: detected double lock at src/sync.cpp:153, details in debug log.

After:

Assertion failed: detected double lock for 'm' in src/test/sync_tests.cpp:40 (in thread ''), details in debug log.

@vasild
sync: print proper lock order location when double lock is detected
6d3689f 
Before:
```
Assertion failed: detected double lock at src/sync.cpp:153, details in debug log.
```

After:
```
Assertion failed: detected double lock for 'm' in src/test/sync_tests.cpp:40 (in thread ''), details in debug log.
```
@vasild
sync: const-qualify the argument of double_lock_detected()
a21dc46 
It is not modified in the function, so should be `const`.
@vasild
sync: use HasReason() in double lock tests
db058ef 
`HasReason()` is shorter than a lambda function.
@vasild
Copy link
Contributor Author

vasild commented Nov 26, 2020

To force printing of the message:

--- i/src/test/sync_tests.cpp
+++ w/src/test/sync_tests.cpp
@@ -46,5 +46,4 @@ void TestDoubleLock(bool should_throw)
 {
     const bool prev = g_debug_lockorder_abort;
-    g_debug_lockorder_abort = false;
 
     MutexType m;

and run ./src/test/test_bitcoin --run_test="sync_tests/double_lock_mutex"

@jonatack
Copy link
Member

jonatack commented Nov 26, 2020

Concept ACK

@vasild vasild mentioned this pull request Nov 26, 2020
Merged
@DrahtBot DrahtBot added the Tests label Nov 26, 2020
hebasto
hebasto approved these changes Nov 26, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK db058ef, tested on Linux Mint 20 (x86_64).

Thanks for fixing 👍

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Dec 1, 2020

Thanks for fixing!
utACK db058ef

@ajtowns
Copy link
Contributor

ajtowns commented Dec 2, 2020

ACK db058ef

@maflcko maflcko merged commit 7f0f01f into bitcoin:master Dec 2, 2020
promag
promag reviewed Dec 2, 2020
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK db058ef.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 2, 2020
Merge bitcoin#20507: sync: print proper lock order location when doub…
cba2197 
…le lock is detected

db058ef sync: use HasReason() in double lock tests (Vasil Dimov)
a21dc46 sync: const-qualify the argument of double_lock_detected() (Vasil Dimov)
6d3689f sync: print proper lock order location when double lock is detected (Vasil Dimov)

Pull request description:

  Before:
  ```
  Assertion failed: detected double lock at src/sync.cpp:153, details in debug log.
  ```

  After:
  ```
  Assertion failed: detected double lock for 'm' in src/test/sync_tests.cpp:40 (in thread ''), details in debug log.
  ```

ACKs for top commit:
  jonasschnelli:
    utACK db058ef
  ajtowns:
    ACK db058ef
  hebasto:
    ACK db058ef, tested on Linux Mint 20 (x86_64).

Tree-SHA512: 452ddb9a14e44bb174135b39f2219c76eadbb8a6c0e80d64a25f995780d6dbc7b570d9902616db94dbfabaee197b5828ba3475171a68240ac0958fb203a7acdb
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hebasto hebasto hebasto approved these changes

+1 more reviewer

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@vasild @jonatack @jonasschnelli @ajtowns @promag @hebasto @maflcko @DrahtBot