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

laanwj · 2016-10-25T10:20:50Z

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 (Remaining instances of ENABLE_WALLET in libbitcoin_server.a #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 · 2016-10-25T18:33:16Z

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 · 2016-11-24T21:04:17Z

ACK.

sipa · 2016-11-25T20:28:55Z

utACK feaea7001fdec6c0ec5ef6cc1b33cb214131ce25

maflcko

utACK feaea7001fdec6c0ec5ef6cc1b33cb214131ce25

maflcko · 2016-11-26T11:52:14Z

src/init.cpp

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

No reason, will do that.

morcos · 2016-11-28T15:28:28Z

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

laanwj · 2016-11-28T15:59:40Z

@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 · 2016-11-28T16:25:03Z

@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 · 2016-11-28T16:30:24Z

@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 · 2016-11-28T16:49:31Z

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.

This allows doing some of the steps before e.g. daemonization and some fater.

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.

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 · 2016-11-29T11:57:54Z

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 · 2016-11-29T14:46:26Z

ACK deec83f

maflcko · 2016-11-30T00:16:10Z

reACK deec83f. Good finding @morcos!

…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)

… 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)

… 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

laanwj added Refactoring Linux/Unix labels Oct 25, 2016

laanwj mentioned this pull request Oct 27, 2016

Cannot obtain lock warning now isn't visible when daemonized. #9009

Closed

maflcko approved these changes Nov 26, 2016

View reviewed changes

maflcko reviewed Nov 26, 2016

View reviewed changes

laanwj added 3 commits November 29, 2016 12:47

init: Split up AppInit2 into multiple phases

0cc8b6b

This allows doing some of the steps before e.g. daemonization and some fater.

init: Get rid of fServer flag

deec83f

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 force-pushed the 2016_10_init_split_daemon branch from feaea70 to deec83f Compare November 29, 2016 11:55

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

laanwj mentioned this pull request Dec 5, 2016

Some minor testing cleanups #9276

Merged

laanwj mentioned this pull request Feb 23, 2017

Trivial: fix comments referencing AppInit2 #9833

Merged

furszy mentioned this pull request Dec 17, 2020

[WIP][Validation] Back ports up to "bye bye" boost signal usages. PIVX-Project/PIVX#1973

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

laanwj commented Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Oct 25, 2016

Uh oh!

gmaxwell commented Nov 24, 2016

Uh oh!

sipa commented Nov 25, 2016

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 26, 2016

Choose a reason for hiding this comment

Uh oh!

laanwj Nov 28, 2016

Choose a reason for hiding this comment

Uh oh!

morcos commented Nov 28, 2016

Uh oh!

laanwj commented Nov 28, 2016

Uh oh!

morcos commented Nov 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Nov 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morcos commented Nov 28, 2016

Uh oh!

laanwj commented Nov 29, 2016

Uh oh!

morcos commented Nov 29, 2016

Uh oh!

maflcko commented Nov 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

laanwj commented Oct 25, 2016 •

edited

Loading

morcos commented Nov 28, 2016 •

edited

Loading

laanwj commented Nov 28, 2016 •

edited

Loading