What Is the Most Efficient Thread-Safe C++ Logger

Thread-safe queue for Logger thread in C

Your pseudocode could be made to work, but it's difficult in C and mutex synchronization is inefficient. In my experience it's always better to use existing, well-established, asynchronous inter-process communication mechanisms between threads like named pipes. Even then there are many pitfalls and when dependencies aren't a problem using something like ZeroMQ might be the best idea. Specifically something like this. I don't think having multiple threads is a good idea, as file you want to write to will be a contention point.

What is the most efficient way to make this code thread safe?

You could implement a configuration file version variable. When your program starts it is set to 0. The macro can hold a static int that is the last config version it saw. Then a simple atomic comparison between the last seen and the current config version will tell you if you need to do a full lock and re-call updateTraceCallback();.

That way, 99% of the time you'll only add an extra atomic op, or memory barrier or something simmilar, which is very cheap. 1% of the time, just do the full mutex thing, it shouldn't affect your performance in any noticeable way, if its only 1% of the time.

Edit:

Some .h file:

extern long trace_version;

Some .cpp file:

long trace_version = 0;

The macro:

#define TRACE(msg)                                                  
{
    static long __lastSeenVersion = -1;
    static TraceProc traceCallback = 0;
    if ( !traceCallback || __lastSeenVersion != trace_version )
        updateTraceCallback( &traceCallback, &__lastSeenVersion, __FILE__, __LINE__ );
    traceCallback( msg );
}

The functions for incrementing a version and updates:

static long oldVersionRefcount = 0;
static long curVersionRefCount = 0;

void updateTraceCallback( TraceProc *callback, long &version, const char *file, unsinged int lineno ) {
   if ( version != trace_version ) {
      if ( InterlockedDecrement( oldVersionRefcount ) == 0 ) {
        //....free resources.....
        //...no mutex needed, since no one is using this,,,
      }
      //....aquire mutex and do stuff....
      InterlockedIncrement( curVersionRefCount );
      *version = trace_version;
      //...release mutex...
   }
 }

 void setNewTraceCallback( TraceProc *callback ) {
    //...aquire mutex...
    trace_version++;  // No locks, mutexes or anything, this is atomic by itself.  
    while ( oldVersionRefcount != 0 ) { //..sleep? }
    InterlockedExchange( &oldVersionRefcount, curVersionRefCount );
    curVersionRefCount = 0;
    //.... and so on...
    //...release mutex...

Of course, this is very simplified, since if you need to upgrade the version and the oldVersionRefCount > 0, then you're in trouble; how to solve this is up to you, since it really depends on your problem. My guess is that in those situations, you could simply wait until the ref count is zero, since the amount of time that the ref count is incremented should be the time it takes to run the macro.

Shared file logging between threads in C++ 11

My First question and request would be to point out any bad practices / mistakes that I have overlooked (although the code works with VC 2015).

Subjective, but the code looks fine to me. Although you are not synchronizing threads (some std::mutex in logger would do the trick).

Also note that this:

std::thread threadPool[THREADS_NUM];

auto logger = std::make_shared<Logger>("test.log");

for (int i = 0; i < THREADS_NUM; ++i)
{
    threadPool[i] = std::thread(spawnThread, i, logger);
    threadPool[i].join();
}

is pointless. You create a thread, join it and then create a new one. I think this is what you are looking for:

std::vector<std::thread> threadPool;

auto logger = std::make_shared<Logger>("test.log");

// create all threads
for (int i = 0; i < THREADS_NUM; ++i)
    threadPool.emplace_back(spawnThread, i, logger);
// after all are created join them
for (auto& th: threadPool)
    th.join();

Now you create all threads and then wait for all of them. Not one by one.

Secondly and this is what is my main concern is that I'm not closing the file handle, and I'm not sure If that causes any issues. If it does when and how would be the most appropriate way to close it?

And when do you want to close it? After each write? That would be a redundant OS work with no real benefit. The file is supposed to be open through entire program's lifetime. Therefore there is no reason to close it manually at all. With graceful exit std::ofstream will call its destructor that closes the file. On non-graceful exit the os will close all remaining handles anyway.

Flushing a file's buffer (possibly after each write?) would be helpful though.

Lastly and correct me if I'm wrong I don't want to "pause" a thread while another thread is writing. I'm writing line by line each time. Is there any case that the output messes up at some point?

Yes, of course. You are not synchronizing writes to the file, the output might be garbage. You can actually easily check it yourself: spawn 10000 threads and run the code. It's very likely you will get a corrupted file.

There are many different synchronization mechanisms. But all of them are either lock-free or lock-based (or possibly a mix). Anyway a simple std::mutex (basic lock-based synchronization) in the logger class should be fine.

---

Related Topics