A simple multithreaded FileLogger in C++

Question

In order to learn about multithreading programming in C++, I am implementing a basic multithreaded logger.

I use a std::deque to store messages inside a FileLogger class. Each time a thread logs a message, that message is pushed to the back of the deque.

In a separate thread the FileLogger checks if there are any messages in the deque and if so writes them to file.

Access to the deque is guarded by a mutex.

In order to make it easy to log from anywhere, the logger is implemented as a singleton.

Is my code correct? How can it be improved?

// FileLogger.h:
class FileLogger
{
public:
    static void initialize(const char* filePath) { // called by main thread before any threads are spawned
        instance_ = new FileLogger(filePath);
    }
    static FileLogger* instance() { // called from many threads simultaneously
        return instance_;
    }
    void log(const std::string &msg);
private:
    FileLogger(const char* filePath);
    void writeToFile();
    static FileLogger*     instance_;
    std::deque<std::string> messages;
    std::mutex         messagesMutex; // lock/unlock this each time messages is pushed or popped
    std::ofstream               fout;
    std::thread         writerThread;
};
// FileLogger.cpp:
FileLogger* FileLogger::instance_ = nullptr;

void FileLogger::writeToFile() {
    for (;;) {
        std::string message;
        while (messages.empty()) {
            std::this_thread::sleep_for(std::chrono::nanoseconds(10));
        }
        messagesMutex.lock();
        message = messages.front();
        messages.pop_front();
        messagesMutex.unlock();
        fout << message << std::endl << std::flush;
    }
}

FileLogger::FileLogger(const char* filePath) {
    fout.open(filePath);
    std::thread t(&FileLogger::writeToFile, this);
    writerThread = std::move(t);
}

void FileLogger::log(const std::string &msg) {
    std::lock_guard<std::mutex> lg(messagesMutex);
    messages.push_back(msg);
}

Answer 1

Just up front, your code doesn't have any blatant bugs which would prevent its use. There are a few things that you could do to improve it though:

• One thing that I'd try to avoid is singletons. These are effectively globals and bring with them almost all their problems. In particular, it prevents isolating code for unit tests. The alternative is that you pass a logger to the constructor of every object that needs it. This is called dependency injection, as a keyword for future research.
• Instead of fout.open(path), you could pass the filename to the constructor of fout using the initializer list.
• Your logger isn't copyable or assignable due to its members. I'd make that explicit though.
• When writing, std::endl already flushes the stream, so the explicit std::flush is redundant.
• The biggest issue is the way your writing thread operates. In effect, it wakes up every 10ns (that's 100.000.000 times per second) and looks for work (a.k.a. polling). Firstly, I can't imagine that this fast response time is actually relevant to a logger. If you have 0.1s delay between writing to the logger and seeing the data in the file, that should be enough by far. However, that's not a solution. What you actually want there is a condition variable, which allows you to only wake up the writer thread when actual works needs to be done.
• There is a comment lock/unlock this each time messages is pushed or popped, which is already in the right direction. What has helped me in the past is to document which mutex protects which data. It may even be a good idea to put just those parts into a separate struct in some cases, just to make it clearer. Also, for all other members, document who is allowed to access them. For example, fout doesn't require synchronization, because it is exclusively accessed by the writer thread.
• A minor fault, but still a fault is also that you access the queue without locking to check if it is non-empty. Since this typically only involves a simple pointer comparison (though that's implementation-defined), it will probably not cause issue. Using a condition variable as sketched below fixes this part as well.
• You could implement this using lock-free algorithms. This is a complete rewrite though and not "smoothing out the rough edges" of your current approach. Just keep that in mind as future research project perhaps.
• A minor flaw is that there is no way to shut down the logger. This is probably harmless, but it will probably trigger memory debuggers (leak detectors). If you decide not to implement shutdown, you don't need writerThread though, just call t.detach() after starting.
• If your target is C++ 17 or later, you could also use the filesystem library which has a dedicated path type. I'd generally prefer that over raw char* string. Even without C++ 17, I'd use std::string, unless I have good reasons not to.
• Concerning pushing on the queue, consider using emplace_back() instead, to avoid one more copying operation.
• Concerning removal from the queue, there is a trick: Create a second, local queue (initially empty) and swap that with the member (holding a lock for the time of that operation, of course). Then you have all the content in a local object that you can process at leasure, without having to worry about locking. This becomes important when retrieving multiple messages at once, so that you don't repeatedly lock/unlock without need. This can lead to "thread convoys": Imagine one thread perpetually writing messages using lock/push/unlock triples, so it becomes a stream of lock/push/unlock/lock/push/unlock... Now, if this thread ever gets scheduled out while it holds the lock and the writer thread is scheduled in, it will then itself run a lock/pop/unlock/lock/pop/unlock.. stream. On the first lock, it will block until the logging thread continues. This thread will be able to write one message, before it is blocked by the writer thread. In other words, ownership of the mutex will pingpong between the two threads, wrecking performance. Also, this is hard to catch, because it both comes and goes spontaneously.

For reference, here is a sketch of the push and pop operations using a CV:

mutex mtx;
condition_variable cv;
deque<string> queue;

void push(string s) {
    unique_lock<mutex> lck(mtx);
    queue.push_back(s);
    lck.unlock();
    cv.notify_one();
}

string pop() {
     unique_lock<mutex> lck(mtx);
     cv.wait(lck, []{ return !queue.empty();})
     string res = queue.front();
     queue.pop_front();
     return res;
}

This is not much more than the examples given by cppreference.

Answer 2

Your singleton initialization pattern is unsafe for a logger that you may want to use from other static initializers (that have undefined initialization order) because you cannot guarantee that the initialize function has been executed.

Typically you would use C++11 magic statics for this, but then you can't give a filename to it. Providing a filename as user input is always going to be a problem if you need to log from static initializes that run before main(). Yes, you could read the log filename from a static configuration file, but you might want to log errors from the config parse itself, and at that point the log path could be static as well. In this particular case, I recommend picking a static or generated file path for your logs.

Next up, you will basically keep a whole CPU core busy with polling every 10ns. Use std::condition_variable to sleep the thread until it had logs to write.

Finally your logging thread never exits; this is a problem depending on the platform: the behaviour of the process when you return from main() with active threads can differ. For example, read If you return from the main thread, does the process exit? by Raymond Chen. Your logger class should have a destructor that stops the thread's loop, joins the thread and deletes the singleton instance (not necessary if you use magic statics).

Answer 3

It seems your code will work expected. I am sure that there are other improvements for multithreading but I will just suggest some improvements in terms of readability and usability. I hesitated to share but maybe it could be beneficial for you or others.

• Firstly, intuitive expectation of most of developers is using operator << to stream out things.

• Second, actually singleton is detail of your implementation. It might be hidden in a class. It also provides you to change the implementation later without breaking backward compatibility.

So it is possible to support with the help of std::stringstream combined with RAII technique.

Possible implementation :

#include <iostream>
#include <sstream>
#include <functional>

struct singleton_impl
{

    static singleton_impl& instance()
    {
        static singleton_impl impl;
        return impl;
    }

    void log( const std::string& l )
    {
        std::cout << l << std::endl;
    }
};

template<typename Fn>
class message
{
    public:

        message( Fn&& fn ) : m_f { std::forward<Fn>( fn ) } 
        {  }

        ~message() {
            // Push to message queue
            m_f( m_ss.str() );
        }

        template<typename T>
        message& operator<<( const T& msg ) 
        {
            m_ss << msg;
            return *this;
        }

    private:

        std::stringstream m_ss;
        Fn m_f;
};

struct logger
{

    auto operator()() {
        return message { std::bind( static_cast< void(logger::*)( const std::string& )>( &logger::log ) , this , std::placeholders::_1 ) };
    }

    void log( const std::string& l )
    {
        // std::cout << l << std::endl;
        singleton_impl::instance().log( l );
    }
};

int main()
{
    logger logger;

    logger() << "Hello, world ! a number : " << 15 << ". Supports 'operator <<'";
    logger.log( "another way" );


    {
        logger() << "outputs at end of the scope.";
    }

    auto output = logger();
    output << "Output over object.";
    output << "Appended to the previous text.";

    return 0;
}

Hello, world ! a number : 15. Supports 'operator <<'
another way
outputs at end of the scope.
Output over object.Appended to the previous text

run online

• Another improvement could be making it extendable with the help of metaprogramming. I won't implement but the interface could be like this.

  #include <iostream>
  #include <string>
  
  struct with_thread_id {};
  struct with_timestamp {};
  struct with_this_token {
      with_this_token( const std::string& t ) : m_token { t } {}
  
      private:
          std::string m_token;
  };
  // And many more
  
  template< typename... Features >
  class logger_impl {
      // Implement somehow
  };
  
  using logger = logger_impl< with_thread_id , with_timestamp >;
  
  int main()
  {
      logger l;
  
      l() << "There will be `thread_id` and `timestamp` before this text.";
  
      return 0;
  }
  

• Another little performance improvement in your code. Use move assignment instead of copy assignment when it is possible.

   // message = messages.front();
   message = std::move( messages.front() );
  

• For sake of better readability, std::scoped_lock could be used instead of lock()/unlock() pair.

  /* messagesMutex.lock();
  message = messages.front();
  messages.pop_front();
  messagesMutex.unlock(); */
  
  {
      std::scoped_lock lock { messagesMutex };
      message = messages.front();
      messages.pop_front();
  }