Avoid locking mutexes that are already held by the same thread

practicalswift commented at 6:54 pm on November 24, 2017: contributor

Avoid locking mutexes that are already held by the same thread. These are reentrant mutexes, but still no need to lock them more than once per thread :-)

fanquake added the label Refactoring on Nov 24, 2017

promag commented at 4:29 pm on November 27, 2017: member

utACK 9f857d5.

Are these random picks? For instance, CTxMemPool::CalculateMemPoolAncestors can assert the lock too right?

MarcoFalke commented at 5:44 pm on November 27, 2017: member

Why would we need the runtime check when there is a compile time check?

practicalswift commented at 7:27 pm on November 27, 2017: contributor

@promag No, these are not random picks. They were the ones I could find, verify manually and passes both make check + test/functional/test_runner.py with nothing but trivial changes :-)

The lock in CTxMemPool::UpdateTransactionsFromBlock you suggested could also be removed but that would require more fine grained locking in the corresponding tests. If you have time then please post a diff of that locking change and I’ll make it part of this PR :-)

practicalswift commented at 7:29 pm on November 27, 2017: contributor

@MarcoFalke I replaced the LOCK(…); with AssertLockHeld(…); to make it easy to verify also for reviewers not building with Clang thread safety analysis warnings enabled. Should I remove them? :-)

practicalswift commented at 7:35 pm on November 27, 2017: contributor

@promag Do you have any additional suggestions beyond CTxMemPool::CalculateMemPoolAncestors(…)? :-)

MarcoFalke commented at 7:36 pm on November 27, 2017: member

I think it is fine to leave them for now, but it seems arbitrary to add run time checks to a subset of methods that already have the compile time check. I know that not everyone compiles with clang, but at least for releases we do and I am sure some developers build with clang, so it should be noticed quickly.

promag commented at 9:07 pm on November 27, 2017: member

I wasn’t suggesting to remove more locks. Maybe these changes are preferable in small PR’s.

practicalswift force-pushed on Nov 27, 2017

practicalswift commented at 10:47 pm on November 27, 2017: contributor

Added the removeConflicts(…) EXCLUSIVE_LOCKS_REQUIRED(…) annotation to txmempool.h. @promag Would you mind re-reviewing?

in src/wallet/walletdb.cpp:525 in dc6cd0bc10 outdated

521@@ -522,13 +522,13 @@ bool CWalletDB::IsKeyType(const std::string& strType)
522             strType == "mkey" || strType == "ckey");
523 }
524 
525-DBErrors CWalletDB::LoadWallet(CWallet* pwallet)
526+DBErrors CWalletDB::LoadWallet(CWallet* pwallet) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)

theuni commented at 9:55 pm on November 28, 2017:

Adding this only in the .cpp means that the lock requirement will only be enforced for other functions in this file.

I realize that it can’t be added in the header as-is due to the circular dependency, but that needs to be fixed somehow.

practicalswift commented at 8:13 pm on November 29, 2017:

@theuni Yes, I’d like to put the annotation in the header file but I need help with resolving the circular dependency. What would be the cleanest way to solve it? :-)

theuni commented at 10:22 pm on December 20, 2017:

I’m afraid I don’t see a simple solution :(

ajtowns commented at 7:06 am on January 11, 2018:

I don’t know if it’s a simple solution, but it looks like you can resolve the circular dependency by declaring:

inline CCriticalSection &CWallet_cs_wallet(CWallet *pwallet) LOCK_RETURNED(pwallet->cs_wallet)
{
    return pwallet->cs_wallet;
}

in wallet.h and

CCriticalSection &CWallet_cs_wallet(CWallet *pwallet);
class CWalletDB {
    DBErrors LoadWallet(CWallet* pwallet) EXCLUSIVE_LOCKS_REQUIRED(CWallet_cs_wallet(pwallet));
}

in walletdb.h.

in src/txmempool.cpp:542 in dc6cd0bc10 outdated

538@@ -539,10 +539,10 @@ void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMem
539     RemoveStaged(setAllRemoves, false, MemPoolRemovalReason::REORG);
540 }
541 
542-void CTxMemPool::removeConflicts(const CTransaction &tx)
543+void CTxMemPool::removeConflicts(const CTransaction &tx) EXCLUSIVE_LOCKS_REQUIRED(cs)

theuni commented at 9:56 pm on November 28, 2017:

No need for the dupe here, header is enough.

practicalswift force-pushed on Nov 29, 2017

practicalswift force-pushed on Feb 22, 2018

practicalswift commented at 9:21 pm on February 22, 2018: contributor

@theuni @promag @MarcoFalke Updated to address feedback. Please re-review :-)

MarcoFalke commented at 9:26 pm on February 22, 2018: member

utACK 96b6899e2c9b4a86698a6ea64460d045fde68c43

sipa commented at 6:57 am on July 21, 2018: member

utACK 96b6899e2c9b4a86698a6ea64460d045fde68c43

in src/wallet/walletdb.cpp:527 in 96b6899e2c outdated

523@@ -524,7 +524,7 @@ DBErrors CWalletDB::LoadWallet(CWallet* pwallet)
524     bool fNoncriticalErrors = false;
525     DBErrors result = DB_LOAD_OK;
526 
527-    LOCK(pwallet->cs_wallet);
528+    AssertLockHeld(pwallet->cs_wallet);

MarcoFalke commented at 2:59 pm on July 21, 2018:

This will no longer compile due to the missing lock annotation in the header.

Could just remove it for now?

DrahtBot commented at 5:29 pm on July 21, 2018: member

DrahtBot closed this on Jul 21, 2018

DrahtBot reopened this on Jul 21, 2018

Avoid locking mutexes that are already held by the same thread 01a06d6686

practicalswift force-pushed on Jul 21, 2018

practicalswift commented at 5:47 pm on July 21, 2018: contributor

@MarcoFalke Now skipping the change to src/wallet/walletdb.cpp as suggested.

Please re-review :-)

MarcoFalke merged this on Jul 22, 2018

MarcoFalke closed this on Jul 22, 2018

MarcoFalke referenced this in commit 05714f96e4 on Jul 22, 2018

jasonbcox referenced this in commit ed346d416f on Sep 27, 2019

jonspock referenced this in commit 33539606ca on Dec 24, 2019

jonspock referenced this in commit 13acc3f200 on Dec 26, 2019

jonspock referenced this in commit fdc2db4a58 on Dec 27, 2019

PastaPastaPasta referenced this in commit cbfd59ad1f on Jul 29, 2020

LarryRuane referenced this in commit 26e68c5ca9 on Mar 24, 2021

practicalswift deleted the branch on Apr 10, 2021

LarryRuane referenced this in commit 6308962e7e on Apr 26, 2021

LarryRuane referenced this in commit 55f58b3ee7 on May 27, 2021

str4d referenced this in commit d5c1e759c8 on Sep 23, 2021

gades referenced this in commit f0806d083f on Jan 29, 2022

str4d referenced this in commit 6f84b596e0 on May 14, 2022

DrahtBot locked this on Aug 16, 2022

Avoid locking mutexes that are already held by the same thread #11762