Thread safety in std::map of std::shared_ptr

Question

I know there are a lot of similar questions with answers around, but since I still don't understand this particular case, I decided to pose a question.

What I have is a map of shared_ptrs to a dynamically allocated array (MyVector). What I want is limited concurrent access without the need to lock. I know that the map per se is not thread safe, but I always thought what I'm doing here should be ok, which is:

I fill the map in a single threaded environment like that:

typedef shared_ptr<MyVector<float>> MyVectorPtr;

for (int i = 0; i < numElements; i++)
{
    content[i] = MyVectorPtr(new MyVector<float>(numRows));
}

After the initialization, I have one thread that reads from the elements and one that replaces what the shared_ptrs point to.

Thread 1:

for(auto i=content.begin();i!=content.end();i++)
{
    MyVectorPtr p(i->second);
    if (p)
    {
        memory_use+=sizeof(int) + sizeof(float) * p->number;
    }
}

Thread 2:

    for (auto itr=content.begin();content.end()!=itr;++itr)
    {
        itr->second.reset(new MyVector<float>(numRows));
    }

After a while I get either a seg fault or a double free in one of the two threads. Somehow not really surprisingly, but still I don't really get it.

The reasons why I thought this would work, are:

1. I don't add or remove any items of the map in the multi-threaded environment, so the iterators should always point to something valid.
2. I thought concurrently changing a single element of the map is fine as long as the operation is atomic.
3. I thought the operations I do on the shared_ptr (increment ref count, decrement ref count in Thread 1, reset in Thread 2) are atomic. SO Question

Obviously, either one ore more of my assumptions are wrong, or I'm not doing what I think I am. I think that reset actually is not thread safe, would std::atomic_exchange help?

Can someone release me? Thanks a lot!

If someone wants to try out, here is the full code example:

#include <stdio.h>
#include <iostream>
#include <string>
#include <map>
#include <unistd.h>
#include <pthread.h>


using namespace std;

template<class T>
class MyVector
{
public:
    MyVector(int length)
    : number(length)
    , array(new T[length])
    {
    }

    ~MyVector()
    {
        if (array != NULL)
        {
            delete[] array;
        }
        array = NULL;
    }

    int number;

private:
    T* array;
};

typedef shared_ptr<MyVector<float>> MyVectorPtr;


static map<int,MyVectorPtr> content;
const int numRows = 1000;
const int numElements = 10;

//pthread_mutex_t write_lock;

double get_cache_size_in_megabyte()
{
    double memory_use=0;
    //BlockingLockGuard guard(write_lock);
    for(auto i=content.begin();i!=content.end();i++)
    {
        MyVectorPtr p(i->second);
        if (p)
        {
            memory_use+=sizeof(int) + sizeof(float) * p->number;
        }
    }

    return memory_use/(1024.0*1024.0);

}


void* write_content(void*)
{
    while(true)
    {
        //BlockingLockGuard guard(write_lock);
        for (auto itr=content.begin();content.end()!=itr;++itr)
        {
            itr->second.reset(new MyVector<float>(numRows));
            cout << "one new written" <<endl;
        }

    }
    return NULL;
}

void* loop_size_checker(void*)
{
    while (true)
    {
        cout << get_cache_size_in_megabyte() << endl;;
    }
    return NULL;
}

int main(int argc, const char* argv[])
{
    for (int i = 0; i < numElements; i++)
    {
        content[i] = MyVectorPtr(new MyVector<float>(numRows));
    }

    pthread_attr_t attr;
    pthread_attr_init(&attr) ;
    pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
    pthread_attr_setscope(&attr, PTHREAD_SCOPE_SYSTEM);

    pthread_t *grid_proc3 = new pthread_t;
    pthread_create(grid_proc3, &attr, &loop_size_checker,NULL);

    pthread_t *grid_proc = new pthread_t;
    pthread_create(grid_proc, &attr, &write_content,(void*)NULL);


    // to keep alive and avoid content being deleted
    sleep(10000);
}

Answer 1

I thought concurrently changing a single element of the map is fine as long as the operation is atomic.

Changing the element in a map is not atomic unless you have a atomic type like std::atomic.

I thought the operations I do on the shared_ptr (increment ref count, decrement ref count in Thread 1, reset in Thread 2) are atomic.

That is correct. Unfortunately you are also changing the underlying pointer. That pointer is not atomic. Since it is not atomic you need synchronization.

One thing you can do though is use the atomic free functions that are introduced with std::shared_ptr. This will let you avoid having to use a mutex.

Answer 2

TL;DR;

Changing std::map isn't thread safe, while using std::shared_ptr regarding additional references is.

You should protect accessing your map regarding read/write operations using an appropriate synchronization mechanism, like e.g. a std::mutex.

Also if the state of an instance referenced by the std::shared_ptr should change, it needs to be protected against data races if it's accessed from concurrent threads.

---

BTW, the MyVector you are showing is a way too naive implementation.

Answer 3

Lets expand MyVectorPtr p(i->second); which is running on thread-1:

The constructor called for this is:

template< class Y > 
shared_ptr( const shared_ptr<Y>& r ) = default;

Which probably boils down to 2 assignments of the underlying shared pointer and the reference count.

It may very well happen that thread 2 would delete the shared pointer while in thread-1 the pointer is being assigned to p. The underlying pointer stored inside shared_ptr is not atomic.

Thus, you usage of std::shared_ptr is not thread safe. It is thread safe as long as you do not update or modify the underlying pointer.