tests: Add missing locks to tests #11623

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:test-locks changing 7 files +60 −10

practicalswift commented at 10:14 PM on November 6, 2017: contributor

Add missing locks to tests to satisfy lock requirements (such as EXCLUSIVE_LOCKS_REQUIRED(...) (Clang Thread Safety Analysis, see #11226), AssertLockHeld(...) and implicit lock assumptions).
fanquake added the label Tests on Nov 6, 2017
promag commented at 1:07 AM on November 7, 2017: member

utACK 361136d.
practicalswift cross-referenced this on Nov 7, 2017 from issue Assert cs_main is held when retrieving node state by promag
in src/test/DoS_tests.cpp:66 in 361136d45e outdated
```
  65 | @@ -66,11 +66,14 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
  66 |      dummyNode1.fSuccessfullyConnected = true;
```
laanwj commented at 7:39 AM on November 8, 2017:

In tests where no multi-threading happens (most of them) it would work just as well to just take the necessary locks once in the beginning of each test case. That would make the diff smaller likely. (but maybe explicit is better, I don't know)

practicalswift commented at 8:41 AM on November 8, 2017:

@laanwj Do you know which tests in this PR that do not have any multi-threading? What is the best way to identify the tests for which multi-threading happens?

(Personally I like explicit locking better since that communicates where/why the lock is needed which is helpful for locking newcomers. I'll happily adjust to consensus however :-))

laanwj commented at 8:51 AM on November 8, 2017:

Yeah I'd say just keep it like this...
ryanofsky cross-referenced this on Nov 10, 2017 from issue [WIP] Add Clang thread safety analysis annotations: GUARDED_BY(lock) / EXCLUSIVE_LOCKS_REQUIRED(lock) by practicalswift
in src/test/miner_tests.cpp:84 in 361136d45e outdated
```
  80 | @@ -81,7 +81,7 @@ CBlockIndex CreateBlockIndex(int nHeight)
  81 |  
  82 |  bool TestSequenceLocks(const CTransaction &tx, int flags)
  83 |  {
  84 | -    LOCK(mempool.cs);
  85 | +    LOCK2(cs_main, mempool.cs);
```
MarcoFalke commented at 3:30 PM on November 10, 2017:

The lock is already acquired in src/test/miner_tests.cpp:208, no?

practicalswift commented at 4:16 PM on November 10, 2017:

Good point! Fixed!
MarcoFalke commented at 3:32 PM on November 10, 2017: member

utACK 361136d45e354e303fb96ce25c4b3945505633ab

tests: Add missing locks to tests

Add missing locks to tests to satisfy lock requirements (such as
EXCLUSIVE_LOCKS_REQUIRED(...) (Clang Thread Safety Analysis),
AssertLockHeld(...) and implicit lock assumptions).

109a858995

practicalswift force-pushed on Nov 10, 2017
MarcoFalke commented at 4:56 PM on November 10, 2017: member

re-utACK 109a85899571aa499572e211bb08f05715e8743b
MarcoFalke merged this on Nov 10, 2017
MarcoFalke closed this on Nov 10, 2017
MarcoFalke referenced this in commit ee92243e66 on Nov 10, 2017
promag commented at 4:59 PM on November 10, 2017: member

re-utACK 109a858.
practicalswift cross-referenced this on Apr 30, 2018 from issue Meta-issue: Add Clang thread safety analysis annotations by practicalswift
PastaPastaPasta referenced this in commit a3cd9783af on Dec 27, 2019
PastaPastaPasta referenced this in commit ba8f6a31a2 on Jan 2, 2020
PastaPastaPasta referenced this in commit 675f655722 on Jan 4, 2020
PastaPastaPasta referenced this in commit 76c527d49d on Jan 12, 2020
PastaPastaPasta referenced this in commit a36d0ef602 on Jan 12, 2020
PastaPastaPasta referenced this in commit be7ff9c186 on Jan 12, 2020
PastaPastaPasta referenced this in commit 6db34655f5 on Jan 12, 2020
PastaPastaPasta referenced this in commit 523338304e on Jan 12, 2020
PastaPastaPasta referenced this in commit 2377ac6533 on Jan 12, 2020
PastaPastaPasta referenced this in commit 58adb1c28d on Jan 16, 2020
PastaPastaPasta referenced this in commit e5d9dba3e9 on Jan 22, 2020
PastaPastaPasta referenced this in commit efea72890c on Jan 22, 2020
PastaPastaPasta referenced this in commit 0c4c5ff939 on Jan 29, 2020
PastaPastaPasta referenced this in commit a3ad3120bf on Jan 29, 2020
ckti referenced this in commit 2c0e302702 on Mar 28, 2021
practicalswift deleted the branch on Apr 10, 2021
gades referenced this in commit 386e85aae4 on Jun 30, 2021
gades referenced this in commit 2d10e92600 on Feb 10, 2022
bitcoin locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#11226 [WIP] Add Clang thread safety analysis annotations: GUARDED_BY(lock) / EXCLUSIVE_LOCKS_REQUIRED(lock)#11515 Assert cs_main is held when retrieving node state #13129 Meta-issue: Add Clang thread safety analysis annotations