init: Keep track of whether data directory locked, don't cleanup if not#10818
init: Keep track of whether data directory locked, don't cleanup if not#10818laanwj wants to merge 1 commit intobitcoin:masterfrom
Conversation
Keep a flag indicating whether the data directory was locked. If not, Interrupt and Shutdown are no-ops. This avoids things from being cleaned up if they were created by another instance.
|
This seems right to me and it solved the problem, but I'm not confident enough in understanding startup/shutdown to actually give it an ACK. Thanks! |
|
utACK 73b6b4e |
|
utACK 73b6b4e |
1 similar comment
|
utACK 73b6b4e |
|
Why not just make AppInitSanityCheck actually take the lock instead of just probing? It seems strange to call the entire shutdown sequence instead when literally none of it is needed when we already have a framework for skipping it (ala if AppinitSanityCheck fails, except for GUI, which really should be fixed in GUI, not by adding a boolean). |
|
@TheBlueMatt I'm confused how that would help here. |
|
My point is that this is (effecitlvely) not a problem in bitcoind. The issue in bitcoin-qt is that it calls Shutdown() where it should not (and bitcoind does not). If we want to separately fix the race that is also present in bitcoind, we should stop doing this strange LockDataDirectory(true)...one function return/call later...LockDataDirectory(false) thing. |
| std::unique_ptr<PeerLogicValidation> peerLogic; | ||
|
|
||
| /** Set at startup if the data directory was succesfully locked */ | ||
| bool g_locked_data_directory = false; |
There was a problem hiding this comment.
Should be static and atomic?
There was a problem hiding this comment.
static ok, atomic no, there's no concurrent modification as the flag is set from one place in the initialization sequence.
The point, which I explain in #10815 (comment), is that that mechanism ALSO doesn't work for bitcoind.
The point of that 'strange sequence' is to be able to give a proper error in the case of Do you mean calling LockDataDirectory in a separate initialization step after daemonizing? That could work, though as it is already so easy to introduce bugs like this I thought it was more robust to add a flag. |
|
Closign in favor of #10832 |
Alternative to bitcoin#10818, alternative solution to bitcoin#10815. After this change: All the AppInit steps before and inclusive AppInitLockDataDirectory must not have Shutdown() called in case of failure. Only when AppInitMain fails, Shutdown should be called. Changes the GUI and bitcoind code to consistently do this.
…up core dump issue dba485d init: Factor out AppInitLockDataDirectory (Wladimir J. van der Laan) Pull request description: Alternative to #10818, alternative solution to #10815. After this change: All the AppInit steps before and inclusive AppInitLockDataDirectory must not have Shutdown() called in case of failure. Only when AppInitMain fails, Shutdown should be called. Changes the GUI and bitcoind code to consistently do this. Tree-SHA512: 393e1a0ae05eb8e791025069e3ac4f6f3cdeb459ec63feda85d01cf6696ab3fed7632b6a0ac3641b8c7015af51d46756b5bba77f5e5f0c446f0c2dea58bbc92e
Alternative to bitcoin#10818, alternative solution to bitcoin#10815. After this change: All the AppInit steps before and inclusive AppInitLockDataDirectory must not have Shutdown() called in case of failure. Only when AppInitMain fails, Shutdown should be called. Changes the GUI and bitcoind code to consistently do this.
…x startup core dump issue dba485d init: Factor out AppInitLockDataDirectory (Wladimir J. van der Laan) Pull request description: Alternative to bitcoin#10818, alternative solution to bitcoin#10815. After this change: All the AppInit steps before and inclusive AppInitLockDataDirectory must not have Shutdown() called in case of failure. Only when AppInitMain fails, Shutdown should be called. Changes the GUI and bitcoind code to consistently do this. Tree-SHA512: 393e1a0ae05eb8e791025069e3ac4f6f3cdeb459ec63feda85d01cf6696ab3fed7632b6a0ac3641b8c7015af51d46756b5bba77f5e5f0c446f0c2dea58bbc92e
8 participants
Keep a flag in
init.cppindicating whether the data directory was locked.If not, Interrupt and Shutdown are no-ops. This avoids things from being cleaned up if they were created by another instance.
I think this is the most robust, sure solution to #10815.
n.b.: We can't simple do "don't call Interrupt/Shutdown if Init*() failed" because some of the things needs to be interrupted and shut down in case of an error later in initialization when some things have already been started.