C++ multithreading logger class

Question

I have designed a logger class to log messages to a file. It uses an independent thread to log the messages save to a queue previously using the main thread. I want to receive reviews about it.

#include <iostream>
#include <queue>
#include <mutex>
#include <condition_variable>
#include <fstream>
#include <atomic>
#include <string>
using namespace std::chrono_literals;

class Logger
{
    std::mutex queueMutex;
    std::condition_variable condVar;
    std::queue<std::string> messagesQueue;
    std::thread loggingThread;//background process launcher
    std::atomic_bool exit = false;//safety condition
    void processEntries()//the background process
    {
        // Open log file.
        std::ofstream logFile("log.txt");
        if (logFile.fail()) {
            std::cerr << "Failed to open logfile." << std::endl;
            return;
        }

        // Start processing loop. It process only one message for each iteration
        // to gain more performance
        while (!exit) {
            std::unique_lock lock(queueMutex);

            // Wait for a notification and don't wakeup unless the queue isn't empty.
            condVar.wait(lock, [this]{return !messagesQueue.empty();});

            //log to the file
            logFile << messagesQueue.front() << std::endl;
            messagesQueue.pop();
        }

        //At the end, if the queue has some messages. here you don't need mutexes
        // because you have reached the destructor i.e you won't enqueue any messages anymore
        while(exit && !messagesQueue.empty()){

            //log to the file
            logFile << messagesQueue.front() << std::endl;
            messagesQueue.pop();
        }

    }

public:
    Logger()
    {
        //the default ctor launches the background process task
        loggingThread = std::thread{ &Logger::processEntries, this };
    }
    Logger(const Logger& src) = delete;
    Logger& operator=(const Logger& rhs) = delete;

    //logs the messages to the queue
    void log(std::string_view entry)
    {
        std::unique_lock lock(queueMutex);
        messagesQueue.push(std::string(entry));
        condVar.notify_all();
    }


    ~Logger()
    {
        exit = true;
        loggingThread.join();
    }

};


int main(){

    Logger lg;
    for(int i = 1;i < 10000;i++){
        lg.log("This is the message number " + std::to_string(i));
    }

    std::this_thread::sleep_for(10ms);

    for(int i = 10000;i < 20001;i++){
        lg.log("This is the message number " + std::to_string(i));
    }

}

Answer 1

1. std::ofstream logFile("log.txt"); the name should be configurable. Or at least have current time/date as a part of its name; otherwise you'll overwrite your log file each time you launch your program.

2. You should have an option to print messages to console.

3. The pop-from-queue implementation in processEntries is bugged.

   while (!exit) {
        std::unique_lock lock(queueMutex);
   
        condVar.wait(lock, [this]{return !messagesQueue.empty();});
        ....
    }
   

Now, when will it leave the wait if exit is called but no more messages are being forwarded? To make a proper wait, make exit part of the condition (you'll also have to lock the mutex whenever you change exit, so just make it a bool instead of atomic).

4. You should force-flush the stream once every few seconds. At times people want to see the log during run and they won't be able to if it isn't flushed. Without flushing you may also lose lots of log-information during crashes - like 16KB of text. However, force-flushing every message like you do with std::endl is a poor idea in terms of performance.

5. log(std::string_view entry) there is a general debate as to what is faster, copy a string or move a string - it surely depends on the size of the string... regardless, you should have a version that doesn't use a string_view but a string so you don't make an extra allocation in the queue. It improves health of memory fragmentation.

6. Normally, I'd expect a logger to print information "when" (time), "what"(log level - info, warning, error), and "who" (source of message) in addition to the message itself. It is important for logging.

7. You lack message filtration options depending on log-level as well as desired verbose level.

To implement 6 and 7, consider separating logger into two classes - one for writing log to file/console and another that wraps functionality and generates the messages - with former begin the shared state across all units while the latter being copied and modified for each unit so it can store private information of "who" sends the message as well as some configuration regarding importance of the log message.

Answer 2

Pretty good for a beginning, but I would have a lot of feature requests if I were a user.

1. Allow the user to provide a full file specification so that the file appears where the user expects it.
2. The logger should get the day and time of day and report that in each message.
3. The logger should be initialized with the program name so that the all the messages contain the program name, this way if multiple processes are writing to the same file one will know which process made the log entry.

Make it into a library by providing a header file and a C++ source file.