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

practicalswift · 2017-11-24T18:54:00Z

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 :-)

promag · 2017-11-27T16:29:52Z

utACK 9f857d5.

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

maflcko · 2017-11-27T17:44:08Z

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

practicalswift · 2017-11-27T19:27:52Z

@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 · 2017-11-27T19:29:26Z

@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 · 2017-11-27T19:35:13Z

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

maflcko · 2017-11-27T19:36:25Z

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 · 2017-11-27T21:07:06Z

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

practicalswift · 2017-11-27T22:47:12Z

Added the removeConflicts(…) EXCLUSIVE_LOCKS_REQUIRED(…) annotation to txmempool.h.

@promag Would you mind re-reviewing?

theuni · 2017-11-28T21:55:35Z

src/wallet/walletdb.cpp

@@ -522,13 +522,13 @@ bool CWalletDB::IsKeyType(const std::string& strType)
            strType == "mkey" || strType == "ckey");
 }

-DBErrors CWalletDB::LoadWallet(CWallet* pwallet)
+DBErrors CWalletDB::LoadWallet(CWallet* pwallet) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)


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.

@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? :-)

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

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.

theuni · 2017-11-28T21:56:36Z

src/txmempool.cpp

@@ -539,10 +539,10 @@ void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMem
    RemoveStaged(setAllRemoves, false, MemPoolRemovalReason::REORG);
 }

-void CTxMemPool::removeConflicts(const CTransaction &tx)
+void CTxMemPool::removeConflicts(const CTransaction &tx) EXCLUSIVE_LOCKS_REQUIRED(cs)


No need for the dupe here, header is enough.

practicalswift · 2018-02-22T21:21:41Z

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

maflcko · 2018-02-22T21:26:44Z

utACK 96b6899e2c9b4a86698a6ea64460d045fde68c43

sipa · 2018-07-21T06:57:09Z

utACK 96b6899e2c9b4a86698a6ea64460d045fde68c43

maflcko · 2018-07-21T14:59:41Z

src/wallet/walletdb.cpp

@@ -524,7 +524,7 @@ DBErrors CWalletDB::LoadWallet(CWallet* pwallet)
    bool fNoncriticalErrors = false;
    DBErrors result = DB_LOAD_OK;

-    LOCK(pwallet->cs_wallet);
+    AssertLockHeld(pwallet->cs_wallet);


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

Could just remove it for now?

DrahtBot · 2018-07-21T17:29:37Z

The last travis run for this pull request was 148 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

practicalswift · 2018-07-21T17:47:57Z

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

Please re-review :-)

… thread 01a06d6 Avoid locking mutexes that are already held by the same thread (practicalswift) Pull request description: 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 :-) Tree-SHA512: e2fb85882e8800892fd8e8170f3c13128d6acfeb14d7b69fb9555f2b7ad0884fb201cf945b8144ffaf6fb1253c28af7c8c6c435319a7ae30ca003f28aa645a98

Summary: 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 :-) Backport of Bitcoin Core PR11762 bitcoin/bitcoin#11762 Test Plan: 1. Build with Clang in Debug mode: ``` CXX=clang++ CC=clang cmake .. -GNinja -DCMAKE_BUILD_TYPE=Debug ninja check ``` 2. Verify that the compiler has not emitted a `thread-safety` warning. 3. Run the node: `./src/bitcoind -regtest` 4. Verify that text similar to `"Assertion failed: lock ... not held ..."` is not printed on `stderr`. Reviewers: Fabien, #bitcoin_abc, deadalnix Reviewed By: Fabien, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D4145

…he same thread 01a06d6 Avoid locking mutexes that are already held by the same thread (practicalswift) Pull request description: 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 :-) Tree-SHA512: e2fb85882e8800892fd8e8170f3c13128d6acfeb14d7b69fb9555f2b7ad0884fb201cf945b8144ffaf6fb1253c28af7c8c6c435319a7ae30ca003f28aa645a98

zcash: cherry picked from commit 01a06d6 zcash: bitcoin/bitcoin#11762

…he same thread 01a06d6 Avoid locking mutexes that are already held by the same thread (practicalswift) Pull request description: 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 :-) Tree-SHA512: e2fb85882e8800892fd8e8170f3c13128d6acfeb14d7b69fb9555f2b7ad0884fb201cf945b8144ffaf6fb1253c28af7c8c6c435319a7ae30ca003f28aa645a98

fanquake added the Refactoring label Nov 24, 2017

practicalswift force-pushed the remove-double-locks branch from 9f857d5 to dc6cd0b Compare November 27, 2017 21:40

theuni reviewed Nov 28, 2017

View reviewed changes

practicalswift force-pushed the remove-double-locks branch from dc6cd0b to efc69c8 Compare November 29, 2017 20:11

practicalswift force-pushed the remove-double-locks branch from efc69c8 to 96b6899 Compare February 22, 2018 21:20

This was referenced Mar 11, 2018

mempool: Avoid locking mutex that is already held by the same thread #12671

Closed

[WIP] Add Clang thread safety analysis annotations: GUARDED_BY(lock) / EXCLUSIVE_LOCKS_REQUIRED(lock) #11226

Closed

practicalswift mentioned this pull request Apr 30, 2018

Meta-issue: Add Clang thread safety analysis annotations #13129

Closed

30 tasks

maflcko reviewed Jul 21, 2018

View reviewed changes

DrahtBot closed this Jul 21, 2018

DrahtBot reopened this Jul 21, 2018

Avoid locking mutexes that are already held by the same thread

01a06d6

practicalswift force-pushed the remove-double-locks branch from 96b6899 to 01a06d6 Compare July 21, 2018 17:46

maflcko merged commit 01a06d6 into bitcoin:master Jul 22, 2018

Bushstar mentioned this pull request Jul 24, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#357

Merged

LarryRuane mentioned this pull request Mar 24, 2021

Bitcoin 0.17 locking PRs zcash/zcash#5034

Open

practicalswift deleted the remove-double-locks branch April 10, 2021 19:35

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request Apr 26, 2021

Avoid locking mutexes that are already held by the same thread

6308962

zcash: cherry picked from commit 01a06d6 zcash: bitcoin/bitcoin#11762

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request May 27, 2021

Avoid locking mutexes that are already held by the same thread

55f58b3

zcash: cherry picked from commit 01a06d6 zcash: bitcoin/bitcoin#11762

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

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

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

Uh oh!

Conversation

practicalswift commented Nov 24, 2017 • edited by DrahtBot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Nov 27, 2017

Uh oh!

maflcko commented Nov 27, 2017

Uh oh!

practicalswift commented Nov 27, 2017

Uh oh!

practicalswift commented Nov 27, 2017

Uh oh!

practicalswift commented Nov 27, 2017

Uh oh!

maflcko commented Nov 27, 2017

Uh oh!

promag commented Nov 27, 2017

Uh oh!

practicalswift commented Nov 27, 2017

Uh oh!

theuni Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

practicalswift Nov 29, 2017

Choose a reason for hiding this comment

Uh oh!

theuni Dec 20, 2017

Choose a reason for hiding this comment

Uh oh!

ajtowns Jan 11, 2018

Choose a reason for hiding this comment

Uh oh!

theuni Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Feb 22, 2018

Uh oh!

maflcko commented Feb 22, 2018

Uh oh!

sipa commented Jul 21, 2018

Uh oh!

maflcko Jul 21, 2018

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jul 21, 2018

Uh oh!

practicalswift commented Jul 21, 2018

Uh oh!

Uh oh!

practicalswift commented Nov 24, 2017 •

edited by DrahtBot

Loading