This is another step (after #19854) to transit CTxMemPool::cs from RecursiveMutex to Mutex.
Split out from #19306. Thread safety annotations, lock assertions, and required explicit locking added. No behavior change.
Please note that now, since #19668 has been merged, it is safe to apply AssertLockHeld() macros as they do not swallow compile time Thread Safety Analysis warnings.
Updated c22c4d2320f288f57fd04515d29b72d6342ea11a -> e79bc0f275977fafcb98a3065256222639a46328 (pr19872.01 -> pr19872.02, diff):
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
No change in behavior, some call sites already held the lock and a lock
is added in the remaining sites.
No change in behavior, some call sites already held the lock and a lock
is added in the remaining sites.
No change in behavior, some call sites already held the lock and a lock
is added in the remaining sites.
No change in behavior, some call sites already held the lock and a lock
is added in the remaining sites.
No change in behavior, some call sites already held the lock and a lock
is added in the remaining sites.
No change in behavior, some call sites already held the lock and a lock
is added in the remaining sites.
No change in behavior, some call sites already held the lock and a lock
is added in the remaining sites.
No change in behavior, some call sites already held the lock and a lock
is added in the remaining sites.
No change in behavior, some call sites already held the lock and a lock
is added in the remaining sites.
No change in behavior, some call sites already held the lock and a lock
is added in the remaining sites.
614@@ -615,7 +615,7 @@ static void CheckInputsAndUpdateCoins(const CTransaction& tx, CCoinsViewCache& m
615
616 void CTxMemPool::check(const CCoinsViewCache *pcoins) const
617 {
618- LOCK(cs);
619+ AssertLockHeld(cs);
check() is never currently called with the lock held.
check() isn’t currently called anywhere with the lock held.
1089@@ -1090,8 +1090,9 @@ uint64_t CTxMemPool::CalculateDescendantMaximum(txiter entry) const {
1090 return maximum;
1091 }
1092
1093-void CTxMemPool::GetTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants) const {
1094- LOCK(cs);
1095+void CTxMemPool::GetTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants) const
1096+{
1097+ AssertLockHeld(cs);
GetTransactionAncestry() is only called with the lock held in the tests, which can be updated.
CTxMemPool::exists. Looks like it’s a premature change.
Concept ACK.
Similar changes are justified, assuming we want to get rid of recursive mutexes.
Recursive mutexes are used by functions that need to own a mutex to do their stuff, but some of the callers of those functions own the mutex and some don’t. So the function does not have a clear interface wrt the mutex, rather it has a vague interface like “the caller may own the mutex and I will lock it myself anyway”.
IMO the cleaner interface is if the mutex is acquired inside the function and none of the callers own it. If that is not possible then the function must not lock it and mandate that the callers own it. If that is too verbose for the callers, then two functions could be used - one that locks and another that does not:
0funcUnprotected() EXCLUSIVE_LOCKS_REQUIRED(cs)
1{
2 AssertLockHeld(cs);
3 ...
4}
5
6func() EXCLUSIVE_LOCKS_REQUIRED(!cs)
7{
8 LOCK(cs);
9 funcUnprotected();
10}
Many thanks to @jnewbery, @promag and @vasild for your attention to thread safety design!
Let me summarize the two approaches to preventing recursive locking of a mutex that are suggested/discussed/used in the repo:
“move the lock up the stack if it prevents unnecessary unlocks/locks” (credits to @promag) E.g., #19833, this PR (in its current state, ff19c7ddb8b8b076e5c538be434abd53af572b8d)
“two functions could be used - one that locks and another that does not” (credits to @vasild) E.g., #19238
I believe we should reach the consensus about approach we will use while migrating from RecursiveMutex to Mutex before any further step.
Is it a topic for meeting?
- (most preferred, if possible) lock the mutex only inside the function that needs it
Sure! I just described variants when this is not possible :)
IMO we should move the lock up the stack if it prevents unnecessary unlocks/locks. That’s not the case, for instance in
CTxMemPool::exists. Looks like it’s a premature change.
and mempool_tests.cpp
CTxMemPool, and add member functions with locking required for tests.
🐙 This pull request conflicts with the target branch and needs rebase.
Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.