Skip to content

Commit

Permalink
kernel: Drop global Logger instance
Browse files Browse the repository at this point in the history
Change LogInstance() function to no longer allocate (and leak) a BCLog::Logger
instance. Instead allow kernel applications to initialize their own logging
instances that can be returned by LogInstance().

The LogInstance() function is not actually used for the majority of logging in
kernel code. Most kernel log output goes directly to BCLog::Logger instances
specified when kernel objects like ChainstateManager and CTxMemPool are
constructed, which allows applications to create multiple Logger instances and
control which log output is sent to them.

The only kernel code that does rely on LogInstance() is certain low level code in
the util library, like the RNG seeder, that is not passed a specific instance
and needs to rely on the global LogInstance() function.

Other code outside the kernel library uses LogInstance() heavily, and may
continue to do so. bitcoind and test code now just create a log instance before
the first LogInstance() call, which it returns, so all behavior is the same as
before.
  • Loading branch information
ryanofsky committed Dec 5, 2024
1 parent ca87fdd commit 493dd26
Showing 20 changed files with 61 additions and 19 deletions.
2 changes: 2 additions & 0 deletions src/bench/bench_bitcoin.cpp
Original file line number Diff line number Diff line change
@@ -5,6 +5,7 @@
#include <bench/bench.h>
#include <common/args.h>
#include <crypto/sha256.h>
#include <logging.h>
#include <tinyformat.h>
#include <util/fs.h>
#include <util/string.h>
@@ -76,6 +77,7 @@ static std::vector<std::string> parseTestSetupArgs(const ArgsManager& argsman)

int main(int argc, char** argv)
{
BCLog::Logger logger;
ArgsManager argsman;
SetupBenchArgs(argsman);
SHA256AutoDetect();
2 changes: 2 additions & 0 deletions src/bitcoin-cli.cpp
Original file line number Diff line number Diff line change
@@ -11,6 +11,7 @@
#include <common/system.h>
#include <compat/compat.h>
#include <compat/stdin.h>
#include <logging.h>
#include <policy/feerate.h>
#include <rpc/client.h>
#include <rpc/mining.h>
@@ -1331,6 +1332,7 @@ MAIN_FUNCTION
common::WinCmdLineArgs winArgs;
std::tie(argc, argv) = winArgs.get();
#endif
BCLog::Logger logger;
SetupEnvironment();
if (!SetupNetworking()) {
tfm::format(std::cerr, "Error: Initializing networking failed\n");
2 changes: 2 additions & 0 deletions src/bitcoin-tx.cpp
Original file line number Diff line number Diff line change
@@ -14,6 +14,7 @@
#include <consensus/consensus.h>
#include <core_io.h>
#include <key_io.h>
#include <logging.h>
#include <policy/policy.h>
#include <primitives/transaction.h>
#include <script/script.h>
@@ -864,6 +865,7 @@ static int CommandLineRawTx(int argc, char* argv[])

MAIN_FUNCTION
{
BCLog::Logger logger;
SetupEnvironment();

try {
2 changes: 2 additions & 0 deletions src/bitcoin-util.cpp
Original file line number Diff line number Diff line change
@@ -13,6 +13,7 @@
#include <common/system.h>
#include <compat/compat.h>
#include <core_io.h>
#include <logging.h>
#include <streams.h>
#include <util/exception.h>
#include <util/strencodings.h>
@@ -152,6 +153,7 @@ static int Grind(const std::vector<std::string>& args, std::string& strPrint)
MAIN_FUNCTION
{
ArgsManager& args = gArgs;
BCLog::Logger logger;
SetupEnvironment();

try {
2 changes: 2 additions & 0 deletions src/bitcoin-wallet.cpp
Original file line number Diff line number Diff line change
@@ -104,6 +104,8 @@ MAIN_FUNCTION
std::tie(argc, argv) = winArgs.get();
#endif

BCLog::Logger logger;

int exit_status;
std::unique_ptr<interfaces::Init> init = interfaces::MakeWalletInit(argc, argv, exit_status);
if (!init) {
4 changes: 4 additions & 0 deletions src/bitcoind.cpp
Original file line number Diff line number Diff line change
@@ -14,6 +14,7 @@
#include <init.h>
#include <interfaces/chain.h>
#include <interfaces/init.h>
#include <logging.h>
#include <kernel/context.h>
#include <node/context.h>
#include <node/interface_ui.h>
@@ -259,6 +260,9 @@ MAIN_FUNCTION
std::tie(argc, argv) = winArgs.get();
#endif

// Intentionally leaked! See BCLog::g_logger description for rationale.
new BCLog::Logger;

NodeContext node;
int exit_status;
std::unique_ptr<interfaces::Init> init = interfaces::MakeNodeInit(node, argc, argv, exit_status);
27 changes: 20 additions & 7 deletions src/logging.cpp
Original file line number Diff line number Diff line change
@@ -21,8 +21,9 @@ using util::RemovePrefixView;
const char * const DEFAULT_DEBUGLOGFILE = "debug.log";
constexpr auto MAX_USER_SETABLE_SEVERITY_LEVEL{BCLog::Level::Info};

BCLog::Logger& LogInstance()
{
bool fLogIPs = DEFAULT_LOGIPS;

namespace BCLog {
/**
* NOTE: the logger instances is leaked on exit. This is ugly, but will be
* cleaned up by the OS/libc. Defining a logger as a global object doesn't work
@@ -38,17 +39,29 @@ BCLog::Logger& LogInstance()
* This method of initialization was originally introduced in
* ee3374234c60aba2cc4c5cd5cac1c0aefc2d817c.
*/
static BCLog::Logger* g_logger{new BCLog::Logger()};
return *g_logger;
}

bool fLogIPs = DEFAULT_LOGIPS;
Logger* g_logger{nullptr};
} // namespace BCLog

static int FileWriteStr(std::string_view str, FILE *fp)
{
return fwrite(str.data(), 1, str.size(), fp);
}

BCLog::Logger& LogInstance()
{
return *Assert(BCLog::g_logger);
}

BCLog::Logger::Logger()
{
if (!g_logger) g_logger = this;
}

BCLog::Logger::~Logger()
{
if (g_logger == this) g_logger = nullptr;
}

bool BCLog::Logger::StartLogging()
{
StdLockGuard scoped_lock(m_cs);
3 changes: 3 additions & 0 deletions src/logging.h
Original file line number Diff line number Diff line change
@@ -153,6 +153,9 @@ namespace BCLog {
fs::path m_file_path;
std::atomic<bool> m_reopen_file{false};

Logger();
~Logger();

/** Send a string to the log output */
void LogPrintStr(std::string_view str, std::string_view logging_function, std::string_view source_file, int source_line, BCLog::LogFlags category, BCLog::Level level)
EXCLUSIVE_LOCKS_REQUIRED(!m_cs);
3 changes: 3 additions & 0 deletions src/qt/bitcoin.cpp
Original file line number Diff line number Diff line change
@@ -492,6 +492,9 @@ int GuiMain(int argc, char* argv[])
std::tie(argc, argv) = winArgs.get();
#endif

// Intentionally leaked! See BCLog::g_logger description for rationale.
new BCLog::Logger;

std::unique_ptr<interfaces::Init> init = interfaces::MakeGuiInit(argc, argv);

SetupEnvironment();
2 changes: 2 additions & 0 deletions src/qt/test/test_main.cpp
Original file line number Diff line number Diff line change
@@ -6,6 +6,7 @@

#include <interfaces/init.h>
#include <interfaces/node.h>
#include <logging.h>
#include <qt/bitcoin.h>
#include <qt/guiconstants.h>
#include <qt/test/apptests.h>
@@ -37,6 +38,7 @@ const std::function<std::string()> G_TEST_GET_FULL_NAME{};
// This is all you need to run all the tests
int main(int argc, char* argv[])
{
BCLog::Logger logger;
// Initialize persistent globals with the testing setup state for sanity.
// E.g. -datadir in gArgs is set to a temp directory dummy value (instead
// of defaulting to the default datadir), or globalChainParams is set to
2 changes: 1 addition & 1 deletion src/test/blockfilter_tests.cpp
Original file line number Diff line number Diff line change
@@ -16,7 +16,7 @@

#include <boost/test/unit_test.hpp>

BOOST_AUTO_TEST_SUITE(blockfilter_tests)
BOOST_FIXTURE_TEST_SUITE(blockfilter_tests, BasicTestingSetup)

BOOST_AUTO_TEST_CASE(gcsfilter_test)
{
4 changes: 2 additions & 2 deletions src/test/fuzz/block_index.cpp
Original file line number Diff line number Diff line change
@@ -14,7 +14,7 @@

namespace {

const BasicTestingSetup* g_setup;
BasicTestingSetup* g_setup;

// Hardcoded block hash and nBits to make sure the blocks we store pass the pow check.
uint256 g_block_hash;
@@ -46,7 +46,7 @@ CBlockHeader ConsumeBlockHeader(FuzzedDataProvider& provider)

void init_block_index()
{
static const auto testing_setup = MakeNoLogFileContext<>(ChainType::MAIN);
static const auto testing_setup = MakeNoLogFileContext<BasicTestingSetup>(ChainType::MAIN);
g_setup = testing_setup.get();
g_block_hash = Params().GenesisBlock().GetHash();
}
4 changes: 4 additions & 0 deletions src/test/fuzz/fuzz.cpp
Original file line number Diff line number Diff line change
@@ -4,6 +4,7 @@

#include <test/fuzz/fuzz.h>

#include <logging.h>
#include <netaddress.h>
#include <netbase.h>
#include <test/util/random.h>
@@ -103,6 +104,9 @@ void ResetCoverageCounters() {}

void initialize()
{
// Initialize logger first because RNG startup uses it.
static BCLog::Logger g_logger;

// By default, make the RNG deterministic with a fixed seed. This will affect all
// randomness during the fuzz test, except:
// - GetStrongRandBytes(), which is used for the creation of private key material.
4 changes: 2 additions & 2 deletions src/test/fuzz/mini_miner.cpp
Original file line number Diff line number Diff line change
@@ -20,11 +20,11 @@

namespace {

const TestingSetup* g_setup;
TestingSetup* g_setup;
std::deque<COutPoint> g_available_coins;
void initialize_miner()
{
static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>();
static const auto testing_setup = MakeNoLogFileContext<TestingSetup>();
g_setup = testing_setup.get();
for (uint32_t i = 0; i < uint32_t{100}; ++i) {
g_available_coins.emplace_back(Txid::FromUint256(uint256::ZERO), i);
4 changes: 2 additions & 2 deletions src/test/fuzz/script_sigcache.cpp
Original file line number Diff line number Diff line change
@@ -19,12 +19,12 @@
#include <vector>

namespace {
const BasicTestingSetup* g_setup;
BasicTestingSetup* g_setup;
} // namespace

void initialize_script_sigcache()
{
static const auto testing_setup = MakeNoLogFileContext<>();
static const auto testing_setup = MakeNoLogFileContext<BasicTestingSetup>();
g_setup = testing_setup.get();
}

2 changes: 1 addition & 1 deletion src/test/reverselock_tests.cpp
Original file line number Diff line number Diff line change
@@ -9,7 +9,7 @@

#include <stdexcept>

BOOST_AUTO_TEST_SUITE(reverselock_tests)
BOOST_FIXTURE_TEST_SUITE(reverselock_tests, BasicTestingSetup)

BOOST_AUTO_TEST_CASE(reverselock_basics)
{
3 changes: 2 additions & 1 deletion src/test/scheduler_tests.cpp
Original file line number Diff line number Diff line change
@@ -4,6 +4,7 @@

#include <random.h>
#include <scheduler.h>
#include <test/util/setup_common.h>
#include <util/time.h>

#include <boost/test/unit_test.hpp>
@@ -13,7 +14,7 @@
#include <thread>
#include <vector>

BOOST_AUTO_TEST_SUITE(scheduler_tests)
BOOST_FIXTURE_TEST_SUITE(scheduler_tests, BasicTestingSetup)

static void microTask(CScheduler& s, std::mutex& mutex, int& counter, int delta, std::chrono::steady_clock::time_point rescheduleTime)
{
2 changes: 1 addition & 1 deletion src/test/sync_tests.cpp
Original file line number Diff line number Diff line change
@@ -77,7 +77,7 @@ void TestInconsistentLockOrderDetected(MutexType& mutex1, MutexType& mutex2) NO_
}
} // namespace

BOOST_AUTO_TEST_SUITE(sync_tests)
BOOST_FIXTURE_TEST_SUITE(sync_tests, BasicTestingSetup)

BOOST_AUTO_TEST_CASE(potential_deadlock_detected)
{
4 changes: 3 additions & 1 deletion src/test/util/setup_common.cpp
Original file line number Diff line number Diff line change
@@ -78,6 +78,8 @@ constexpr inline auto TEST_DIR_PATH_ELEMENT{"test_common bitcoin"}; // Includes
/** Random context to get unique temp data dirs. Separate from m_rng, which can be seeded from a const env var */
static FastRandomContext g_rng_temp_path;
static const bool g_rng_temp_path_init{[] {
// Make sure there is a global logger SeedStartup() can use.
BCLog::Logger logger;
// Must be initialized before any SeedRandomForTest
(void)g_rng_temp_path.rand64();
return true;
@@ -106,7 +108,7 @@ static void ExitFailure(std::string_view str_err)
}

BasicTestingSetup::BasicTestingSetup(const ChainType chainType, TestOpts opts)
: m_logger{LogInstance()}, m_args{}
: m_args{}
{
m_node.shutdown_signal = &m_interrupt;
m_node.shutdown_request = [this]{ return m_interrupt(); };
2 changes: 1 addition & 1 deletion src/test/util/setup_common.h
Original file line number Diff line number Diff line change
@@ -62,7 +62,7 @@ struct TestOpts {
* This just configures logging, data dir and chain parameters.
*/
struct BasicTestingSetup {
BCLog::Logger& m_logger;
BCLog::Logger m_logger;
util::SignalInterrupt m_interrupt;
node::NodeContext m_node; // keep as first member to be destructed last

0 comments on commit 493dd26

Please sign in to comment.