Break circular dependency: init -> * -> init by extracting shutdown.h #13235

Empact · 2018-05-15T01:40:15Z

Most includers just wanted to react to pending shutdown.

This isolates access to fRequestShutdown and limits access to the shutdown api functions, including the new CancelShutdown for setting it to false.

Empact · 2018-05-15T01:41:33Z

Prompted by #13228

fanquake · 2018-05-15T04:15:20Z

Failures in bench_bitcoin in a few of the Travis builds:

  CXXLD    bench/bench_bitcoin
libbitcoin_wallet.a(libbitcoin_wallet_a-wallet.o): In function `CWalletTx::RelayWalletTransaction(CConnman*)':
wallet.cpp:(.text+0x5cea): undefined reference to `CConnman::NodeFullyConnected(CNode const*)'
collect2: error: ld returned 1 exit status
make[2]: *** [bench/bench_bitcoin] Error 1

practicalswift · 2018-05-15T07:19:09Z

Concept ACK

laanwj · 2018-05-15T07:35:01Z

I entertained the idea of creating a shutdown.(cpp|h) a few times as well. It seems we have too much non-initialization-related things in init. Concept ACK.

Empact · 2018-05-15T20:02:26Z

Based this on #13239, which fixes the linker issues.

jonasschnelli · 2018-05-16T06:40:39Z

utACK f7033bf5767e674abfee2bd53cd6b9f5c3831482

practicalswift · 2018-05-16T07:06:12Z

utACK f7033bf5767e674abfee2bd53cd6b9f5c3831482

promag

Concept ACK.

The PR suggests that the following dependencies wouldn't exist:

Circular dependency: init -> net_processing -> init
Circular dependency: init -> validationinterface -> init

~~Also, I've removed the first 3 commits locally and it still compiles and reports the same dependencies as above. I wonder why are those commits included here.~~

Edit: oh this is rebased on #13239.

promag · 2018-05-16T11:47:03Z

src/shutdown.cpp

+
+#include <atomic>
+
+std::atomic<bool> fRequestShutdown(false);


static std::atomic<bool> fRequestShutdown{false};

promag · 2018-05-16T11:47:19Z

src/shutdown.h

@@ -0,0 +1,13 @@
+// Copyright (c) 2009-2010 Satoshi Nakamoto
+// Copyright (c) 2009-2017 The Bitcoin Core developers


promag · 2018-05-16T11:47:35Z

src/txdb.cpp

@@ -12,7 +12,7 @@
 #include <uint256.h>
 #include <util.h>
 #include <ui_interface.h>
-#include <init.h>
+#include <shutdown.h>


Keep sorted.

Note that you can install clang-format and run the https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy script which fixes all white space and sorting issues

promag · 2018-05-16T11:57:08Z

src/shutdown.cpp

+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <atomic>


nit, also #include <shutdown.h>?

Empact · 2018-05-16T19:28:24Z

Declared fRequestShutdown static
Included shutdown.h in shutdown.cpp
Updated copyright years
Ran clang-format to sort includes

Thanks @promag / @MarcoFalke

promag · 2018-05-16T19:33:27Z

Still has the following circular dependencies:

Circular dependency: init -> net_processing -> init
Circular dependency: init -> validationinterface -> init

Should these be removed in this PR?

Empact · 2018-05-16T19:52:00Z

@promag Added a commit to remove unused includes of init from 4 files. I'd looked into those before but left them out to narrow this PR.

promag · 2018-05-16T19:52:35Z

@Empact needs rebase.

Empact · 2018-05-16T20:12:04Z

Rebased

Empact · 2018-05-18T18:40:22Z

~~@jonasschnelli @practicalswift would you take a look at #13239? It's the necessary precondition for this.~~

Most includers just wanted to react to pending shutdown. This isolates access to `fRequestShutdown` and limits access to the shutdown api functions, including the new `AbortShutdown` for setting it to `false`. Note I originally called `AbortShutdown` `CancelShutdown` but that name was already taken by winuser.h https://travis-ci.org/bitcoin/bitcoin/jobs/386913329 This change also triggered a build error in bench. Fixing it required moving LIBBITCOIN_SERVER after LIBBITCOIN_WALLET in bench_bench_bitcoin_LDADD To make server definitions in src/net.cpp available to wallet methods in src/wallet/wallet.cpp. Specifically, solving: libbitcoin_wallet.a(libbitcoin_wallet_a-wallet.o): In function `CWalletTx::RelayWalletTransaction(CConnman*)': wallet.cpp:(.text+0x3f0e): undefined reference to `CConnman::NodeFullyConnected(CNode const*)' collect2: error: ld returned 1 exit status https://travis-ci.org/bitcoin/bitcoin/jobs/392133581 Need for remaining init.h includes confirmed via a thorough search with a more specific regex: \bInterrupt\(\)|\bShutdown\(\)|\bInitLogging\(\)|\bInitParameterInteraction\(\)|\bAppInitBasicSetup\(\)|\bAppInitParameterInteraction\(\)|\bAppInitSanityChecks\(\)|\bAppInitLockDataDirectory\(\)|\bAppInitMain\(\)|\bSetupServerArgs\(\)|\bLicenseInfo\(\)|g_wallet_init_interface|init.h

Empact · 2018-06-25T04:23:29Z

Rebased for #13458

ken2812221 · 2018-06-25T09:12:14Z

utACK 1fabd59

sipa · 2018-06-27T00:22:40Z

@Empact In your first commit you write "These were entirely unused, as based on successful compilation". That's not a sufficient argument - init.h could still be included indirectly through another header, while exported symbols from init.h are still used.

This doesn't mean it wrong to remove them, just that "it compiles without them" isn't a sufficient reason. Tools like iwyu can help with this.

Empact · 2018-06-27T00:32:32Z

@sipa thanks I’ll check that out

Empact · 2018-06-27T01:17:06Z

Oh btw I did manually check using this grep:

  \bStartShutdown\(\)|\bShutdownRequested\(\)|\bInterrupt\(\)|\bShutdown\(\)|\bInitLogging\(\)|\bInitParameterInteraction\(\)|\bAppInitBasicSetup\(\)|\bAppInitParameterInteraction\(\)|\bAppInitSanityChecks\(\)|\bAppInitLockDataDirectory\(\)|\bAppInitMain\(\)|\bSetupServerArgs\(\)|\bLicenseInfo\(\)|g_wallet_init_interface|init.h

But I'll check again with the above and using iwyu, which is probably more robust.

sipa · 2018-06-27T01:26:52Z

@Empact Oh, ignore my comment in that case. I just wanted to make sure you're not relying on indirect inclusions.

sipa · 2018-06-27T01:28:00Z

utACK 1fabd59

Empact · 2018-07-01T19:19:08Z

@practicalswift @jonasschnelli @promag could I get another look?

Alternatively I could split this into 2 PRS, the removal of the unnecessary includes and the shutdown.h extraction, if that would make review easier.

laanwj · 2018-07-04T13:28:09Z

utACK 1fabd59

…ing shutdown.h 1fabd59 Break circular dependency: init -> * -> init by extracting shutdown.h (Ben Woosley) e62fdfe Drop unused init.h includes (Ben Woosley) Pull request description: Most includers just wanted to react to pending shutdown. This isolates access to `fRequestShutdown` and limits access to the shutdown api functions, including the new `CancelShutdown` for setting it to `false`. Tree-SHA512: df42f75dfbba163576710e9a67cf1228531fd99d70a2f187bfba0bcc476d6749cf88180a97e66a81bb5b6c3c7f0917de7402d26039ba7b644cb7509b02f7e267

…extracting shutdown.h 1fabd59 Break circular dependency: init -> * -> init by extracting shutdown.h (Ben Woosley) e62fdfe Drop unused init.h includes (Ben Woosley) Pull request description: Most includers just wanted to react to pending shutdown. This isolates access to `fRequestShutdown` and limits access to the shutdown api functions, including the new `CancelShutdown` for setting it to `false`. Tree-SHA512: df42f75dfbba163576710e9a67cf1228531fd99d70a2f187bfba0bcc476d6749cf88180a97e66a81bb5b6c3c7f0917de7402d26039ba7b644cb7509b02f7e267

Empact force-pushed the init-circ-init branch from 5781037 to 6e38d03 Compare May 15, 2018 01:41

fanquake added the Refactoring label May 15, 2018

Empact force-pushed the init-circ-init branch from 6e38d03 to adfa3cd Compare May 15, 2018 06:42

Empact force-pushed the init-circ-init branch 3 times, most recently from 58bc7bf to 624d44a Compare May 15, 2018 20:01

Empact force-pushed the init-circ-init branch 2 times, most recently from a5710f1 to f7033bf Compare May 15, 2018 20:49

promag reviewed May 16, 2018

View reviewed changes

Empact force-pushed the init-circ-init branch 2 times, most recently from 4f4cae6 to e52d0bf Compare May 16, 2018 19:26

Empact force-pushed the init-circ-init branch from f90a522 to 4610b96 Compare May 16, 2018 20:11

Empact mentioned this pull request May 18, 2018

[moveonly] Fix CConnman template methods to be fully-defined in net.h #13239

Closed

Empact force-pushed the init-circ-init branch 2 times, most recently from 5ab1f4c to 2e73ca5 Compare May 24, 2018 00:07

Empact force-pushed the init-circ-init branch 2 times, most recently from a4e0fdf to 9b583bd Compare June 1, 2018 23:19

Empact force-pushed the init-circ-init branch from ab30690 to 1fabd59 Compare June 25, 2018 04:09

DrahtBot removed the Needs rebase label Jun 25, 2018

laanwj merged commit 1fabd59 into bitcoin:master Jul 4, 2018

Bushstar mentioned this pull request Jul 5, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#352

Merged

Empact mentioned this pull request Jul 5, 2018

Extract AppInitLoadBlockIndex from AppInitMain #13582

Closed

l2a5b1 mentioned this pull request Jul 6, 2018

Allow quicker shutdowns during LoadBlockIndex() #12980

Merged

This was referenced Aug 3, 2018

Refactor: separate wallet from node #10973

Merged

Add BitcoinApplication & RPCConsole tests #11625

Merged

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

		@@ -0,0 +1,13 @@
		// Copyright (c) 2009-2010 Satoshi Nakamoto
		// Copyright (c) 2009-2017 The Bitcoin Core developers

Break circular dependency: init -> * -> init by extracting shutdown.h #13235

Break circular dependency: init -> * -> init by extracting shutdown.h #13235

Uh oh!

Conversation

Empact commented May 15, 2018

Uh oh!

Empact commented May 15, 2018

Uh oh!

fanquake commented May 15, 2018

Uh oh!

practicalswift commented May 15, 2018

Uh oh!

laanwj commented May 15, 2018

Uh oh!

Empact commented May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented May 16, 2018

Uh oh!

practicalswift commented May 16, 2018

Uh oh!

promag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag May 16, 2018

Choose a reason for hiding this comment

Uh oh!

promag May 16, 2018

Choose a reason for hiding this comment

Uh oh!

promag May 16, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko May 16, 2018

Choose a reason for hiding this comment

Uh oh!

promag May 16, 2018

Choose a reason for hiding this comment

Uh oh!

Empact commented May 16, 2018

Uh oh!

promag commented May 16, 2018

Uh oh!

Empact commented May 16, 2018

Uh oh!

promag commented May 16, 2018

Uh oh!

Empact commented May 16, 2018

Uh oh!

Empact commented May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Jun 25, 2018

Uh oh!

ken2812221 commented Jun 25, 2018

Uh oh!

sipa commented Jun 27, 2018

Uh oh!

Empact commented Jun 27, 2018

Uh oh!

Empact commented Jun 27, 2018

Uh oh!

sipa commented Jun 27, 2018

Uh oh!

sipa commented Jun 27, 2018

Uh oh!

Empact commented Jul 1, 2018

Uh oh!

laanwj commented Jul 4, 2018

Uh oh!

Uh oh!

Empact commented May 15, 2018 •

edited

Loading

promag left a comment •

edited

Loading

Empact commented May 18, 2018 •

edited

Loading