Is mutex+atomic necessary to make this code thread safe, or is mutex enough?

I have some doubts wether mutexes are enough to ensure thread safety of the following code example or if atomics are required. In short question is: Would making idxActive a regular int make this code thread unsafe? Or is code even with atomics thread unsafe? :( If it is important, I'm on 32 bit x86, linux, gcc 4.6. Of course I presume that 32 or 64 bit makes no diff, but if there is any diff between 32 and 64 bit I would like to know.

#include <memory>
#include <boost/thread/thread.hpp>
#include <string>
#include <vector>
#include <atomic>
#include <boost/thread/mutex.hpp>
using namespace std;
using namespace boost;
static const int N_DATA=2;
class Logger
{
    vector<string> data[N_DATA];
    atomic<int> idxActive;
    mutex addMutex;
    mutex printMutex;
public:
    Logger()
    {
        idxActive=0;
        for (auto& elem: data)
            elem.reserve(1024);
    }
private:
    void switchDataUsed()
    {
        mutex::scoped_lock sl(addMutex);
        idxActive.store( (idxActive.load()+1)%N_DATA );
    }
public:
    void addLog(const string& str)
    {
        mutex::scoped_lock sl(addMutex);
        data[idxActive.load()].push_back(str);
    }
    void printCurrent()
    {
        mutex::scoped_lock sl(printMutex);
        switchDataUsed();
        auto idxOld=(idxActive.load()+N_DATA-1)%N_DATA; //modulo -1
        for (auto& elem:data[idxOld])
            cout<<elem<<endl;
        data[idxOld].clear();
    }
};
int main()
{
    Logger log;
    log.a开发者_高级运维ddLog(string("Hi"));
    log.addLog(string("world"));
    log.printCurrent();
    log.addLog(string("Hi"));
    log.addLog(string("again"));
    log.printCurrent();
    return 0;
}

You do not need to use atomic variables if all accesses to those variables are protected by a mutex. This is the case in your code, as all public member functions lock addMutex on entry. Therefore addIndex can be a plain int and everything will still work fine. The mutex locking and unlocking ensures that the correct values become visible to other threads in the right order.

std::atomic<> allows concurrent access outside the protection of a mutex, ensuring that threads see correct values of the variable, even in the face of concurrent modifications. If you stick to the default memory ordering it also ensures that each thread reads the latest value of the variable. std::atomic<> can be used to write thread-safe algorithms without mutexes, but is not required if all accesses are protected by the same mutex.

Important Update:

I just noticed that you're using two mutexes: one for addLog and one for printCurrent. In this case, you do need idxActive to be atomic, because the separate mutexes do not provide any synchronization between them.

atomic is not directly related to thread safety. It just ensures that the operations on it are exactly what it says: atomic. Even if all your operations were atomic your code would not necessarily be thread safe.

In your case, the code should be safe. Only 1 thread at a time can enter printCurrent(). While this function is executed other threads can call addLog() (but also only 1 at a time). Depending whether or not switchCurrent has already been executed those entries will make it into the current log or they won't but none will be entered while iterating over it. Only 1 thread at a time can enter addLog which shares its mutex with switchCurrent so they cannot be executed at the same time.

This will be the case even if you make idxActive a simple int Mh, the C++ memory model only deals with single-threaded code — so I'm not too sure if theoretically it could break it. I think if you make idxActive volatile (basically disallowing any load/store optimization on it at all) that it will be ok for all practical purposes. Alternatively you could remove the mutex from switchCurrent but then you need to keep idxActive atomic.

As improvement I would let switchCurrent return the old index instead of recalculating it.