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

  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) {
    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()) {

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.

  • \$\begingroup\$ Are you programming embedded computers or micro-controllers? If not, why are you using volatile? \$\endgroup\$
    – Mast
    Jun 12, 2018 at 9:01
  • \$\begingroup\$ I am not programming embedded computers etc, but I am working with RDMA where the NIC is issuing DMA writes to local memory on behalf of remote nodes. I am using volatile to guard against compiler optimizations (O3 mostly). Sometimes the compiler determines that a given memory region is not being changed by the program and optimizes it incorrectly. See the accepted answer of this post: stackoverflow.com/questions/4437527/…. Here, I have thread_shutdown as volatile for a similar reason. It is read by one thread and modified by the other. \$\endgroup\$
    – Sagar Jha
    Jun 12, 2018 at 20:55
  • 1
    \$\begingroup\$ I'm afraid this question does not match what this site is about. Code Review is about improving existing, working code. The example code that you have posted is not reviewable in this form because it leaves us guessing at your intentions. Unlike Stack Overflow, Code Review needs to look at concrete code in a real context. Please see Why is hypothetical example code off-topic for CR? \$\endgroup\$ Jun 19, 2018 at 13:04
  • \$\begingroup\$ Sorry, I didn't know that I cannot post hypothetical code-examples. I was unsure whether to post on Stack Overflow or here. \$\endgroup\$
    – Sagar Jha
    Jun 19, 2018 at 15:38

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

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() :
    // 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)
    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.

  • 2
    \$\begingroup\$ Note: volatile is what the MS compiler recommended in the old days (C++03). This of course has been replaced with real thread language semantics and atomic. \$\endgroup\$ Jun 12, 2018 at 19:35

Not the answer you're looking for? Browse other questions tagged or ask your own question.