Using InterlockedIncrement to increment STL map's value field. Is it threadsafe?

Question

I have a map defined like this.

typedef std::map< tstring, unsigned int >  ErrorToCount_T;
ErrorToCount_T m_ErrorToSuppress;

I am using like it like this.

ErrorToCount_T::iterator itr = m_ErrorToSuppress.find( val );
if( itr != m_ErrorToSuppress.end())
{
    if( (itr->second) % m_LogFreq == 0)
        //Do something
    else
        //Do something else
    InterlockedIncrement( &itr->second);
}

I saw this and I understand that find is thread-safe. But I was thinking that InterlockedIncrement( &itr->second) will be threadsafe too? Is the above code thread safe. There are absolutely no inserts in this map in multithreaded environment.

Answer 1

If two threads execute the code with the same key (i.e. val), then the expressions marked with (1) and (2) in the following listing might be executed at the same time:

if( (itr->second) % m_LogFreq == 0) // (1)
    //Do something
else
    //Do something else
InterlockedIncrement( &itr->second); // (2)

In expression (1) the memory location itr->second is read, and in expression (2) it is written. That is called a data race, and the C++11 standard states, that the behaviour of the program is undefined if it contains data races.

As long as your compiler vendor doesn't give you additional guarantees about the memory model, you have to use operations that don't cause data races. Using C++11, it might look as follows:

using ErrorToCount = std::map<tstring, std::atomic<unsigned int>>;
ErrorToCount errorToSuppress;

ErrorToCount::iterator itr = errorToSuppress.find( val );
if( itr != errorToSuppress.end()) {
    if (itr->second.load(std::memory_order_seq_cst) % m_LogFreq == 0)
        //Do something
    else
        //Do something else
    itr->second.fetch_add(1, std::memory_order_seq_cst);
}