Added ability to respond to signals during Block Loading stage. #959

rebroad commented at 4:34 PM on March 20, 2012: contributor

As the Loading of the Block Index can take several minutes, it's possible the user might want to exit the process during this, and so far the only way was with a kill -9. This small change allows the Block Index Loading to watch out for a requested exit, and abort during the process.

It's been tested on my computer so far.

Without this change: the bitcoin-qt process is unkillable (without a kill -9) during the initial Loading of the Block Index. This goes against standards for processes, which should try to exit cleanly as soon as a kill signal is sent.

luke-jr commented at 5:18 AM on March 21, 2012: member

Don't ask me how, but this makes bitcoind and Bitcoin-Qt both segfault at startup (after " wallet 9172ms")... (I didn't try to interrupt anything)

laanwj commented at 6:05 AM on March 21, 2012: member

Returning true from AppInit2 implies that initialization succeeded, it should return false when interrupted.

rebroad commented at 7:33 AM on March 21, 2012: contributor

@luke-jr - I'll assume you mean every time. Have you compared with the version my changes were based on? What does gdb show? My changes are so few, and especially if you're not exiting during the load, I struggle to believe it would be due to this change.

rebroad commented at 7:44 AM on March 21, 2012: contributor

@laanwj I initially coded it to return false but changed it to true as 1) it made the code smaller, 2) it's not failing, 3) would you normally expect a return code of false from any program that exited only because the user requested it?

In answer to 3, the answer is no, and so I believe and function should follow that logic.

Returning true doesn't go against and C standards that I know of either.

luke-jr commented at 7:54 AM on March 21, 2012: member

I couldn't get a useful backtrace out of gdb. git bisect blames this change, and indeed removing it fixes the problem.

laanwj commented at 8:03 AM on March 21, 2012: member

How does this "make the code smaller" if you return true? The UI code will assume that initialization was successful and display the main window, right?

Edit: and yes, many programs return non-zero return status when forced to terminate by a signal.

rebroad commented at 8:03 AM on March 21, 2012: contributor

@luke-jr What OS are you using?

P.S. @luke-jr are you sure you're not some Manchurian candidate or perhaps a cylon without knowing it?!

luke-jr commented at 8:06 AM on March 21, 2012: member

Gentoo GNU/Linux (32-bit x86 stable)

rebroad commented at 8:12 AM on March 21, 2012: contributor

@laanwj No. But if LoadBlockIndex returns false then AppInit2 will display an error without additional code.

rebroad commented at 8:13 AM on March 21, 2012: contributor

@luke-jr Ok, so how would you code it to allow it to be killed during the block index load?

luke-jr commented at 8:14 AM on March 21, 2012: member

I don't see anything wrong with the way you did it. Maybe when I'm more awake I can be of more help tracking down the segfault.

laanwj commented at 8:16 AM on March 21, 2012: member

I'm not saying that LoadBlockIndex should return false in this case, but AppInit2 should.

See src/qt/bitcoin.cpp. If AppInit2 returns true, it will spin up the UI.

laanwj commented at 8:21 AM on March 21, 2012: member

Though this shows the limitations of using boolean return values to signify conditions more than anything else :-)

rebroad commented at 7:52 PM on March 21, 2012: contributor

@laanwj - ah, so it does. Thanks, I will change this. I'm currently debugging to find the cause of the SIGSEGV @luke-jr mentioned.

rebroad commented at 11:31 PM on March 21, 2012: contributor

ok. There was a bug, which I've fixed with the fixup. This was also the cause of the SEGV initially reported by @luke-jr. Thanks Luke!

rebroad commented at 8:55 PM on March 26, 2012: contributor

Hi @luke-jr, would you be willing to ACK this please to confirm that the issue you found has been resolved please?

sipa commented at 10:42 AM on April 4, 2012: member

ACK on the changes. The direct "return true" from AppInit worried me, but the only thing necessary is shutting down the database environment, which happens automatically anyway.

Can you rebase this into one commit though?

gavinandresen commented at 5:54 PM on April 17, 2012: contributor

ACK assuming luke's issue isn't an issue.

jgarzik commented at 8:47 PM on April 17, 2012: contributor

Seems sane, though it would be nice if the two commits were combined into a single one, before it hits upstream.

luke-jr commented at 8:48 PM on April 17, 2012: member

It seems to be in the next-test that I've been using regularly for a week now, so it's probably safe.

I'd want to test it out specifically before I give it an "ACK" myself, but don't wait on me.

rebroad commented at 11:01 PM on April 17, 2012: contributor

sorry I've not combined these two changes into one. If someone can tell me the git commands to do it, I'd be happy to oblige. :-s

sipa commented at 11:31 PM on April 17, 2012: member

go to your working directory, checkout this branch ("git checkout LoadBlockIndexKillable"), fetch upstream ("git fetch upstream"), rebase against master ("git rebase -i upstream/master"), you should see an editor now with two commits listed, change the "pick" of the second on to "fixup" (meaning that it needs to me combined with the previous one), save and exit the editor. You should now have a rewritten branch with one combined commit. Push that to github (using git push -f).

Added ability to respond to signals during Block Loading stage. 871c3557bf

sipa commented at 11:36 AM on April 18, 2012: member

Do a "git reset --hard" in advance. This will throw away any local changes you may have made in the current branch. If you don't want that, commit them first. If you want to chat about it, come to #bitcoin-dev on irc.freenode.org (using an IRC client, or on http://webchat.freenode.net/?channels=bitcoin-dev)

rebroad commented at 11:37 AM on April 18, 2012: contributor

ok, that worked.. (except I had to do a "git stash" before the rebase - I'm not sure why it said there were other changes...)

sipa commented at 8:05 PM on April 18, 2012: member

ACK

laanwj commented at 6:10 AM on April 19, 2012: member

ACK

sipa referenced this in commit 3b9e6b7820 on Apr 19, 2012

sipa merged this on Apr 19, 2012

sipa closed this on Apr 19, 2012

coblee referenced this in commit 912cc1fb59 on Jul 17, 2012

DrahtBot locked this on Sep 8, 2021