Added ability to respond to signals during Block Loading stage. #959
Conversation
|
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) |
|
Returning true from AppInit2 implies that initialization succeeded, it should return false when interrupted. |
|
@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. |
|
@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. |
|
I couldn't get a useful backtrace out of gdb. git bisect blames this change, and indeed removing it fixes the problem. |
|
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. |
|
Gentoo GNU/Linux (32-bit x86 stable) |
|
@laanwj No. But if LoadBlockIndex returns false then AppInit2 will display an error without additional code. |
|
@luke-jr Ok, so how would you code it to allow it to be killed during the block index load? |
|
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. |
|
I'm not saying that LoadBlockIndex should return false in this case, but AppInit2 should. See |
|
Though this shows the limitations of using boolean return values to signify conditions more than anything else :-) |
|
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! |
|
Hi @luke-jr, would you be willing to ACK this please to confirm that the issue you found has been resolved please? |
|
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? |
|
ACK assuming luke's issue isn't an issue. |
|
Seems sane, though it would be nice if the two commits were combined into a single one, before it hits upstream. |
|
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. |
|
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 |
|
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). |
|
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) |
|
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...) |
|
ACK |
1 similar comment
|
ACK |
Added ability to respond to signals during Block Loading stage.
Added ability to respond to signals during Block Loading stage.
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.