4
\$\begingroup\$

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

}
\$\endgroup\$
  • \$\begingroup\$ I' would suggest, when using the start value different from zero in for loops to use <= instead <, seems more idiomatic to me. \$\endgroup\$ – Ilkhd Jun 21 at 12:52
  • \$\begingroup\$ The problem with the code is that you are keeping the lock even when you you writing to file. This is time consuming, and it would cause unnecessary stalls of the calling thread. You need to release the lock once you acquired the message, before writing to the file. You also need to put some limit on the size of the queue. \$\endgroup\$ – Ilkhd Jun 21 at 13:09
  • \$\begingroup\$ @llkhd Does that mean If I used an std::string to save the message instead of writing to the file directly, then wrote to the file after releasing the lock, it would be more performant? – \$\endgroup\$ – asmmo Jun 21 at 13:35
  • \$\begingroup\$ @Ilkhd Your comments should be an answer. \$\endgroup\$ – pacmaninbw Jun 21 at 13:36
2
\$\begingroup\$
  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).

  1. 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.

  2. 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.

  3. 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.

  4. 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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ thanks a lot for catching the bug \$\endgroup\$ – asmmo Jun 22 at 7:00
2
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ If I used an std::string to save the message instead of writing to the file directly, then wrote to the file after releasing the lock, would it be more performant? \$\endgroup\$ – asmmo Jun 21 at 14:13
  • \$\begingroup\$ std::ofstream is already buffered, but you are negating the buffering by using std::endl, because endl flushes the buffer. \$\endgroup\$ – pacmaninbw Jun 21 at 15:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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