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?

Answer 1

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));
}

Answer 2

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;
}