Simple multithreaded FileLogger in C++

Question

I'm new to C++ and am following up on: A simple multithreaded FileLogger in C++

I am curious to know if I implemented his ideas correctly, if there are further improvements to the code and whether it's (still) correct.

FileLoggerThread.h

#pragma once
#include <string>
#include <fstream>
#include <mutex>
#include <deque>
#include <condition_variable>
#include <filesystem>

class FileLoggerThread
{
public:
    explicit FileLoggerThread(std::filesystem::path filePath);
    ~FileLoggerThread();                            // makes code more easily unit testable (unit tests can otherwise be in an infinite loop)

    void log(const std::string &msg);

    FileLoggerThread(const FileLoggerThread&) = delete;
    FileLoggerThread& operator=(const FileLoggerThread&) = delete;

private:
    void writeToFile();

    std::deque<std::string> mMessages;              // accessed by log (any thread) and writeToFile (the writer thread)
    std::ofstream mOutputFile;                      // only accessible by writeToFile
    
    // thread internal resources
    std::mutex mMessagesMutex;                      // lock/unlock this each time messages is pushed or popped
    std::condition_variable mThreadScheduler;       // log needs to wake up writeToFile
    bool mIsRunning;                                // if set to false then the writer thread stops
};

FileLoggerThread.cpp

#include "FileLoggerThread.h"
#include <thread>

FileLoggerThread::FileLoggerThread(std::filesystem::path filePath) : mOutputFile {filePath}, mIsRunning {true} {
    std::thread t(&FileLoggerThread::writeToFile, this);
    t.detach();
}

void FileLoggerThread::writeToFile() {
    while (mIsRunning) {
        std::deque<std::string> localMessages;
        std::unique_lock<std::mutex> writerLock {mMessagesMutex};
        mThreadScheduler.wait(writerLock, [&]{ return !mMessages.empty() || !mIsRunning;});
        localMessages.swap(mMessages);
        writerLock.unlock();

        for (const auto& message : localMessages) {
            mOutputFile << message << std::endl;
        }
    }
}

void FileLoggerThread::log(const std::string &msg) {
    std::unique_lock loggerLock(mMessagesMutex);
    mMessages.emplace_back(msg);
    loggerLock.unlock();
    mThreadScheduler.notify_one(); // done for performance reasons, see: https://youtu.be/F6Ipn7gCOsY?t=2205
}

FileLoggerThread::~FileLoggerThread() {
    mIsRunning = false;
    mThreadScheduler.notify_one();
}

Answer 1

• It is not a good idea to have a completely detached thread. When one detaches thread, they usually use some sort of notification system like future/promise to ensure that the thread finished using resources. Here you file logger might have finished working and got destroyed while the detached thread still accesses this leading to UB. Instead you should keep thread as a part of the logger and trigger join in the destructor.

• You shouldn't use std::endl; for every message - the flushing is a slow operation. You surely don't have a good reason to trigger it more often than more than once a second . Just make sure that you do trigger it once a second or so. std::condition_variable::wait_for/wait_until could be used to ensure that.

• Another bug in destructor is that you modify mIsRunning without locking the mutex. Technically, it is a UB. In practice, this bug results in some very rare cases with the wait missing the notification and lasting eternity.

• With the scheme that you are running I don't see why you use a std::deque for holding messages. Why not just std::vector? It is more efficient and also you shouldn't create new localMessages each time - just keep one constantly existing and clear it after each use. It saves some unnecessary memory allocations.

• Also, once mIsRunning is false you need to ensure that all messages were written instead of quitting. Moreover, you shouldn't access mIsRunning in the while-loop condition as it should be guarded by the mutex.

• The log function void log(const std::string &msg); accepts a string and copies it into the queue. It is a waste. You'd better have two other overloads: void log(std::string&& msg); and void log(std::string_view msg); with the former moving the string and it's content for printing instead of unnecessarily copying it, while latter is just a more general form of const std::string&.

• In general, logger should support features like verbosity filterting and identification (e.g., info, warning, error), timestamping, and similar stuff that should be inherent part of a logger. Here you just accept arbitrary messages.