1
\$\begingroup\$

My goal was to have a logger that does the blocking file I/O in a separate thread.

A few notes:

  1. I made it a singleton rather than having a global "logger" variable, or a bunch of loggers that are passed around. It is used by saying Logger::Log("Hello, this is a log").
  2. Since the class is a singleton, it feels pointless to put the implementation variables in the class. I just kept the class declaration clean and put the variables in the CPP file. This is unconventional I think, but please let me know your thoughts on this.
  3. This is slightly stripped down, the main code has some stuff that adds timestamps, etc. but I wanted to focus on the thread synchronization because that is new to me.
  4. I strongly prefer the C-style "printf" string formatting, so that is why I used it.

Header:

class Logger {
    static Logger* instance;
    Logger(const char *path);
    Logger(const Logger& o);
    void operator = (const Logger& o);
    void push(const std::string &str);

public:
    static void Init(const char* path);     //not thread safe, don't be an idiot
    static void Log(const char* s, ...);    //is thread safe
};

CPP:

Logger * Logger::instance = 0;

mutex q_mutex;
queue <string> log_queue;

condition_variable log_data_ready_cv;

FILE* log_file;

void log_monitor() {
    for (;;) {
        unique_lock lock(q_mutex);
        log_data_ready_cv.wait(lock, [] {return !log_queue.empty(); });
        string s = log_queue.front();
        log_queue.pop();
        
        lock.unlock();

        fputs(s.c_str(), log_file);
        fputc('\n', log_file);
        fflush(log_file);
    }
}

Logger::Logger(const char* path) {
    log_file = fopen(path, "a");
}

void Logger::Init(const char* path) {
    if (instance) return;
    instance = new Logger(path);

    thread(&log_monitor).detach();
}

void Logger::Log(const char* s, ...) {
    if (!instance) return;

    const size_t MaxLen = 2000;
    char line[MaxLen];
    
    va_list args;
    va_start(args, s);
    int len = vsnprintf(line, MaxLen, s, args);
    va_end(args);

    if (len < 0) return;    //encoding error
    if (len >= MaxLen) return;  //not enough space

    instance->push(line);
}

void Logger::push(const string &str, int len) {
    q_mutex.lock();
    log_queue.push(str);
    q_mutex.unlock();
    log_data_ready_cv.notify_one();
}
\$\endgroup\$
3
  • \$\begingroup\$ Why do you prefer C-style printf? I can understand not liking streaming style - cause it's awful. But in comparison to fmt::format? fmt::format is safer, faster, more general, and much more convenient than C-style printf. \$\endgroup\$
    – ALX23z
    Commented Apr 11, 2022 at 17:22
  • \$\begingroup\$ I've never heard of fmt::format but in general I rarely use 3rd party libraries. I am curious about this new library, do you know of any benchmarks? \$\endgroup\$ Commented Apr 11, 2022 at 17:35
  • \$\begingroup\$ it's a couple of years old and some of it's features were accepted into C++20. Yes, you can find some benchmarks. It is slightly faster than printf because certain operations can be done at compile-time... though this depends on compiler and library version. \$\endgroup\$
    – ALX23z
    Commented Apr 11, 2022 at 18:04

1 Answer 1

1
\$\begingroup\$

Comments

A Singletons. Everybody's love hate pattern.

You should check out this:

I actually like the idea of Singleton so unlike a lot of people I am not going to complain about that. BUT I am going to complain that you did not think about the creation of the Singleton. The main issue with the Singleton is that it can cause hard dependencies unless you separate its creation into another constructor pattern.

Please No. Raw pointers!

Logger * Logger::instance = 0;

Children threads need to be taken account of.

thread(&log_monitor).detach();

It is undefined behaviour if the main thread of execution finishes before a child thread. So you can simply detach the thread here. You need to know its finished before you can let the application quit.

Code Review

The classic Myers Singleton (though I was using it before that so I think it should be the Loki Singleton :-) ) uses a static function variable to hold the instance.

    static Logger* instance;

Rather than a static global pointer!!!

The benefit of this is that you know the lifespan of the object and know it is going to be correctly destroyed (hey and here is a though, could keep track of your thread).


This is the old way of removing the copy ability of a class.

    Logger(const Logger& o);
    void operator = (const Logger& o);

Since C++11 you can declare them as deleted.

    Logger(Logger const& o)            = delete;
    void operator=(Logger const& o)    = delete;

This means you find out during compilation rather than linking when you make a mistake.


Why are these global?

mutex q_mutex;
queue <string> log_queue;
condition_variable log_data_ready_cv;
FILE* log_file;

Should these not be part of your logger class?


Lets try and move the string around to prevent copying and use RAII to handle the logging. I don't see any danger from exceptions now, but you need to think about future maintenance (where more complexity may be added).

void log_monitor() {
    for (;;) {
        unique_lock lock(q_mutex);
        log_data_ready_cv.wait(lock, [] {return !log_queue.empty(); });
        string s = log_queue.front();
        log_queue.pop();
        
        lock.unlock();

        fputs(s.c_str(), log_file);
        fputc('\n', log_file);
        fflush(log_file);
    }
}

I would write like this:

std::string getNextMessage()
{
    unique_lock lock(q_mutex);
    log_data_ready_cv.wait(lock, [] {return !log_queue.empty(); });
    string s = std::move(log_queue.front());
    log_queue.pop();

    return s;
}
        
void log_monitor()
{
    for (;;) {
        std::string s = getNextMessage()

        fputs(s.c_str(), log_file);
        fputc('\n', log_file);
        fflush(log_file);
    }
}

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your useful comments. The justification for making all the variables global instead of putting them in the class was that since it's a singleton anyway, there is only 1 set of data, so it keeps the header file simpler. (Kind of like a p_impl pattern but with only one implementation) \$\endgroup\$ Commented Apr 9, 2022 at 8:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.