Graceful thread shutdown with std::move [closed]

Question

I have a thread as part of a class that access the class's data members. The class has a move constructor that moves some of the old class's data members into the new object being constructed. Before moving the class members of the old object, I need to shutdown its thread. I do this by calling thread_shutdown in the move constructor. I would like to know if this is the most elegant way to achieve thread-safety.

#include <chrono>
#include <iostream>
#include <memory>
#include <thread>

class B {
public:
  int x;
};

class A {
  std::unique_ptr<B> b_ptr;
  volatile bool thread_shutdown = false;
  std::thread t;
  void run() {
    while (!thread_shutdown) {
      int x = b_ptr->x;
    } 
  }

public:
  A() : b_ptr(std::make_unique<B>()), t(std::thread(&A::run, this)) {}
  // A(A &&a) : b_ptr(std::move(a.b_ptr)), t(std::thread(&A::run, this)) {}
  A(A && a) {
    a.shutdown_thread();
    b_ptr = std::move(a.b_ptr);
    t = std::thread(&A::run, this);
  }
  ~A() { shutdown_thread(); }
  void shutdown_thread() {
    thread_shutdown = true;
    if (t.joinable()) {
      t.join();
    }
  }
};

int main() {
  std::unique_ptr<A> a_ptr = std::make_unique<A>();
  a_ptr = std::make_unique<A>(std::move(*a_ptr)); 
}

If I replace the move constructor definition by the commented line in the code, I get a segmentation fault in the second line of main because the old thread is trying to access b_ptr->x, but b_ptr is NULL as it has been moved to the new object's b_ptr. Thus, I cannot rely on the destructor of class A to shutdown the thread.

So, the question is: What is the best way to handle thread shutdown before some of the objects accessed by it are moved? I don't like having to call shutdown_thread in the move constructor.

Answer 1

volatile bool thread_shutdown = false;

This is definitely not correct. volatile is not for thread safety. What you want here is atomic<bool>.

Other than that, I'd say everything else looks technically kosher. The thing is, the semantics of your move operations will be weird now. For example, if I write:

auto func(A a) {}

auto a = A{};
func(std::move(a));

What I expect to happen is that a is cheaply moved into the function argument (and thus, into the function). Instead what happens here is that I've triggered a thread death plus the creation of a whole new thread. So much for moves being cheap - in fact, copying might be cheaper because it would create a new thread, but not destroy the old one.

If you want to define a constructor that steals data from another object, and creates an entirely new thread to use that data in, that's fine. But using the move constructor for that seems unwise. This is probably a job for a tagged constructor:

A() :
    b_ptr{std::make_unique<B>()}
{
    // I wouldn't rely on order of data members to guarantee the thread
    // is started after the data is set.
    t = std::thread{&A::run, this};
}

A(steal_data_t, A&& a)
{
    a.shutdown_thread();
    b_ptr = std::move(a.b_ptr());
    t = std::thread{&A::run, this};
}

A(A&&) noexcept = default; // well, you'd need to handle moving the atomic, I believe

// Usage:
auto a1 = A{};
auto a2 = std::move(a1); // this invalidates a1, as usual
auto a3 = A{steal_data, std::move(a2)}; // this invalidates a2, as expected

You could avoid some repetition with private helper functions.