Skip to content
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.

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

Split up AppInit2 into multiple phases, daemonize after datadir lock errors #9010

Merged
merged 3 commits into from Dec 1, 2016

Conversation

@laanwj
Copy link
Member

commented Oct 25, 2016

Split up AppInit2 into multiple steps: This allows doing some of the steps before daemonization and some later.

To avoid the issue described in #9009 (comment):

  • Before daemonization, just probe the data directory lock and print an early error message if possible.
  • After daemonization get the data directory lock again and hold on to it until exit. This creates a slight window for a race condition to happen, however this condition is harmless: it will at most make the process exit without printing a message to console as it will be detected after daemonization.

Solves #9009:

$ src/bitcoind -testnet -daemon
Bitcoin server starting
$ src/bitcoind -testnet -daemon
Error: Cannot obtain a lock on data directory /home/orion/.bitcoin/testnet3. Bitcoin Core is probably already running.

As for the AppInit2 split: This is something I've been wanting to do for a while. Note that, to reduce risk of regressions, this is just mechanic code movement and doesn't change ordering, besides the daemon() invocation.

Some smarter things could be done such as:

  • Move basic context initialization from AppInitBasicSetup() to SetupEnvironment() where possible, especially things such as the windows DEP activation
  • Unify InitParameterInteraction() and AppInitParameterInteraction() if/where possible. I'm not sure why this is done in two steps with just AppInitBasicSetup() in between right now.
  • Do something smarter than // Variables internal to initialization process only (e.g. an initialization context structure). Some variables there may be completely avoided by moving the code to extract the arguments.
  • Splitting up further steps, this will allow factoring out ENABLE_WALLET sections from libbitcoin_server.a (which contains init.cpp) and remove the circular dependencies (#7965)
  • It is now possible to do unit testing on our initialization steps in isolation.

However I do not intend to do these things in this pull.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Oct 25, 2016

Code Review utACK feaea7001fdec6c0ec5ef6cc1b33cb214131ce25.
And yes, I think we should unify InitParameterInteraction and AppInitParameterInteraction() to avoid confusion (can be done in a later PR).

@gmaxwell

This comment has been minimized.

Copy link
Contributor

commented Nov 24, 2016

ACK.

@sipa

This comment has been minimized.

Copy link
Member

commented Nov 25, 2016

utACK feaea7001fdec6c0ec5ef6cc1b33cb214131ce25

@MarcoFalke
Copy link
Member

left a comment

utACK feaea7001fdec6c0ec5ef6cc1b33cb214131ce25

src/init.cpp Outdated
@@ -977,8 +988,7 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
// Option to startup with mocktime set (used for regression testing):
SetMockTime(GetArg("-mocktime", 0)); // SetMockTime(0) is a no-op

ServiceFlags nLocalServices = NODE_NETWORK;
ServiceFlags nRelevantServices = NODE_NETWORK;
nLocalServices = NODE_NETWORK;

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Nov 26, 2016

Member

Any reason the default value was not assigned in the namespace (like nRelevantServices)?

This comment has been minimized.

Copy link
@laanwj

laanwj Nov 28, 2016

Author Member

No reason, will do that.

@morcos

This comment has been minimized.

Copy link
Member

commented Nov 28, 2016

@laanwj I don't think you're setting fServer correctly. I think you need to move the SoftSetBoolArg("-server", true) to before AppInitParameterInteraction.

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2016

@laanwj I don't think you're setting fServer correctly. I think you need to move the SoftSetBoolArg("-server", true) to before AppInitParameterInteraction.

There isn't any parameter interaction based on -server, so I don't think this makes a difference in practice. It'd be more logical to move it though, so I will.

@morcos

This comment has been minimized.

Copy link
Member

commented Nov 28, 2016

@laanwj fServer gets set in AppInitParameterInteraction. I compiled this PR and ran src/bitcoind -daemon and was unable to connect to bitcoind using bitcoin-cli.

EDIT: Perhaps the right solution is to remove fServer

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2016

@laanwj fServer gets set in AppInitParameterInteraction. I compiled this PR and ran src/bitcoind -daemon and was unable to connect to bitcoind using bitcoin-cli.

Oh you're right, sorry.
I expected the RPC tests to catch this but apparently they set -server explicitly.

qa/rpc-tests/test_framework/util.py:            args = [ os.getenv("BITCOIND", "bitcoind"), "-server", "-keypool=1", "-datadir="+datadir, "-discover=0" ]
qa/rpc-tests/test_framework/util.py:    args = [ binary, "-datadir="+datadir, "-server", "-keypool=1", "-discover=0", "-rest", "-mocktime="+str(get_mocktime()) ]

EDIT: Perhaps the right solution is to remove fServer

It exists to make running a RPC server optional (disabled by default) with the GUI. I think we should keep that.
In any case the goal here is not to change the working of any options, so the setting should just be moved...

@morcos

This comment has been minimized.

Copy link
Member

commented Nov 28, 2016

Yes I just meant get rid of the variable fServer since its used only once and in AppInitMain just check GetBoolArg("-server", false) directly, but either way.

laanwj added 3 commits Oct 25, 2016
init: Split up AppInit2 into multiple phases
This allows doing some of the steps before e.g. daemonization and some
fater.
init: Try to aquire datadir lock before and after daemonization
Before daemonization, just probe the data directory lock and print an
early error message if possible.

After daemonization get the data directory lock again and hold on to it until exit
This creates a slight window for a race condition to happen, however this condition is harmless: it
will at most make us exit without printing a message to console.

    $ src/bitcoind -testnet -daemon
    Bitcoin server starting
    $ src/bitcoind -testnet -daemon
    Error: Cannot obtain a lock on data directory /home/orion/.bitcoin/testnet3. Bitcoin Core is probably already running.
init: Get rid of fServer flag
There is no need to store this flag globally, the variable is only used
inside the initialization process.

Thanks to Alex Morcos for the idea.

@laanwj laanwj force-pushed the laanwj:2016_10_init_split_daemon branch to deec83f Nov 29, 2016

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2016

Yes I just meant get rid of the variable fServer since its used only once and in AppInitMain just check GetBoolArg("-server", false) directly, but either way.

Good idea. I pushed these changes:

  • Initialize nRelevantServices in the namespace like nLocalServices (MarcoFalke's comment)
  • Move SetSoftBool("-server") to before parameter interaction in bitcoind.cpp, in case someone adds parameter interaction based on this
  • Get rid of global fServer flag
@morcos

This comment has been minimized.

Copy link
Member

commented Nov 29, 2016

ACK deec83f

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 30, 2016

reACK deec83f. Good finding @morcos!

@sipa sipa merged commit deec83f into bitcoin:master Dec 1, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
sipa added a commit that referenced this pull request Dec 1, 2016
Merge #9010: Split up AppInit2 into multiple phases, daemonize after …
…datadir lock errors

deec83f init: Get rid of fServer flag (Wladimir J. van der Laan)
16ca0bf init: Try to aquire datadir lock before and after daemonization (Wladimir J. van der Laan)
0cc8b6b init: Split up AppInit2 into multiple phases (Wladimir J. van der Laan)
@laanwj laanwj referenced this pull request Dec 5, 2016
codablock added a commit to codablock/dash that referenced this pull request Jan 17, 2018
Merge bitcoin#9010: Split up AppInit2 into multiple phases, daemonize…
… after datadir lock errors

deec83f init: Get rid of fServer flag (Wladimir J. van der Laan)
16ca0bf init: Try to aquire datadir lock before and after daemonization (Wladimir J. van der Laan)
0cc8b6b init: Split up AppInit2 into multiple phases (Wladimir J. van der Laan)
andvgal added a commit to energicryptocurrency/energi that referenced this pull request Jan 6, 2019
Merge bitcoin#9010: Split up AppInit2 into multiple phases, daemonize…
… after datadir lock errors

deec83f init: Get rid of fServer flag (Wladimir J. van der Laan)
16ca0bf init: Try to aquire datadir lock before and after daemonization (Wladimir J. van der Laan)
0cc8b6b init: Split up AppInit2 into multiple phases (Wladimir J. van der Laan)
CryptoCentric added a commit to absolute-community/absolute that referenced this pull request Feb 25, 2019
Merge bitcoin#9010: Split up AppInit2 into multiple phases, daemonize…
… after datadir lock errors

deec83f init: Get rid of fServer flag (Wladimir J. van der Laan)
16ca0bf init: Try to aquire datadir lock before and after daemonization 
(Wladimir J. van der Laan)
0cc8b6b init: Split up AppInit2 into multiple phases (Wladimir J. va
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.