From 62aaf0382f4b3204969a56e2557439f9f69d12e7 Mon Sep 17 00:00:00 2001 From: Kam Cheung Ting Date: Thu, 30 Jul 2026 21:11:44 +0000 Subject: [PATCH] fix(logging): run FatalHandler on all kFatal paths (#726 review) The FatalHandler only ran for fixed ICEBERG_LOG_FATAL; reaching kFatal via the runtime-level ICEBERG_LOG(kFatal, ...) or ICEBERG_LOG_TO(sink, kFatal, ...) aborted without invoking it (and only formatted when ShouldLog passed). Extract the fatal sequence into a shared DispatchFatal (format once -> emit-if-enabled -> flush -> run handler -> abort) and route LogFatal, LogToCurrentRuntime, and LogToExplicitRuntime through it. Adds death tests for the two runtime paths. Co-authored-by: Isaac --- src/iceberg/logging/log_macros.h | 84 +++++++++++++++++--------------- src/iceberg/test/macros_test.cc | 26 ++++++++++ 2 files changed, 71 insertions(+), 39 deletions(-) diff --git a/src/iceberg/logging/log_macros.h b/src/iceberg/logging/log_macros.h index cbf33dc36..9b31bc7de 100644 --- a/src/iceberg/logging/log_macros.h +++ b/src/iceberg/logging/log_macros.h @@ -79,53 +79,24 @@ void LogToCurrent(LogLevel level, const std::source_location& location, } } -/// \brief Runtime-level variant against the current logger: emit if enabled, then -/// flush + abort when level == kFatal (using the same acquired logger). -template -void LogToCurrentRuntime(LogLevel level, const std::source_location& location, - MakeMessage&& make_message) noexcept { - const std::shared_ptr& logger = CurrentLogger(); - if (logger) { - EmitIfEnabled(*logger, level, location, std::forward(make_message)); - } - if (level == LogLevel::kFatal) { - if (logger) logger->Flush(); - std::abort(); - } -} - -/// \brief Runtime-level variant against an explicit logger: emit if enabled, then -/// flush + abort when level == kFatal. -template -void LogToExplicitRuntime(Logger& logger, LogLevel level, - const std::source_location& location, - MakeMessage&& make_message) noexcept { - EmitIfEnabled(logger, level, location, std::forward(make_message)); - if (level == LogLevel::kFatal) { - logger.Flush(); - std::abort(); - } -} - -/// \brief Fatal path: acquire the effective (scoped-or-default) logger ONCE, emit -/// if enabled, flush that same logger, run any registered FatalHandler, then -/// abort. Never returns. +/// \brief The one fatal sequence, shared by every kFatal path: format the message +/// once, emit it if \p logger is enabled, flush \p logger, run any registered +/// FatalHandler, then std::abort(). Never returns. /// -/// The message is always formatted here (independent of ShouldLog) so the handler -/// receives it even when the fatal record itself is filtered out. The handler runs -/// after emit+flush and before abort; if it does not itself terminate the process, -/// std::abort() still runs. +/// \p logger may be null (no sink). The message is formatted regardless of +/// ShouldLog so the handler always receives it, even when the record is filtered +/// out. The handler runs after emit+flush and before abort; if it does not itself +/// terminate the process, std::abort() still runs. template -[[noreturn]] void LogFatal(const std::source_location& location, - MakeMessage&& make_message) noexcept { +[[noreturn]] void DispatchFatal(Logger* logger, const std::source_location& location, + MakeMessage&& make_message) noexcept { std::string message; try { message = std::forward(make_message)(); } catch (...) { message = ""; } - auto logger = GetCurrentLogger(); - if (logger) { + if (logger != nullptr) { if (logger->ShouldLog(LogLevel::kFatal)) { Emit(*logger, LogLevel::kFatal, location, std::string(message)); } @@ -140,6 +111,41 @@ template std::abort(); } +/// \brief Runtime-level variant against the current logger: on kFatal run the +/// shared fatal sequence; otherwise emit if enabled. +template +void LogToCurrentRuntime(LogLevel level, const std::source_location& location, + MakeMessage&& make_message) noexcept { + const std::shared_ptr& logger = CurrentLogger(); + if (level == LogLevel::kFatal) { + DispatchFatal(logger.get(), location, std::forward(make_message)); + } + if (logger) { + EmitIfEnabled(*logger, level, location, std::forward(make_message)); + } +} + +/// \brief Runtime-level variant against an explicit logger: on kFatal run the +/// shared fatal sequence; otherwise emit if enabled. +template +void LogToExplicitRuntime(Logger& logger, LogLevel level, + const std::source_location& location, + MakeMessage&& make_message) noexcept { + if (level == LogLevel::kFatal) { + DispatchFatal(&logger, location, std::forward(make_message)); + } + EmitIfEnabled(logger, level, location, std::forward(make_message)); +} + +/// \brief Fixed-severity fatal path (ICEBERG_LOG_FATAL): route the effective +/// (scoped-or-default) logger through the shared fatal sequence. Never returns. +template +[[noreturn]] void LogFatal(const std::source_location& location, + MakeMessage&& make_message) noexcept { + auto logger = GetCurrentLogger(); // keep the shared_ptr alive across the call + DispatchFatal(logger.get(), location, std::forward(make_message)); +} + } // namespace iceberg::internal // --------------------------------------------------------------------------- diff --git a/src/iceberg/test/macros_test.cc b/src/iceberg/test/macros_test.cc index a92e03795..98128a4cb 100644 --- a/src/iceberg/test/macros_test.cc +++ b/src/iceberg/test/macros_test.cc @@ -204,4 +204,30 @@ TEST(MacrosDeathTest, FatalHandlerRunsEvenWhenRecordSuppressed) { "H\\[suppressed 7\\]"); } +// The FatalHandler also runs when kFatal is reached via the runtime-level generic +// macro (not just fixed ICEBERG_LOG_FATAL). +TEST(MacrosDeathTest, FatalHandlerRunsOnGenericRuntimeFatal) { + EXPECT_DEATH( + { + SetFatalHandler([](const std::source_location&, std::string_view message) { + std::cerr << "GEN[" << message << "]\n"; + }); + ICEBERG_LOG(LogLevel::kFatal, "gen {}", 5); + }, + "GEN\\[gen 5\\]"); +} + +// ...and when kFatal is reached via ICEBERG_LOG_TO on an explicit logger. +TEST(MacrosDeathTest, FatalHandlerRunsOnLogToFatal) { + EXPECT_DEATH( + { + SetFatalHandler([](const std::source_location&, std::string_view message) { + std::cerr << "TO[" << message << "]\n"; + }); + CerrLogger sink(LogLevel::kTrace); + ICEBERG_LOG_TO(sink, LogLevel::kFatal, "to {}", 6); + }, + "TO\\[to 6\\]"); +} + } // namespace iceberg