About shared_mutex and shared_ptr across multiple threads

Question

I implemented code such that multiple instances running on different threads reads other instances' data using reader-writer lock and shared_ptr. It seemed fine, but I am not 100% sure about that and I came up with some questions about usage of those.

Detail

I have multiple instances of a class called Chunk and each instance does some calculations in a dedicated thread. A chunk needs to read neighbour chunks' data as well as its own data, but it doesn't write neighbours' data, so reader-writer lock is used. Also, neighbours can be set at runtime. For example, I might want o set a different neighbour chunk at runtime, sometimes just nullptr. It is possible to delete a chunk at runtime, too. Raw pointers can be used but I thought shared_ptr and weak_ptr are better for this, in order to keep track of the lifetime. Own data in shared_ptr and neighbours' data in weak_ptr.

I provided a simpler version of my code below. ChunkData has data and a mutex for it. I use InitData for data initialization and DoWork function is called in a dedicated thread after that. other functions can be called from main thread. This seems to work, but I am not so confident. Especially, about use of shared_ptr across multiple threads.

1. What happens if a thread calls shared_ptr's reset() (in ctor and InitData) and other uses it with weak_ptr's lock (in DoWork)? Does this need a lock dataMutex or chunkMutex?

2. How about copy(in SetNeighbour)? Do I need locks for this as well?

I think other parts are ok, but please let me know if you find anything dangerous. Appreciate that.

By the way, I considered about storing shared_ptr of Chunk instead of ChunkData, but decided not to use this method because internal code, which I don't manage, has GC system and it can delete a pointer to Chunk when I don't expect it.

class Chunk
{
public:
    class ChunkData
    {
    public:

        shared_mutex dataMutex; // mutex to read/write data
        int* data;
        int size;

        ChunkData() : data(nullptr) { }

        ~ChunkData()
        {
            if (data)
            {
                delete[] data;
                data = nullptr;
            }
        }
    };

private:
    mutex chunkMutex;   // mutex to read/write member variables
    shared_ptr<ChunkData> chunkData;
    weak_ptr<ChunkData> neighbourChunkData;
    string result;

public:
    Chunk(string _name)
        : chunkData(make_shared<ChunkData>())
    {
    }

    ~Chunk()
    {
        EndProcess();
        unique_lock lock(chunkMutex);   // is this needed?
        chunkData.reset();
    }

    void InitData(int size)
    {
        ChunkData* NewData = new ChunkData();
        NewData->size = size;
        NewData->data = new int[size];

        {
            unique_lock lock(chunkMutex);   // is this needed?
            chunkData.reset(NewData);
            cout << "init chunk " << name << endl;
        }
    }

    // This is executed in other thread. e.g. thread t(&Chunk::DoWork, this);
    void DoWork()
    {
        lock_guard lock(chunkMutex); // we modify some members such as result(string) reading chunk data, so need this.
        if (chunkData)
        {
            shared_lock readLock(chunkData->dataMutex);
            if (chunkData->data)
            {
                // read chunkData->data[i] and modify some members such as result(string)
                for (int i = 0; i < chunkData->size; ++i)
                {
                    // Is this fine, or should I write data result outside of readLock scope?
                    result += to_string(chunkData->data[i]) + " ";
                }
            }
        }
        // does this work?
        if (shared_ptr<ChunkData> neighbour = neighbourChunkData.lock())
        {
            shared_lock readLock(neighbour->dataMutex);
            if (neighbour->data)
            {
                // read neighbour->data[i] and modify some members as above
            }
        }
    }

    shared_ptr<ChunkData> GetChunkData()
    {
        unique_lock lock(chunkMutex);
        return chunkData;
    }

    void SetNeighbour(Chunk* neighbourChunk)
    {
        if (neighbourChunk)
        {
            // safe?
            shared_ptr<ChunkData> newNeighbourData = neighbourChunk->GetChunkData();
            unique_lock lock(chunkMutex);   // lock for chunk properties
            {
                shared_lock readLock(newNeighbourData->dataMutex);  // not sure if this is needed.
                neighbourChunkData = newNeighbourData;
            }
        }
    }

    int GetDataAt(int index)
    {
        shared_lock readLock(chunkData->dataMutex);
        if (chunkData->data && 0 <= index && index < chunkData->size)
        {
            return chunkData->data[index];
        }
        return 0;
    }

    void SetDataAt(int index, int element)
    {
        unique_lock writeLock(chunkData->dataMutex);
        if (chunkData->data && 0 <= index && index < chunkData->size)
        {
            chunkData->data[index] = element;
        }
    }
};

Edit 1

I added more detail for DoWork function. Chunk data is read and chunk's member variables are edited in the function.

After Homer512's anwer, I came up with other questions.

A) In DoWork function I write a member variable inside a read lock. Should I only read data in a read lock scope and if I need to modify other data based on read data, do I have to do outside of the read lock? For example, copy the whole array to a local variable in a read lock, and modify other members outside of the read lock using the local.

B) I followed Homer512 and modifed GetDataAt/SetDataAt as below. I do read/write lock chunkData->dataMutex before unlocking chunkMutex. I also do this in DoWork function. Should I instead do locks separately? For example, make a local variable shared_ptr and set chunkData to it in a chunkMutex lock, unlock it, then lastly read/write lock that local variable's dataMutex and read/write data.

    int GetDataAt(int index)
    {
        lock_guard chunkLock(chunkMutex);
        shared_lock readLock(chunkData->dataMutex);
        if (chunkData->data && 0 <= index && index < chunkData->size)
        {
            return chunkData->data[index];
        }
        return 0;
    }

    void SetDataAt(int index, int element)
    {
        lock_guard chunkLock(chunkMutex);
        unique_lock writeLock(chunkData->dataMutex);
        if (chunkData->data && 0 <= index && index < chunkData->size)
        {
            chunkData->data[index] = element;
        }
    }

Answer 1

I have several remarks:

1. ~ChunkData: You could change your data member from int* to unique_ptr<int[]> to get the same result without an explicit destructor. Your code is correct though, just less convenient.

2. ~Chunk: I don't think you need a lock or call the reset method. By the time the destructor runs, by definition, no one should have a reference to the Chunk object. So the lock can never be contested. And reset is unnecessary because the shared_ptr destructor will handle that.

3. InitData: Yes, the lock is needed because InitData can race with DoWork. You could avoid this by moving InitData to the constructor but I assume there are reasons for this division. You could also change the shared_ptr to std::atomic<std::shared_ptr<ChunkData> > to avoid the lock.

4. It is more efficient to write InitData like this:

void InitData(int size)
{
    std::shared_ptr<ChunkData> NewData = std::make_shared<ChunkData>();
    NewData->size = size;
    NewData->data = new int[size]; // or std::make_unique<int[]>(size)
    {
        std::lock_guard<std::mutex> lock(chunkMutex);
        chunkData.swap(NewData);
    }
    // deletes old chunkData outside locked region if it was initialized before 
}

make_shared avoids an additional memory allocation for the reference counter. This also moves all allocations and deallocations out of the critical section.

5. DoWork: Your comment "ready chunkData->data[i] and modify some members". You only take a shared_lock but say that you modify members. Well, which is it, reading or writing? Or do you mean to say that you modify Chunk but not ChunkData, with Chunk being protected by its own mutex?

6. SetNeighbour: You need to lock both your own chunkMutex and the neighbour's. You should not lock both at the same time to avoid the dining philosopher's problem (though std::lock solves this).

    void SetNeighbour(Chunk* neighbourChunk)
    {
        if(! neighbourChunk)
            return;
        std::shared_ptr<ChunkData> newNeighbourData;
        {
            std::lock_guard<std::mutex> lock(neighbourChunk->chunkMutex);
            newNeighbourData = neighbourChunk->chunkData;
        }
        std::lock_guard<std::mutex> lock(this->chunkMutex);
        this->neighbourChunkData = newNeighbourData;
    }

7. GetDataAt and SetDataAt: You need to lock chunkMutex. Otherwise you might race with InitData. There is no need to use std::lock because the order of locks is never swapped around.

EDIT 1:

8. DoWork: The line if (shared_ptr<ChunkData> neighbour = neighbourChunkData.lock()) doesn't keep the neighbur alive. Move the variable declaration out of the if to keep the reference.

EDIT: Alternative design proposal

What I'm bothered with is that your DoWork may be unable to proceed if InitData is still running or waiting to run. How do you want to deal with this? I suggest you make it possible to wait until the work can be done. Something like this:

class Chunk
{
    std::mutex chunkMutex;
    std::shared_ptr<ChunkData> chunkData;
    std::weak_ptr<ChunkData> neighbourChunkData;
    std::condition_variable chunkSet;

    void waitForChunk(std::unique_lock<std::mutex>& lock)
    {
        while(! chunkData)
            chunkSet.wait(lock);
    }
public:
    // modified version of my code above
    void InitData(int size)
    {
        std::shared_ptr<ChunkData> NewData = std::make_shared<ChunkData>();
        NewData->size = size;
        NewData->data = new int[size]; // or std::make_unique<int[]>(size)
        {
            std::lock_guard<std::mutex> lock(chunkMutex);
            chunkData.swap(NewData);
        }
        chunkSet.notify_all();
    }
    void DoWork()
    {
        std::unique_lock<std::mutex> ownLock(chunkMutex);
        waitForChunk(lock); // blocks until other thread finishes InitData
        {
            shared_lock readLock(chunkData->dataMutex);
            ...
        }
        
        shared_ptr<ChunkData> neighbour = neighbourChunkData.lock();
        if(! neighbour)
            return;
        shared_lock readLock(neighbour->dataMutex);
        ...
    }
    void SetNeighbour(Chunk* neighbourChunk)
    {
        if(! neighbourChunk)
            return;
        shared_ptr<ChunkData> newNeighbourData;
        {
            std::unique_lock<std::mutex> lock(neighbourChunk->chunkMutex);
            neighbourChunk->waitForChunk(lock); // wait until neighbor has finished InitData
            newNeighbourData = neighbourChunk->chunkData;
        }
        std::lock_guard<std::mutex> ownLock(this->chunkMutex);
        this->neighbourChunkData = std::move(newNeighbourData);
    }
};

The downside to this is that you could deadlock if InitData is never called or if it failed with an exception. There are ways around this, like using an std::shared_future which knows that it is valid (set when InitData is scheduled) and whether it failed (records exception of associated promise or packaged_task).