From 314f6aa2841dff9d92dad2134a8686c8bda17fb7 Mon Sep 17 00:00:00 2001 From: Odysseas Georgoudis Date: Thu, 26 Sep 2024 01:23:05 +0100 Subject: [PATCH] explicitly specify the `Logger` used by the built-in `SignalHandler` --- CHANGELOG.md | 3 + include/quill/Backend.h | 9 +- include/quill/backend/SignalHandler.h | 31 ++++- test/integration_tests/CMakeLists.txt | 1 + .../SignalHandlerLoggerTest.cpp | 109 ++++++++++++++++++ test/integration_tests/SignalHandlerTest.cpp | 2 +- 6 files changed, 146 insertions(+), 9 deletions(-) create mode 100644 test/integration_tests/SignalHandlerLoggerTest.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e2aa995..61478326 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,8 +80,11 @@ ## v7.3.0 +- Added the option to explicitly specify the `Logger` used by the built-in `SignalHandler` for logging errors during + application crashes. ([#590](https://github.com/odygrd/quill/issues/590)) - Prevented error logs from the `SignalHandler` from being output to CSV files when a `CsvWriter` is in use. ([#588](https://github.com/odygrd/quill/issues/588)) +- Implemented a workaround to resolve false positive warnings from `clang-tidy` on Windows. ## v7.2.2 diff --git a/include/quill/Backend.h b/include/quill/Backend.h index 26829e5e..4c6564bc 100644 --- a/include/quill/Backend.h +++ b/include/quill/Backend.h @@ -60,6 +60,9 @@ class Backend * function. The signal handler sets up an alarm to ensure that the process will terminate if it * does not complete within the specified time frame. This is particularly useful to prevent the * process from hanging indefinitely in case the signal handler encounters an issue. + * @param signal_handler_logger The logger instance that the signal handler will use to log errors when the application crashes. + * The logger is accessed by the signal handler and must be created by your application using Frontend::create_or_get_logger(...). + * If the specified logger is not found, or if this parameter is left empty, the signal handler will default to using the first valid logger it finds. * * @note When using the SignalHandler on Linux/MacOS, ensure that each spawned thread in your * application has performed one of the following actions: @@ -79,10 +82,10 @@ class Backend BackendOptions const& options = BackendOptions{}, QUILL_MAYBE_UNUSED std::initializer_list const& catchable_signals = std::initializer_list{SIGTERM, SIGINT, SIGABRT, SIGFPE, SIGILL, SIGSEGV}, - uint32_t signal_handler_timeout_seconds = 20u) + uint32_t signal_handler_timeout_seconds = 20u, std::string const& signal_handler_logger = {}) { std::call_once(detail::BackendManager::instance().get_start_once_flag(), - [options, catchable_signals, signal_handler_timeout_seconds]() + [options, catchable_signals, signal_handler_timeout_seconds, signal_handler_logger]() { #if defined(_WIN32) (void)catchable_signals; @@ -100,6 +103,8 @@ class Backend // Run the backend worker thread, we wait here until the thread enters the main loop detail::BackendManager::instance().start_backend_thread(options); + detail::SignalHandlerContext::instance().logger_name = signal_handler_logger; + detail::SignalHandlerContext::instance().signal_handler_timeout_seconds.store( signal_handler_timeout_seconds); diff --git a/include/quill/backend/SignalHandler.h b/include/quill/backend/SignalHandler.h index 67a8b978..bc4d12db 100644 --- a/include/quill/backend/SignalHandler.h +++ b/include/quill/backend/SignalHandler.h @@ -58,8 +58,28 @@ class SignalHandlerContext return instance; } + /***/ + QUILL_NODISCARD LoggerBase* get_logger() noexcept + { + LoggerBase* logger_base{nullptr}; + + if (!SignalHandlerContext::instance().logger_name.empty()) + { + logger_base = detail::LoggerManager::instance().get_logger(SignalHandlerContext::instance().logger_name); + } + + // This also checks if the logger was found above + if (!logger_base || !logger_base->is_valid_logger()) + { + logger_base = detail::LoggerManager::instance().get_valid_logger(SignalHandlerContext::excluded_logger_name_substr); + } + + return logger_base; + } + static constexpr std::string_view excluded_logger_name_substr = {"__csv__"}; + std::string logger_name; std::atomic signal_number{0}; std::atomic lock{0}; std::atomic backend_thread_id{0}; @@ -138,8 +158,7 @@ void on_signal(int32_t signal_number) else { // This means signal handler is running on a frontend thread, we can log and flush - LoggerBase* logger_base = - detail::LoggerManager::instance().get_valid_logger(SignalHandlerContext::excluded_logger_name_substr); + LoggerBase* logger_base = SignalHandlerContext::instance().get_logger(); if (logger_base) { @@ -251,8 +270,8 @@ BOOL WINAPI on_console_signal(DWORD signal) (signal == CTRL_C_EVENT || signal == CTRL_BREAK_EVENT)) { // Log the interruption and flush log messages - LoggerBase* logger_base = - detail::LoggerManager::instance().get_valid_logger(SignalHandlerContext::excluded_logger_name_substr); + LoggerBase* logger_base = SignalHandlerContext::instance().get_logger(); + if (logger_base) { auto logger = reinterpret_cast*>(logger_base); @@ -279,8 +298,8 @@ LONG WINAPI on_exception(EXCEPTION_POINTERS* exception_p) if ((backend_thread_id != 0) && (current_thread_id != backend_thread_id)) { // Log the interruption and flush log messages - LoggerBase* logger_base = - detail::LoggerManager::instance().get_valid_logger(SignalHandlerContext::excluded_logger_name_substr); + LoggerBase* logger_base = SignalHandlerContext::instance().get_logger(); + if (logger_base) { auto logger = reinterpret_cast*>(logger_base); diff --git a/test/integration_tests/CMakeLists.txt b/test/integration_tests/CMakeLists.txt index 6bbb9621..c4844422 100644 --- a/test/integration_tests/CMakeLists.txt +++ b/test/integration_tests/CMakeLists.txt @@ -80,6 +80,7 @@ quill_add_test(TEST_RotatingSinkDailyRotation RotatingSinkDailyRotationTest.cpp) quill_add_test(TEST_RotatingSinkKeepOldest RotatingSinkKeepOldestTest.cpp) quill_add_test(TEST_RotatingSinkOverwriteOldest RotatingSinkOverwriteOldestTest.cpp) quill_add_test(TEST_SignalHandler SignalHandlerTest.cpp) +quill_add_test(TEST_SignalHandlerLogger SignalHandlerLoggerTest.cpp) quill_add_test(TEST_SingleFrontendThread SingleFrontendThreadTest.cpp) quill_add_test(TEST_SinkFilter SinkFilterTest.cpp) quill_add_test(TEST_StdArrayLogging StdArrayLoggingTest.cpp) diff --git a/test/integration_tests/SignalHandlerLoggerTest.cpp b/test/integration_tests/SignalHandlerLoggerTest.cpp new file mode 100644 index 00000000..912bbf08 --- /dev/null +++ b/test/integration_tests/SignalHandlerLoggerTest.cpp @@ -0,0 +1,109 @@ +#include "doctest/doctest.h" + +#include "misc/TestUtilities.h" +#include "quill/Backend.h" +#include "quill/CsvWriter.h" +#include "quill/Frontend.h" +#include "quill/LogMacros.h" +#include "quill/sinks/FileSink.h" + +#include +#include +#include +#include +#include + +using namespace quill; + +/** + * Redirect the signal handler error to specific Logger + */ +TEST_CASE("signal_handler_logger") +{ + static constexpr char const* filename_a = "signal_handler_logger_a.log"; + static constexpr char const* filename_b = "signal_handler_logger_b.log"; + + // Loggers are sorted in alphabetical order and signal handler by default can pick the first + // but we specify a different once + static std::string const logger_name_a = "a_logger"; + static std::string const logger_name_b = "b_logger"; + static constexpr size_t number_of_messages = 10; + + // Start the logging backend thread, we expect the signal handler to catch the signal, + // flush the log and raise the signal back + Backend::start_with_signal_handler( + BackendOptions{}, std::initializer_list{SIGABRT}, 40, logger_name_b); + + // For testing purposes we want to keep the application running, we do not reraise the signal + detail::SignalHandlerContext::instance().should_reraise_signal.store(false); + +#if defined(_WIN32) + // NOTE: On windows the signal handler must be installed on each new thread + quill::init_signal_handler(); +#endif + + Logger* logger_a = Frontend::create_or_get_logger(logger_name_a, + Frontend::create_or_get_sink( + filename_a, + []() + { + FileSinkConfig cfg; + cfg.set_open_mode('w'); + cfg.set_fsync_enabled(true); + return cfg; + }(), + FileEventNotifier{})); + + // Create the logger for the signal handler to find it + Frontend::create_or_get_logger(logger_name_b, + Frontend::create_or_get_sink( + filename_b, + []() + { + FileSinkConfig cfg; + cfg.set_open_mode('w'); + cfg.set_fsync_enabled(true); + return cfg; + }(), + FileEventNotifier{})); + + for (size_t i = 0; i < number_of_messages; ++i) + { + LOG_INFO(logger_a, "A log info message {}", i); + } + + // Now raise a signal + std::raise(SIGABRT); + + { + std::vector const file_contents_a = quill::testing::file_contents(filename_a); + REQUIRE_EQ(file_contents_a.size(), number_of_messages); + + // Except the log and the signal handler in the logger_b + std::vector const file_contents_b = quill::testing::file_contents(filename_b); + +#if defined(_WIN32) + REQUIRE(quill::testing::file_contains(file_contents_b, std::string{"Received signal: 22 (signum: 22)"})); +#elif defined(__apple_build_version__) + REQUIRE(quill::testing::file_contains( + file_contents_b, std::string{"Received signal: Abort trap: 6 (signum: 6)"})); +#elif defined(__FreeBSD__) + REQUIRE(quill::testing::file_contains(file_contents_b, std::string{"Received signal: Abort trap (signum: 6)"})); +#else + REQUIRE(quill::testing::file_contains(file_contents_b, std::string{"Received signal: Aborted (signum: 6)"})); +#endif + } + + // Wait until the backend thread stops for test stability + for (Logger* logger : Frontend::get_all_loggers()) + { + logger->flush_log(); + Frontend::remove_logger(logger); + } + + Backend::stop(); + REQUIRE_FALSE(Backend::is_running()); + + testing::remove_file(filename_a); + testing::remove_file(filename_b); +} \ No newline at end of file diff --git a/test/integration_tests/SignalHandlerTest.cpp b/test/integration_tests/SignalHandlerTest.cpp index 6da7dcc9..44530119 100644 --- a/test/integration_tests/SignalHandlerTest.cpp +++ b/test/integration_tests/SignalHandlerTest.cpp @@ -25,7 +25,7 @@ struct OrderCsvSchema TEST_CASE("signal_handler") { static constexpr char const* filename = "signal_handler.log"; - static constexpr char const* csv_filename = "aa.csv"; // use 'aa' because we sort Loggers alphabetically + static constexpr char const* csv_filename = "signal_handler.csv"; static std::string const logger_name = "logger"; static constexpr size_t number_of_messages = 10; static constexpr size_t number_of_threads = 10;