… the actual goal is “make locking logic easier to follow” rather than “remove RecurviseMutex”
Should be noted, to insure that mutex locking policy has not been changed by accident in non-trivial cases, all of the related code branches must be covered by appropriate lock assertions: AssertLockHeld() or AssertLockNotHeld().
Also Clang Thread Safety Analysis annotations with Negative Capabilities are very useful (see #19249) but are not a panacea, of course :)
<sipa> recursive mutexes are evil
<sipa> there is probably a place for them, like goto
<sipa> but almost always, they just lead to badly designed abstractions
<sipa> a clear design should have code that operates outside the critical section, and code that operates inside
<sipa> but not code that works in both
<gwillen> my impression is that the usual better approach is to have Foo() which calls Foo_locked(), and callers who already hold the mutex can call the latter directly
<sipa> indeed
hebasto
commented at 8:59 am on June 18, 2020:
member
ryanofsky
commented at 1:26 pm on June 18, 2020:
contributor
+0.5 concept ACK for this as a low-priority janitorial project, since it can make code easier to reason about. I wouldn’t prioritize working on this over working on user-visible bugfixes and improvements. I wouldn’t even prioritize this over similar cleanups like removing cases of gui blocking #16874 (comment). But it is better to do this than not to do this.
maflcko added the label
Refactoring
on Jun 18, 2020
maflcko added the label
Brainstorming
on Jun 18, 2020
maflcko
commented at 2:43 pm on June 18, 2020:
member
I don’t mind trivial to review changes that are obviously correct and only replace RecursiveMutex with Mutex like commit 1a9ef1d398dd14728b6bc67a89139cdf827c9753. More invasive refactors are hard to prioritize due to the conflicts they cause with non-refactoring changes.
hebasto
commented at 2:50 pm on June 18, 2020:
member
I don’t mind trivial to review changes that are obviously correct and only replace RecursiveMutex with Mutex like commit 1a9ef1d. More invasive refactors are hard to prioritize due to the conflicts they cause with non-refactoring changes.
It is assumed that there will be no non-refactoring changes (e.g., #19306).
ajtowns
commented at 7:21 am on June 20, 2020:
contributor
Makes sense, provided the actual goal is “make locking logic easier to follow” rather than “remove RecurviseMutex” – if removing RecursiveMutex makes the code/reasoning more complex, it’s not a good idea. (The if (mempool_locked) stuff in #19306 seems more complex, for instance)
hebasto
commented at 12:21 pm on June 20, 2020:
member
Makes sense, provided the actual goal is “make locking logic easier to follow” rather than “remove RecurviseMutex”…
Sure! The final goal is to “make locking logic easier to follow”. Btw, I’ve started this work while reasoning about issues like #17397.
… if removing RecursiveMutex makes the code/reasoning more complex, it’s not a good idea.
I think the opposite. Adding simple wrappers to differentiate functions by mutex locking condition does not make the code/reasoning more complex. This refactoring just reveals the actual code comlexity.
The if (mempool_locked) stuff in #19306 seems more complex, for instance
Yes, I’ve used this stuff in 72f7486b5ebe96762c5d5a68849c61e58c812ffd and 21787abedd7a0808c0175a0b1d795df97fd3b970 as a quick-and-dirty solution to fix broken tests.
maflcko referenced this in commit
c8fa03d176
on Jun 25, 2020
laanwj referenced this in commit
2af56d6d5c
on Jun 29, 2020
maflcko referenced this in commit
bab4cce1b0
on Sep 1, 2020
sidhujag referenced this in commit
ed409a3f99
on Sep 3, 2020
maflcko referenced this in commit
417f95fa45
on Jan 5, 2021
sidhujag referenced this in commit
e01d44f50b
on Jan 5, 2021
maflcko referenced this in commit
3a2c84a6b5
on Jun 14, 2021
maflcko referenced this in commit
7be143a960
on Aug 30, 2021
sidhujag referenced this in commit
16d4b8d9dc
on Aug 30, 2021
maflcko referenced this in commit
76392b042e
on Nov 25, 2021
maflcko referenced this in commit
0b30bdd519
on Dec 2, 2021
sidhujag referenced this in commit
c6c63e40d8
on Dec 3, 2021
maflcko referenced this in commit
dbf81a73e3
on Jan 17, 2022
maflcko referenced this in commit
427e9c9435
on Jan 17, 2022
sidhujag referenced this in commit
2ec7929b1c
on Jan 18, 2022
sidhujag referenced this in commit
808dbb9d51
on Jan 18, 2022
maflcko referenced this in commit
1824644a36
on Jan 20, 2022
maflcko referenced this in commit
b32f0d3af1
on Jan 24, 2022
sidhujag referenced this in commit
180b45eb36
on Jan 28, 2022
maflcko referenced this in commit
ad05e68e17
on Jan 31, 2022
PastaPastaPasta referenced this in commit
9fb9404949
on Mar 13, 2022
PastaPastaPasta referenced this in commit
80c1316da7
on Mar 13, 2022
laanwj referenced this in commit
a19f641a80
on Apr 26, 2022
maflcko referenced this in commit
0be1dc1f56
on May 17, 2022
sidhujag referenced this in commit
b1ab840410
on May 28, 2022
vijaydasmp referenced this in commit
ebc27ccc4d
on Jan 9, 2023
vijaydasmp referenced this in commit
0f32add525
on Jan 13, 2023
vijaydasmp referenced this in commit
1a45924b6b
on Jan 18, 2023
vijaydasmp referenced this in commit
01d2e88e9b
on Jan 19, 2023
vijaydasmp referenced this in commit
623d580f42
on Feb 17, 2023
vijaydasmp referenced this in commit
566b399dfd
on Feb 19, 2023
vijaydasmp referenced this in commit
6b51b18a11
on Feb 20, 2023
vijaydasmp referenced this in commit
5069b6d3c8
on Feb 20, 2023
vijaydasmp referenced this in commit
8a04618b1e
on Feb 24, 2023
vijaydasmp referenced this in commit
9528c12d16
on Mar 5, 2023
vijaydasmp referenced this in commit
a7eca79539
on Mar 10, 2023
vijaydasmp referenced this in commit
2900ec8082
on Mar 13, 2023
vijaydasmp referenced this in commit
836f463801
on Mar 14, 2023
PastaPastaPasta referenced this in commit
20fb6158a5
on Apr 15, 2023
PastaPastaPasta referenced this in commit
75ebc275e0
on Apr 15, 2023
hebasto
commented at 3:13 pm on April 18, 2023:
member
A shortlist of remaining class members recursive mutexes at 2fa7344aa9bbce8069ebd6bef5f6a22f0b7c5c56:
FWIW, I’m not able to reproduce “deadlock” TSan warning using clang-14 starting from 9996b1806a189a9632c9f5023489eb47bf87ca05 when depends can be compiled. Last time the warning was observed for clang-10.
maflcko
commented at 7:27 am on April 19, 2023:
member
You should be able to see it in the tsan CI task or with libc++ locally?
fanquake
commented at 8:54 am on April 19, 2023:
member
You should be able to see it in the tsan CI task or with libc++ locally?
I don’t currently reproduce on x86_64 with master +:
0time FILE_ENV="./ci/test/00_setup_env_native_tsan.sh" ./ci/test_run_all.sh
1...
2wallet_upgradewallet.py --legacy-wallet | â—‹ Skipped | 0 s
3 4ALL | ✓ Passed | 3484 s (accumulated) 5Runtime: 990 s
6 7Stop and remove CI container by ID
8d726f7b16435da0d5d35171cc39363cd85be5b22d6fbbfffc28983691d99854b
910real 46m40.485s
hebasto
commented at 1:34 pm on April 19, 2023:
member
You should be able to see it in the tsan CI task or with libc++ locally?
The TSan CI job is pretty stable @ 760651214cd205b91804bc1c40a31c614d3aa26c, and removing the deadlock:Chainstate::ConnectTip suppression does not cause any failure.
My guess is that this is a byproduct of clang upgrade.
maflcko
commented at 3:20 pm on April 19, 2023:
member
Steps to reproduce on a fresh install of Fedora 38:
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-05-18 15:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me