Skip to content

Commit

Permalink
explicitly specify the Logger used by the built-in SignalHandler
Browse files Browse the repository at this point in the history
  • Loading branch information
odygrd committed Sep 26, 2024
1 parent 42a1957 commit b8a801d
Show file tree
Hide file tree
Showing 6 changed files with 146 additions and 9 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
9 changes: 7 additions & 2 deletions include/quill/Backend.h
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -79,10 +82,10 @@ class Backend
BackendOptions const& options = BackendOptions{},
QUILL_MAYBE_UNUSED std::initializer_list<int> const& catchable_signals =
std::initializer_list<int>{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;
Expand All @@ -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);

Expand Down
31 changes: 25 additions & 6 deletions include/quill/backend/SignalHandler.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<int32_t> signal_number{0};
std::atomic<uint32_t> lock{0};
std::atomic<uint32_t> backend_thread_id{0};
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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<LoggerImpl<TFrontendOptions>*>(logger_base);
Expand All @@ -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<LoggerImpl<TFrontendOptions>*>(logger_base);
Expand Down
1 change: 1 addition & 0 deletions test/integration_tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
109 changes: 109 additions & 0 deletions test/integration_tests/SignalHandlerLoggerTest.cpp
Original file line number Diff line number Diff line change
@@ -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 <cstdio>
#include <memory>
#include <string>
#include <string_view>
#include <vector>

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<FrontendOptions>(
BackendOptions{}, std::initializer_list<int>{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<quill::FrontendOptions>();
#endif

Logger* logger_a = Frontend::create_or_get_logger(logger_name_a,
Frontend::create_or_get_sink<FileSink>(
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<FileSink>(
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<std::string> 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<std::string> 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);
}
2 changes: 1 addition & 1 deletion test/integration_tests/SignalHandlerTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit b8a801d

Please sign in to comment.