Moving objects containing a running std::thread member

Question

How to correctly use the move semantic with running thread in object?

Sample:

#include <iostream>
#include <thread>
#include <vector>

struct A {
    std::string v_;
    std::thread t_;

    void start() {
        t_ = std::thread(&A::threadProc, this);
    }

    void threadProc() {
        for(;;) {
            std::cout << "foo-" << v_ << '\n';
            std::this_thread::sleep_for(std::chrono::seconds(5));
        }
    }
};

int main() {
    A m;
    {
        A a;
        a.v_ = "bar";
        a.start();
        m = std::move(a);
    }
    std::cout << "v_ = " << m.v_ << '\n'; /* stdout is 'v_ = bar' as expected    */
                                          /* but v_ in thread proc was destroyed */
                                          /* stdout in thread proc is 'foo-'     */
    m.t_.join();
    return 0;
}

I want to use class members after moving, but when I go out scope, class members are destroyed and std::thread is moved into new object as expected but it starting use destroyed members.

It seems to me because of using this pointer in thread initialization.

What is best practice in this case?

Answer 1

As written, it's not going to work. After moving, the thread m.t_ refers to a thread which is still running a.threadProc(). That will be attempting to print a.v_.

There are even two problems with the snippet: not only is a.v_ moved from (so its value is unspecified), but it's also about to be destroyed in another thread, and that destruction is not sequenced-after its use.

Since the object needs to stay alive long enough, with a non-trivial lifetime due to the thread, you'll need to get it off the stack and out of the vector. Instead, use std::shared_ptr to manage the lifetime. You will probably need to pass that shared_ptr to the thread, to avoid a race condition where the object might expire before the thread starts running. You can't rely on std:shared_from_this.

Answer 2

What is best practice in this case?

The best practice is to delete the move constructor and move assignment operator to prevent this from happening. Your object requires that this never changes, and you're getting undefined behavior because in this case the object was whipped out from beneath your thread and subsequently destroyed.

If, for whatever reason preventing moves goes against your design requirements, then there are a some common approaches that would make the most sense to anybody fortunate enough to be reading and maintaining your code.

1. Use the pimpl idiom to create an internal object dynamically which can move with the outer object. The outer object is movable, but the inner object is not. The thread is bound to that object, and anything the thread needs access to is also within that object. In your case, you would basically take your structure as it is and wrap it. The basic idea is something like:

   class MovableA
   {
   public:
       MovableA() : a_(std::make_unique<A>()) {}
       void start() { a_->start(); }
       A & a() const { return *a_; }
   private:
       std::unique_ptr<A> a_;
   };
   

   The benefit of this approach is that you can move MoveableA without needing to synchronize with the running thread.

2. Abandon the notion of using stack allocation, and just allocate A dynamically. This has the same benefit as option 1, and is simpler because you're not having to wrap your class in anything or provide accessors.

   std::unique_ptr<A> m;
   {
       auto a = std::make_unique<A>();
       a->v_ = "bar";
       a->start();
       m = std::move(a);
   }
   std::cout << "v_ = " << m->v_ << '\n';
   m->t_.join();
   

I started writing an option 3 that avoids dynamic allocation and instead binds a 'floating' version of this to a std::reference_wrapper but I felt I'd get it wrong without thinking about it a lot, and it seemed hacky and horrible anyway.

The bottom line is if you want to keep the object outside your thread and use it in the thread, the best practice is to use dynamic allocation.