Move vector between threads

Question

I have a thread that continuously collects data items with a public interface like this:

class MyThread {
public:
    class Item {
        // ...
    };

    startup();
    shutdown();

    bool hasItems() const;

    // retrieve collected items
    std::vector<Item>&& items();
private:
    std::mutex itemMutex;
    std::vector<Item> currentItems;
};

Retrieving the items should also clear the thread's item list. I return an rvalue so that the move constructor is invoked on the calling side. Of course, retrieving the items should be thread-safe, so the implementation looks like this:

std::vector<MyThread::Item>&& MyThread::items() {
    std::lock_guard<std::mutex> lock(itemMutex);
    return std::move(currentItems);
}

I think that the lock is released too early here: the function returns the rvalue'd vector, but a move constructor hasn't necessarily be called with it when the std::lock_guard is destroyed and releases the mutex. So from my understanding, this isn't thread-safe. Am I right? How can I make it thread-safe?

If you change your function to return a `std::vector` rather than an rvalue reference then there is no problem. — Simple, Jan 22 '14 at 11:59

Sean · Accepted Answer · 2014-01-22T11:11:46.487

9

You're right, it isn't threadsafe as the move will happen after the method returns, at which point the lock will have been released. To fix it just move the vector into a local variable and return that :

std::vector<MyThread::Item> MyThread::items() 
{
    std::lock_guard<std::mutex> lock(itemMutex);
    return std::vector<MyThread::Item>(std::move(currentItems));
}

edited Jan 22 '14 at 11:11

answered Jan 22 '14 at 10:58

Sean

60,939
11
97
136

Is the returned value still an rvalue that will be moved when I call `std::vector list (myThreadInstance.items());`? To me, it looks like this would invoke a deep copy. – flyx Jan 22 '14 at 11:08
When you assign it to a local (`std::vector items=items()`) it will be an rvalue. – Sean Jan 22 '14 at 11:11
I do believe, you don't need to explicitly use the std::move(). If there is a move-ctor, it will be used anyway when returning the value (_currentItems_), so simply: `return currentItems;` should suffice. – CouchDeveloper Jan 22 '14 at 11:15
3

@CouchDeveloper `return currentItems;` only works if `currentItems` is a function-local variable. Here it is a data member so the `std::move` is required. – Simple Jan 22 '14 at 11:59

score 0 · Answer 2 · edited May 23 '17 at 12:21

You have to keep in mind that currentItem is in "partially formed" state after it's moved from (see What lasts after using std::move c++11). In one of the answers there there's an example how to remedy that problem:

std::vector<MyThread::Item> MyThread::items() 
{
    std::lock_guard<std::mutex> lock(itemMutex);
    auto tmp =  std::move(currentItems);
    currentItems.clear();
    return tmp;
}

I would rather go for the most straightforward solution and let the compiler do the heavy work:

std::vector<MyThread::Item> MyThread::items() 
{
    std::lock_guard<std::mutex> lock(itemMutex);
    std::vector<MyThread::Item> tmp;
    tmp.swap(currentItems);
    return tmp;
}

Move vector between threads

2 Answers2