odygrd / quill

Asynchronous Low Latency C++ Logging Library
MIT License
1.14k stars 130 forks source link

Prevent mismatched sink/logger #453

Closed brendene closed 1 month ago

brendene commented 1 month ago

Hello again and thank you for the work on the new quill v4.x

In: https://github.com/odygrd/quill/blob/master/examples/bounded_dropping_queue_frontend.cpp

If a user were to accidentally write:

  auto console_sink = CustomFrontend::create_or_get_sink<quill::ConsoleSink>("sink_id_1");                                                    
  quill::Logger* logger = quill::Frontend::create_or_get_logger("root", std::move(console_sink)); 

The code compiles fine but I would guess that the mismatched FrontendOptions in the sink/logger might create some issues. If not, please disregard, but if there are issues can this be prevented compile time?

minor: s/CustommLogger/CustomLogger/

edit: On further inspection it looks like the sink is independent of the FrontEndOptions so it should work just fine

odygrd commented 1 month ago

I really wanted to eliminate the need for preprocessor variable definitions for frontend configuration, which must be determined at compile time, and I couldn't find a better way to do it than this.

When using CustomFrontend, users must be careful not to use the internally defined default Frontend, which exists for convenience.

The library now requires a bit more attention when using CustomFrontend compared to previous versions, but I believe it's worth it to remove the preprocessor flags.

For sink creation, there shouldn't be any issues. However, problems may arise if you create both a Logger* and a LoggerImpl<CustomFrontend> and use them simultaneously. This will create a second thread context for the same thread, resulting in two lock-free queues for the same thread when only one is expected by design. I haven't thoroughly tested what happens in this scenario, but if I find a way to prevent such an error, I will implement it.

Retrieving an already created Logger* with the wrong Frontend is already checked with a dynamic cast, so the only real danger is the initial logger creation

odygrd commented 1 month ago

It works fine even if you misuse both FrontendOptions and CustomFrontendOptions. The only issue is that a single thread will then have two lock-free queues. I have added an assertion to prevent this. You can trigger it like this:

#include "quill/Backend.h"
#include "quill/Frontend.h"
#include "quill/sinks/ConsoleSink.h"
#include "quill/LogMacros.h"
#include "quill/Logger.h"
#include <utility>

struct CustomFrontendOptions
{
  static constexpr quill::QueueType queue_type = quill::QueueType::BoundedDropping;
  static constexpr uint32_t initial_queue_capacity = 131'072;
  static constexpr uint32_t blocking_queue_retry_interval_ns = 800;
  static constexpr bool huge_pages_enabled = false;
};

using CustomFrontend = quill::FrontendImpl<CustomFrontendOptions>;
using CustomLogger = quill::LoggerImpl<CustomFrontendOptions>;

int main()
{
  quill::BackendOptions backend_options;
  quill::Backend::start(backend_options); // or quill::Backend::start_with_signal_handler<CustomFrontendOptions>();

  auto console_sink = CustomFrontend::create_or_get_sink<quill::ConsoleSink>("sink_id_1");
  CustomLogger* logger = CustomFrontend::create_or_get_logger("root", std::move(console_sink));

  auto console_sink_b = quill::Frontend::create_or_get_sink<quill::ConsoleSink>("sink_id_1");
  quill::Logger * logger_b = quill::Frontend::create_or_get_logger("the_test", std::move(console_sink_b));

  LOG_INFO(logger, "This is a log info example {}", 123);
  LOG_WARNING(logger, "This is a log warning example {}", 123);
  LOG_INFO(logger_b, "This is a log info example {}", 123);
  LOG_WARNING(logger_b, "This is a log warning example {}", 123);
}