5
\$\begingroup\$

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);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Do you ask for improvement in terms of only multithreading ? \$\endgroup\$ – arnes Mar 2 at 12:55
  • \$\begingroup\$ My main focus is multithreading but I am grateful for any improvements. \$\endgroup\$ – Andy Mar 2 at 12:56
  • 1
    \$\begingroup\$ If you have new concerns, please post a new question linking back to this one for extra context. Modifying the question after answers came in tends to create a mess. Thank you. \$\endgroup\$ – Mast Mar 3 at 8:24
  • 1
    \$\begingroup\$ Hi, i am kind of a new learner. I think your code looks good but I was wondering that when do you close the file? Did std::ofstream handles that? thanks. \$\endgroup\$ – thehilmisu Mar 3 at 14:42
  • \$\begingroup\$ @thehilmisu: good point. I don't. When the program exits all filehandles are released. Not really nice. Should be fixed. \$\endgroup\$ – Andy Mar 3 at 14:50
9
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ How do can you wait and wake up on notification in lock-free case? In C++20 atomic variables have wait and notify. One can make fake lock std::condition_variable_any but I heard its implementations tend to be dubius. \$\endgroup\$ – ALX23z Mar 2 at 14:23
  • 1
    \$\begingroup\$ In unit tests, you could also use a logger that makes sure that certain things are logged, not just one that does nothing. Both loggers could share a common interface (baseclass). Using a factory, I'd say no, because you just replace one dependency with another. A factory is useful when you need to create different types of objects based on a parameter at runtime, but here it's just one and its type is fixed. Don't overcomplicate unnecessarily. \$\endgroup\$ – uli Mar 2 at 17:25
  • 2
    \$\begingroup\$ Is it not worrying that the check messages.empty() is being done without a lock? by the time you check and the time you execute another thread could have finished all the work. @url regarding your last point, did you have in mind a more complext situation? How is it different than what is it doing now, just copying the variable to a local and then releasing the lock? \$\endgroup\$ – Ant Mar 2 at 21:32
  • 1
    \$\begingroup\$ @Ant: regarding messages.empty() being done without a lock: I think this is an important point. I find it difficult to understand exactly how this can go wrong. If empty() returns false and the message has not been completely pushed into the deque that would cause a crash when I call front(). Not sure if a push_back would increase size before actually inserting the element. Anyway I think it is probably safest to put the empty() inside a mutex lock/unlock pair? \$\endgroup\$ – Andy Mar 3 at 7:39
  • 1
    \$\begingroup\$ I just slapped my forehead, @Ant, because you're right. Any access to a shared, mutable resource should happen with a lock, unless there's a good reason not to. I guess that I missed this because I had the change in mind that uses a condition variable, which basically guides you into the right direction already and resolves this error as well. \$\endgroup\$ – uli Mar 3 at 11:52
4
\$\begingroup\$

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

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

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();
    }  
    
| improve this answer | |
\$\endgroup\$

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.