Async logger

[barnack]

I think i'm satisfied with the result, can anyone please criticise it?
Code on GitHub: https://github.com/Sephirothbahamut/Cpp-Async-Logger


_____________________________

Greetings,
i'm writing the very basics for a game (main engine, fixed-step/dynamic fps cycle, resources manager, base ingame object class, and finally logger.
I want my logger to be async, writing logs to file in a second thread.
I want that when the logger destructor is called, the writer thread will complete writing all logs remaining in the queue before joining the main thread, and only then it will be destroyed.
My current implementation will wait until current log is flushed, but won't wait until all the remaining logs in the queue (if any) will be written.
I'm kind of confused by mutexes interaction; also i got a safe queue from i dont remember where in the internet, and maybe i just need a bit less checks in it. It's seems made to be safe with multiple writers AND multiple readers, while in my case i can always assume there will be only one thread pushing strings to the queue and only another one popping them.

As a detail, i never tried overriding the left component of << operator, so i might have messed it but it's not the focus here.

Can anyone help me making this do what i actually want, and possibly explain how and why that will work?
Thanks.

#pragma once

#include <condition_variable>
#include <mutex>
#include <thread>
#include <queue>
#include <string>
#include <fstream>
#include <sstream>

namespace engine 
    {
    class SafeQueue;
    class Logger
        {
        private:
            SafeQueue queue;
            std::ofstream log_file;
            std::thread* thread = nullptr;
            bool running = true;

            void writer()
                {
                while (running)
                    {//when running is set to false writer reaches its end, letting the main thread get past "join"
                    log_file << queue.pop() << std::endl << std::flush;
                    //std::this_thread::sleep_for(std::chrono::seconds(1));
                    }
                }

        public:
            void log(std::string string);
                {
                logger.queue.push(string);
                }
                
            Logger()
                {
                log_file.open("log.txt"); 
                thread = new std::thread(&Logger::writer, this);
                }
                
            ~Logger()
                {
                running = false; //set running false to let writer thread stop as soon as it completes its current cycle
                thread->join(); //wait until writer thread regularly exited (leaving time to flush)
                delete thread; //delete it
                log_file.close(); //close the log file
                }
        };
        
    class SafeQueue
        {
        std::condition_variable work_available;
        std::mutex work_mutex;
        std::queue<std::string> queue;

        public:
            void push(std::string string)
                {
                std::unique_lock<std::mutex> lock(work_mutex);

                bool was_empty = queue.empty();
                queue.push(string);

                lock.unlock();

                if (was_empty)
                    {
                    work_available.notify_one();
                    }
                }
                
            std::string pop()
                {
                std::unique_lock<std::mutex> lock(work_mutex);
                while (queue.empty())
                    {
                    work_available.wait(lock);
                    }

                std::string tmp = queue.front();
                queue.pop();
                return tmp;
                }
        };
    }

[dutch]

Maybe your writer loop should be more like:

        while (running || !queue.empty())

I can't comprehend your abnormal operator<< overload.

You don't need std::flush after std::endl since endl includes a flush (that's the way it differs from a simple '\n').

Should the queue .unlock() be before or after the .notify_one() call? I'm not sure myself.

[barnack]

@dutch i never ever worked with threads, so i'm not sure how many things would interact. Thanks for pointing out that endl flushes too, i didnt know that.
Finally, i'm not sure about the operator<< either, i want to use my Logger the way you use cout or cerr; i'm not outputting the Logger, i'm outputting stuff to the logger.

[dutch]

Try the different loop condition. That might fix your problem. You'll need to add an .empty() function to SafeQueue.

As for the operator<< overload, I don't understand the second parameter. How would you use it?

[barnack]

replaced the << with .log because honestly i didnt know what i was doing there anyway and it wasn't that much important.
I'll try with your solution, although its gonna bi difficult to check if such things work with multithread.

[Niccolo]

I don't recall any text adequately covering threads. There were plenty of discussions about conditional variables, mutexes, locks, but few about practical methods using them beyond rather trivial examples.

About std::async vs Thread


Modern C++ takes the approach primarily dependent upon std::futures, std::promise and std::async.

The idea is that std::async has "knowledge" about system resources, and can therefore more intelligently choose whether or not to actually run the task on a thread. It isn't absolutely applicable to all threaded circumstances, especially if the intent is to run a function in a loop like that of, say, a physics engine in a game.

A logger does seem correct for std::async, but whether or not you'd prefer to use that is another matter of choice.

The reason is that for a logger, the logger's thread should wait quietly until there is material to write, without wasting resources. To do that you need an "autoreset event", an object the thread can "wait" on indefinitely until it is signaled to run. An "autoreset event" object can be created using waitable objects. Boost has one pre-made (and Windows has such an object in the OS). Wrapped in a class named, say, AutoResetEvent, you'd fashion one as a member (like you have done with Queue). When the writer thread starts, it simply does something like:

// assuming a member
AutoResetEvent writeEvent;

void writer()
                {
                while (running)
                    {
                     writeEvent.wait();

                     // check and write pending data
                    }
                }

Then, every function that writes to the Queue also signals the event with something like:

writeEvent.signal();

This means a write would "wake up" the writer thread from waiting.

This makes the writer thread quiet and light, but jumps into action when something is there to be written.

Also, the destructor of the owning object (Logger) must signal the event to "wake it" so it exits at conclusion.

The specific function names for "signal" and "wait" may differ depending on the class representing the event.

Contrast that with std::async. You call std::async every time you submit something to the log (providing a callable object for the function to be performed). That, in turn, operates once, on the string passed as a parameter. No loop required. Multiple calls to std::async "queue" behind each other, depending on how you fashion it.

However, there is some loss of control - it takes some study to use this while keeping the order of the text written by the logger in order (that is, maybe you still must use the Queue of strings, but the call to async merely empties the Queue, where any following calls might be "do nothing" calls, discovering an empty Queue - which is ok by design).

About locks, mutexes, and the Queue of strings


Generally it appears you have the idea about std::lock (and the related std::mutex). A thread will obtain the lock before continuing, and if the lock was already "owned" by another thread, will block and wait until the lock is released, synchronizing the operations among threads.

However, if you use a std::queue or std::deque for the string array you have this curious problem: all writing threads block while writing.

Think of it this way. The writer thread obtains a lock, then starts reading the strings in the Queue. It writes each one to the output.

Key point: it does this while holding the lock, making all other threads pushing strings wait for the write to complete.

It would operate as if you don't have a background thread.

You could lock/release lock to get just the next string, then write that string with the lock released. That "multiplexes" the writer, one string at a time.

Another approach would be to hold the Queue using a std::shared_ptr. When the writer comes to obtain the strings to write (there could be several pushed in quickly), the writer thread "takes" the entire Queue (while under lock). A fresh, empty Queue is left in place. The lock is released.

At that point, writer has the entire pending Queue to itself. All other threads will push strings into the fresh, empty Queue left for them, while the writer loops through and writes all the strings in the Queue it "took".

It's like hanging a container on a hook. Threads pushing to the log drops their strings into that container that is on the hook. When the writer starts writing, it takes the entire container, leaving a new empty container on the hook.

This "batches" the writing task, and when things get busy reduces collisions between the writer thread and all the pushing threads.

[barnack]

I did it following your suggestion of having two queues.
Also instead of letting the second thread perform one more pass to flush all remaining messages, i wait until it joins and then the destructor itself on the main thread takes care of flushing the last ones. After all i was waiting the second thread anyway, so no point in making it do that. Link to GitHub in the original topic, i'm open to critics.

[Niccolo]

That looks like progress.

Now, the critiques.

In Logger::~Logger(), I believe you must signal (notify_one) the "work_available" condition variable, so the writer would release immediately when nothing is pending.

On any infinite wait on "work_available" would probably just deadlock until a thread attempts to write something (which calls notify_one).

On Logger::log, I doubt you really need to check if queue was empty, or more importantly you probably don't need to conditionally signal "notify_one" only when it was empty. There is very little negative consequence since the signal isn't a queue, but a "traffic light".

There's a "two for one" point to be made about the raw pointers for queue. Modern C++ should avoid raw pointers unless there are important reasons, and I don't find an important reason to require raw pointers here. This could be done with shared_ptr trivially (though it is a choice in contrast to unique_ptr mainly motivated to avoid having to move rather than copy).

The point is that a smart pointer would eliminate the requirement to call delete explicitly, and while this situation is rather simple and obvious it is an important habit to insist upon for modern code. Smart pointers are one of the single most important aspects of modern C++.

That brings me to the second related point here. You could establish two queues, one "private" for writing, and the other "public" for the log input. That way the container isn't frequently being deleted and new ones created - they are just swapped (of course, the writer should clear the container). If your implementation of std::queue doesn't have "clear" (it doesn't appear to be guaranteed), switch to std::deque, which would drop in and bring "clear" to the interface.

...and consider a smart pointer to hold "thread"