std::thread move problem (use member function)

Question

1. Class MyTask encapsulate thread with some task data ("id" for example).
2. Class MyTaskM is movable cause want to organize them in container.
3. When threads run into accessing member data, Segmentation fault occurred. Don't know why can't access member data.

• specs:
  • g++.exe (Rev2, Built by MSYS2 project) 13.2.0
  • c++ standard 20

just want to know how and why? ty :)

#include <thread>
#include <string>
#include <iostream>
#include <format>
#include <utility>
#include <vector>
#include <memory>

class MyTask {
 public:
  explicit MyTask(std::string id)
      : id_(std::move(id)), 
        thread_(&MyTask::Process, this) {}

  ~MyTask() {
    if (thread_.joinable())
      thread_.join();
  }

  void Process() {
    std::cout << std::format("task {} running ...\n", id_);
    std::this_thread::sleep_for(std::chrono::seconds(3));
    std::cout << std::format("task {} finished ...\n", id_);
  }

 private:
  std::string id_;
  std::thread thread_;
};

void ThreadPtrContainerTest() {
  std::vector<std::unique_ptr<MyTask>> tasks;
  for (int i = 1; i <= 3; ++i) {
    auto task_id = std::to_string(i);
    tasks.emplace_back(std::make_unique<MyTask>(task_id));
  }
}

class MyTaskM {
 public:
  explicit MyTaskM(std::string id)
      : id_(std::move(id)), 
        thread_(&MyTaskM::Process, this) {}

  MyTaskM(MyTaskM &&rhs) noexcept
      : id_(std::move(rhs.id_)),
        thread_(std::move(rhs.thread_)) {}

  MyTaskM &operator=(MyTaskM &&rhs) noexcept {
    if (this != &rhs) {
      id_ = std::move(rhs.id_);
      thread_ = std::move(rhs.thread_);
    }
    return *this;
  }

  ~MyTaskM() {
    if (thread_.joinable())
      thread_.join();
  }

  void Process() {
    std::cout << std::format("task {} running ...\n", id_);
    std::this_thread::sleep_for(std::chrono::seconds(3));
    std::cout << std::format("task {} finished ...\n", id_);
  }

 private:
  std::string id_;
  std::thread thread_;
};

void ThreadContainerTest() {
  std::vector<MyTaskM> tasks;
  for (int i = 1; i <= 3; ++i) {
    auto task_id = std::to_string(i);
    tasks.emplace_back(task_id);
  }
}

int main() {
  ThreadContainerTest();
  /*
   * run into void Process():
   * std::cout << std::format("task {} running ...\n", id_);
   * Error: SIGSEGV (Segmentation fault)
   *
   */

  ThreadPtrContainerTest();
  /*
   * work fine
   */
  return 0;
}

Answer 1

Your threads are bound to the original object that created them. If you move that object, the thread is still bound to the old one. Not only that, but you're also moving without sychronization. Both of these things are bad. You need to modify your design.

The main problem is here:

std::vector<MyTaskM> tasks;
for (int i = 1; i <= 3; ++i) {
    auto task_id = std::to_string(i);
    tasks.emplace_back(task_id);
}

As you emplace new tasks in the vector, the vector's storage must grow. When it grows, all of its existing contents will be moved into newly allocated storage. This is a problem, because you have threads already executing on objects residing at the old storage location.

The simplest fix for this is to reserve enough storage in the vector before creating your tasks. That will ensure that emplace_back does not grow the vector past its initial capacity and so the objects will not be moved:

std::vector<MyTaskM> tasks;
tasks.reserve(3);
for (int i = 1; i <= 3; ++i) {
    auto task_id = std::to_string(i);
    tasks.emplace_back(task_id);
}

This is still not very pretty. The fact that you need to make TaskM moveable, when moving it will result in undefined behavior, is just a bad idea. Your solution using std::unique_ptr is better than this. It might even be an idea to explicitly delete the move constructor to make it clear this class should never be moved.

Consider decoupling your tasks from the threading logic, and maybe read about thread pools while you're at it. A thread pool is essentially a collection of threads kept on standby, ready to execute tasks assigned to them. It's a more flexible and scaleable way to run tasks in parallel.