Fixes & comments

nano/lib/config.cpp

@@ -377,6 +377,7 @@ std::string_view nano::to_string (nano::networks network)
 	return "n/a";
 }
 
+// Using std::cerr here, since logging may not be initialized yet
 nano::tomlconfig nano::load_toml_file (const std::filesystem::path & config_filename, const std::filesystem::path & data_path, const std::vector<std::string> & config_overrides)
 {
 	std::stringstream config_overrides_stream;
@@ -396,7 +397,7 @@ nano::tomlconfig nano::load_toml_file (const std::filesystem::path & config_file
 		{
 			throw std::runtime_error (error.get_message ());
 		}
-		nano::default_logger ().info (nano::log::type::config, "Config for `{}` loaded from node data directory: ", config_filename.string (), toml_config_path.string ());
+		std::cerr << "Config file `" << config_filename.string () << "` loaded from node data directory: " << toml_config_path.string () << std::endl;
 		return toml;
 	}
 	else
@@ -408,7 +409,7 @@ nano::tomlconfig nano::load_toml_file (const std::filesystem::path & config_file
 		{
 			throw std::runtime_error (error.get_message ());
 		}
-		nano::default_logger ().info (nano::log::type::config, "Config for `{}` not found, using default configuration", config_filename.string ());
+		std::cerr << "Config file `" << config_filename.string () << "` not found, using default configuration" << std::endl;
 		return toml;
 	}
 }

nano/lib/config.hpp

@@ -410,11 +410,15 @@ bool is_sanitizer_build ();
 void force_nano_dev_network ();
 
 /**
- * Attempt to read a configuration file from current working directory, or if not found, the nano root directory.
- * Returns empty tomlconfig if nothing is found.
+ * Attempt to read a configuration file from specified directory. Returns empty tomlconfig if nothing is found.
+ * @throws std::runtime_error with error code if the file or overrides are not valid toml
  */
 nano::tomlconfig load_toml_file (const std::filesystem::path & config_filename, const std::filesystem::path & data_path, const std::vector<std::string> & config_overrides);
 
+/**
+ * Attempt to read a configuration file from specified directory. Returns fallback config if nothing is found.
+ * @throws std::runtime_error with error code if the file or overrides are not valid toml or deserialization fails
+ */
 template <typename T>
 T load_config_file (T fallback, const std::filesystem::path & config_filename, const std::filesystem::path & data_path, const std::vector<std::string> & config_overrides)
 {

nano/lib/logging.cpp

@@ -3,18 +3,12 @@
 #include <nano/lib/utility.hpp>
 
 #include <fmt/chrono.h>
-#include <spdlog/cfg/env.h>
 #include <spdlog/pattern_formatter.h>
 #include <spdlog/sinks/basic_file_sink.h>
 #include <spdlog/sinks/rotating_file_sink.h>
 #include <spdlog/sinks/stdout_color_sinks.h>
 #include <spdlog/sinks/stdout_sinks.h>
 
-namespace
-{
-std::atomic<bool> logging_initialized{ false };
-}
-
 nano::logger & nano::default_logger ()
 {
 	static nano::logger logger{ "default" };
@@ -34,9 +28,10 @@ std::function<std::string (nano::log::type tag, std::string identifier)> nano::l
 	return std::string{ to_string (tag) };
 } };
 
-void nano::logger::initialize (nano::log_config fallback, std::filesystem::path data_path, std::vector<std::string> const & config_overrides)
+void nano::logger::initialize (nano::log_config fallback, std::optional<std::filesystem::path> data_path, std::vector<std::string> const & config_overrides)
 {
-	auto config = nano::load_log_config (std::move (fallback), data_path, config_overrides);
+	// Only load log config from file if data_path is available (i.e. not running in cli mode)
+	nano::log_config config = data_path ? nano::load_log_config (fallback, *data_path, config_overrides) : fallback;
 	initialize_common (config, data_path);
 	global_initialized = true;
 }
@@ -97,8 +92,8 @@ class tag_formatter_flag : public spdlog::custom_flag_formatter
 
 void nano::logger::initialize_for_tests (nano::log_config fallback)
 {
-	auto config = nano::load_log_config (std::move (fallback), /* load log config from current workdir */ {});
-	initialize_common (config, /* store log file in current workdir */ {});
+	auto config = nano::load_log_config (std::move (fallback), /* load log config from current workdir */ std::filesystem::current_path ());
+	initialize_common (config, /* store log file in current workdir */ std::filesystem::current_path ());
 
 	// Use tag and identifier as the logger name, since multiple nodes may be running in the same process
 	global_name_formatter = [] (nano::log::type tag, std::string identifier) {
@@ -119,13 +114,13 @@ void nano::logger::initialize_for_tests (nano::log_config fallback)
 	global_initialized = true;
 }
 
-void nano::logger::initialize_common (nano::log_config const & config, std::filesystem::path data_path)
+// Using std::cerr here, since logging may not be initialized yet
+void nano::logger::initialize_common (nano::log_config const & config, std::optional<std::filesystem::path> data_path)
 {
 	global_config = config;
 
 	spdlog::set_automatic_registration (false);
 	spdlog::set_level (to_spdlog_level (config.default_level));
-	spdlog::cfg::load_env_levels ();
 
 	global_sinks.clear ();
 
@@ -156,21 +151,24 @@ void nano::logger::initialize_common (nano::log_config const & config, std::file
 	// File setup
 	if (config.file.enable)
 	{
+		// In cases where data_path is not available, file logging should always be disabled
+		release_assert (data_path);
+
 		auto now = std::chrono::system_clock::now ();
 		auto time = std::chrono::system_clock::to_time_t (now);
 
 		auto filename = fmt::format ("log_{:%Y-%m-%d_%H-%M}-{:%S}", fmt::localtime (time), now.time_since_epoch ());
 		std::replace (filename.begin (), filename.end (), '.', '_'); // Replace millisecond dot separator with underscore
 
-		std::filesystem::path log_path{ data_path / "log" / (filename + ".log") };
+		std::filesystem::path log_path{ data_path.value () / "log" / (filename + ".log") };
 		log_path = std::filesystem::absolute (log_path);
 
 		std::cerr << "Logging to file: " << log_path.string () << std::endl;
 
 		// If either max_size or rotation_count is 0, then disable file rotation
 		if (config.file.max_size == 0 || config.file.rotation_count == 0)
 		{
-			// TODO: Maybe show a warning to the user about possibly unlimited log file size
+			std::cerr << "WARNING: Log file rotation is disabled, log file size may grow without bound" << std::endl;
 
 			auto file_sink = std::make_shared<spdlog::sinks::basic_file_sink_mt> (log_path.string (), true);
 			global_sinks.push_back (file_sink);
@@ -198,13 +196,19 @@ void nano::logger::flush ()
 nano::logger::logger (std::string identifier) :
 	identifier{ std::move (identifier) }
 {
+	release_assert (global_initialized, "logging should be initialized before creating a logger");
+}
+
+nano::logger::~logger ()
+{
+	flush ();
 }
 
 spdlog::logger & nano::logger::get_logger (nano::log::type tag)
 {
 	// This is a two-step process to avoid exclusively locking the mutex in the common case
 	{
-		std::shared_lock slock{ mutex };
+		std::shared_lock lock{ mutex };
 
 		if (auto it = spd_loggers.find (tag); it != spd_loggers.end ())
 		{
@@ -215,8 +219,8 @@ spdlog::logger & nano::logger::get_logger (nano::log::type tag)
 	{
 		std::unique_lock lock{ mutex };
 
-		auto [it2, inserted] = spd_loggers.emplace (tag, make_logger (tag));
-		return *it2->second;
+		auto [it, inserted] = spd_loggers.emplace (tag, make_logger (tag));
+		return *it->second;
 	}
 }
 
@@ -242,7 +246,7 @@ std::shared_ptr<spdlog::logger> nano::logger::make_logger (nano::log::type tag)
 	return spd_logger;
 }
 
-spdlog::level::level_enum nano::to_spdlog_level (nano::log::level level)
+spdlog::level::level_enum nano::logger::to_spdlog_level (nano::log::level level)
 {
 	switch (level)
 	{
@@ -437,6 +441,7 @@ std::map<nano::log_config::logger_id_t, nano::log::level> nano::log_config::defa
  * config loading
  */
 
+// Using std::cerr here, since logging may not be initialized yet
 nano::log_config nano::load_log_config (nano::log_config fallback, const std::filesystem::path & data_path, const std::vector<std::string> & config_overrides)
 {
 	const std::string config_filename = "config-log.toml";

nano/lib/logging.hpp

@@ -59,24 +59,19 @@ class log_config final
 	static std::map<logger_id_t, nano::log::level> default_levels (nano::log::level);
 };
 
-/// @throws std::runtime_error if the log config file is malformed
-nano::log_config load_log_config (nano::log_config fallback, std::filesystem::path const & data_path = {}, std::vector<std::string> const & config_overrides = std::vector<std::string> ());
-}
-
-namespace nano
-{
-spdlog::level::level_enum to_spdlog_level (nano::log::level);
+nano::log_config load_log_config (nano::log_config fallback, std::filesystem::path const & data_path, std::vector<std::string> const & config_overrides = {});
 
 class logger final
 {
 public:
-	logger (std::string identifier = "");
+	explicit logger (std::string identifier = "");
+	~logger ();
 
 	// Disallow copies
 	logger (logger const &) = delete;
 
 public:
-	static void initialize (nano::log_config fallback, std::filesystem::path data_path = {}, std::vector<std::string> const & config_overrides = std::vector<std::string> ());
+	static void initialize (nano::log_config fallback, std::optional<std::filesystem::path> data_path = std::nullopt, std::vector<std::string> const & config_overrides = std::vector<std::string> ());
 	static void initialize_for_tests (nano::log_config fallback);
 	static void flush ();
 
@@ -86,7 +81,7 @@ class logger final
 	static std::vector<spdlog::sink_ptr> global_sinks;
 	static std::function<std::string (nano::log::type tag, std::string identifier)> global_name_formatter;
 
-	static void initialize_common (nano::log_config const &, std::filesystem::path data_path);
+	static void initialize_common (nano::log_config const &, std::optional<std::filesystem::path> data_path);
 
 public:
 	template <class... Args>
@@ -134,7 +129,13 @@ class logger final
 private:
 	spdlog::logger & get_logger (nano::log::type tag);
 	std::shared_ptr<spdlog::logger> make_logger (nano::log::type tag);
+
+	static spdlog::level::level_enum to_spdlog_level (nano::log::level);
 };
 
+/**
+ * Returns a logger instance that can be used before node specific logging is available.
+ * Should only be used for logging that happens during startup and initialization, since it won't contain node specific identifier.
+ */
 nano::logger & default_logger ();
 }

nano/test_common/system.hpp

@@ -67,7 +67,7 @@ namespace test
 		boost::asio::io_context io_ctx;
 		std::vector<std::shared_ptr<nano::node>> nodes;
 		nano::stats stats;
-		nano::logger logger;
+		nano::logger logger{ "tests" };
 		nano::work_pool work{ nano::dev::network_params.network, std::max (nano::hardware_concurrency (), 1u) };
 		std::chrono::time_point<std::chrono::steady_clock, std::chrono::duration<double>> deadline{ std::chrono::steady_clock::time_point::max () };
 		double deadline_scaling_factor{ 1.0 };