Page MenuHomePhabricator

Clean up locking in tests to be consistent
Open, WishlistPublic

Description

In D1079, we discovered multiple locking mechanisms in place to prevent shared memory collisions in tests. Ideally, we should pick one method of locking and stick to it rather than every implementation under the sun. See @matra774's comment for details (copied here for reference):

Boost can execute some test in parallel. Previous version locked on wallet.cs_wallet. Since wallet was a global variable, that resulted in global lock. My commit preserves this behavior (global lock). Is it necessary? I am not sure - the code inside a unit test can access some other global resources that requires the lock.

If you take a look at other test cases (for example BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup) ), you can see that we have different locking patterns:

some of tests first lock on another global variable LOCK(cs_main), and later acquire the lock on local instance of wallet: LOCK(wallet.cs_wallet) (where wallet is a local variable).
some of the test lock both critical section at the same time LOCK2(cs_main, wallet.cs_wallet); - line 611, 652
other tests create a local instance of CWallet, but does not lock it at all - see lines 488, 652, 581
There could be some other lurking problems regarding test code and locks. For example :

test in line 538 (BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)) locks on cs_main and then calls SetMockTime, which sets global Variable
test in line 651 (BOOST_AUTO_TEST_CASE(ComputeTimeSmart)) also calls SetMockTime (through AddTx), but does not take any locks
If those two tests are executed at the same time, one of them should probably fail..

As I said, I wanted to preserve existing (global) locking mechanism, which actually serializes test execution - better safe then sorry :-) What do you propose:
a) get rid of LOCK(walletCriticalSection)
b) replace LOCK(walletCriticalSection) with LOCK (cs_main)
c) replace LOCK(walletCriticalSection) with local lock - LOCK(wallet.cs_wallet)
d) something else?

Event Timeline