Low-Latency Multithreaded Logger

Question

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();
}

Answer 1

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);
    }
}

---