Keep a flag in init.cpp 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.
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.
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 73b6b4e7c0a450bb88074a7d59a9237b4703302b
utACK 73b6b4e7c0a450bb88074a7d59a9237b4703302b
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.
74 | @@ -75,6 +75,9 @@ static const bool DEFAULT_STOPAFTERBLOCKIMPORT = false; 75 | std::unique_ptr<CConnman> g_connman; 76 | std::unique_ptr<PeerLogicValidation> peerLogic; 77 | 78 | +/** Set at startup if the data directory was succesfully locked */ 79 | +bool g_locked_data_directory = false;
Should be static and atomic?
static ok, atomic no, there's no concurrent modification as the flag is set from one place in the initialization sequence.
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).
The point, which I explain in #10815 (comment), is that that mechanism ALSO doesn't work for bitcoind.
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.
The point of that 'strange sequence' is to be able to give a proper error in the case of -daemon.
If you take the data directory lock before forking, bad things happen. If you only take it after the fork, it's impossible to print an error to the standard output as that has been detached. That's why the probing is done.
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.