Locking robustness fixes #4085

pull laanwj wants to merge 3 commits into bitcoin:master from laanwj:2014_04_fix_lockorder_tests changing 7 files +40 −19

laanwj commented at 7:10 AM on April 23, 2014: member
- Add required locks in tests (fixes #4084)
- Get required locks up-front in GUI updates and use a try_lock to skip when lock already held (fixes #4083)
- Add missing AssertLockHeld in ConnectBlock (missed this externally-callable function in #4058)

Add required locks in tests

Unit tests with DEBUG_LOCKORDER were running into assertions.

ed67100565

qt: get required locks upfront in polling functions

This avoids the GUI from getting stuck on
periodical polls if the core is holding the locks for a longer time -
for example, during a wallet rescan.

41106a50d2

Add missing AssertLockHeld in ConnectBlock b39a07dc42

laanwj commented at 7:23 AM on April 23, 2014: member

Having fixed this I get a different exception (not related to locking) on shutdown now sometimes when I try to reproduce #4083,

bitcoin-qt: main.cpp:1737: bool ConnectBlock(CBlock&, CValidationState&, CBlockIndex*, CCoinsViewCache&, bool): Assertion `hashPrevBlock == view.GetBestBlock()' failed.

Traceback:

[#4](/bitcoin-bitcoin/4/)  0x00005555556c5afa in ConnectBlock (block=..., state=..., 
    pindex=pindex@entry=0x7fff78853bc0, view=..., 
    fJustCheck=fJustCheck@entry=false) at main.cpp:1737
[#5](/bitcoin-bitcoin/5/)  0x00005555556c7f96 in ConnectTip (pindexNew=0x7fff78853bc0, state=...)
    at main.cpp:2001
[#6](/bitcoin-bitcoin/6/)  ActivateBestChain (state=...) at main.cpp:2110
[#7](/bitcoin-bitcoin/7/)  0x00005555556c9508 in AddToBlockIndex (block=..., state=..., pos=...)
    at main.cpp:2173
[#8](/bitcoin-bitcoin/8/)  0x00005555556c98ff in AcceptBlock (block=..., state=..., dbp=dbp@entry=0x0)
    at main.cpp:2438
[#9](/bitcoin-bitcoin/9/)  0x00005555556ca8b0 in ProcessBlock (state=..., 
    pfrom=pfrom@entry=0x7fff84000d80, pblock=pblock@entry=0x7fff8effc5e0, 
    dbp=dbp@entry=0x0) at main.cpp:2557
[#10](/bitcoin-bitcoin/10/) 0x00005555556cf4ac in ProcessMessage (pfrom=pfrom@entry=0x7fff84000d80, 
    strCommand=..., vRecv=...) at main.cpp:3815
[#11](/bitcoin-bitcoin/11/) 0x00005555556d0cc2 in ProcessMessages (pfrom=0x7fff84000d80)
    at main.cpp:4127
[#12](/bitcoin-bitcoin/12/) 0x000055555572d714 in operator() (a0=<optimized out>, this=<optimized out>)
    at /usr/include/boost/function/function_template.hpp:767
[#13](/bitcoin-bitcoin/13/) m_invoke (connectionBody=..., this=<optimized out>)
    at /usr/include/boost/signals2/detail/signal_template.hpp:368
[#14](/bitcoin-bitcoin/14/) operator() (connectionBody=..., this=<optimized out>)
    at /usr/include/boost/signals2/detail/signal_template.hpp:345
[#15](/bitcoin-bitcoin/15/) dereference (this=0x7fff8effcb30)
    at /usr/include/boost/signals2/detail/slot_call_iterator.hpp:82
:_List_iterator<boost::shared_ptr<boost::signals2::detail::connection_body<std::pair<boost::signals2::detail::slot_meta_group, boost::optional<int> >, boost::signals2::slot1<bool, CNode*, boost::function<bool(CNode*)> >, boost::signals2::mutex> > >, boost::signals2::detail::connection_body<std::pair<boost::signals2::detail::slot_meta_group, boost::optional<int> >, boost::signals2::slot1<bool, CNode*, boost::function<bool(CNode*)> >, boost::signals2::mutex> > > (f=...)
    at /usr/include/boost/iterator/iterator_facade.hpp:514
[#17](/bitcoin-bitcoin/17/) operator* (this=0x7fff8effcb30)
    at /usr/include/boost/iterator/iterator_facade.hpp:639
... (lots of boost::signal stuff)
[#21](/bitcoin-bitcoin/21/) 0x000055555571803d in operator() (arg1=0x7fff84000d80, 
    this=0x555555eca778 <g_signals+24>)
    at /usr/include/boost/signals2/detail/signal_template.hpp:695
[#22](/bitcoin-bitcoin/22/) ThreadMessageHandler () at net.cpp:1540
[#23](/bitcoin-bitcoin/23/) 0x000055555571ffb3 in TraceThread<void (*)()> (
    name=0x555555b43561 "msghand", 
    func=0x555555717d90 <ThreadMessageHandler()>) at util.h:576

laanwj added this to the milestone 0.9.2 on Apr 23, 2014
BitcoinPullTester commented at 7:37 AM on April 23, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/b39a07dc42ab6ba746a25206969fb81913146f1f for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
rdponticelli commented at 10:39 AM on April 23, 2014: contributor

That looks like the same issue I had when I ran master with 55a1db4fa2cf62b9766ef382c1e14b3ecbdf67fe reverted.
laanwj commented at 11:30 AM on April 23, 2014: member
I think I know what happens:
- CheckDiskSpace calls StartShutdown and returns false.
  
  This error propagates up the call chain, causing ProcessBlock to fail, leaving the view in an inconsistent state.
- After ProcessBlock fails, the message loop continues as normal (see https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L3812 ).
Somehow it manages to process another message before the GUI thread detects that a shutdown is requested.

The solution here would be to either not leave the view in an inconsistent state when failing, change the assertion to a normal error, and/or make sure that all message loops are exited when a fatal exception (such as disk full) occurs.

In any case, it is not a problem with the changes in this pull, but a problem that existed before.
gavinandresen commented at 2:55 PM on April 23, 2014: contributor

ACK
laanwj merged this on Apr 23, 2014
laanwj closed this on Apr 23, 2014
laanwj referenced this in commit 89bbd54fbf on Apr 23, 2014
DrahtBot locked this on Sep 8, 2021

Contributors

Linked (view graph)

#4058 Solve chainActive-related locking issues #4083 Deadlock on exit after low disk space during syncing #4084 Unit tests failing with -DDEBUG_LOCKORDER