Skip to content

Locking robustness fixes #4085

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Apr 23, 2014
Merged

Locking robustness fixes #4085

merged 3 commits into from
Apr 23, 2014

Conversation

laanwj
Copy link
Member

@laanwj laanwj commented Apr 23, 2014

laanwj added 3 commits April 23, 2014 08:05
@laanwj
Add required locks in tests
ed67100 
Unit tests with DEBUG_LOCKORDER were running into assertions.
@laanwj
qt: get required locks upfront in polling functions
41106a5 
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.
@laanwj
Add missing AssertLockHeld in ConnectBlock
b39a07d
@laanwj laanwj mentioned this pull request Apr 23, 2014
Closed
@laanwj
Copy link
Member Author

laanwj commented Apr 23, 2014

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

@laanwj laanwj added this to the 0.9.2 milestone Apr 23, 2014
@BitcoinPullTester
Copy link

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
Copy link
Contributor

That looks like the same issue I had when I ran master with 55a1db4 reverted.

@laanwj
Copy link
Member Author

laanwj commented Apr 23, 2014

I think I know what happens:

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
Copy link
Contributor

ACK

@cozz cozz mentioned this pull request Apr 23, 2014
Merged
@laanwj laanwj merged commit b39a07d into bitcoin:master Apr 23, 2014
laanwj added a commit that referenced this pull request Apr 23, 2014
@laanwj
Merge pull request #4085
89bbd54 
b39a07d Add missing AssertLockHeld in ConnectBlock (Wladimir J. van der Laan)
41106a5 qt: get required locks upfront in polling functions (Wladimir J. van der Laan)
ed67100 Add required locks in tests (Wladimir J. van der Laan)
@laanwj laanwj mentioned this pull request Apr 23, 2014
Closed
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unit tests failing with -DDEBUG_LOCKORDER Deadlock on exit after low disk space during syncing
4 participants
@laanwj @BitcoinPullTester @rdponticelli @gavinandresen