Found similar questions: Is it safe to `delete this`? I know that it's unsafe to access member variable. Because this becomes a dangling pointer after delete. But what about stack variable？Clang ASAN does report error if member variable is accessed. But it does not report any problems about stack variable access.

IMHO. Stack is destroyed after the current execution thread is finished. So it is probably safe to access stack variable even if this is deleted. Is there any better solution in this scenario? Following is the test case.

#include <atomic>
#include <chrono>
#include <cstdlib>
#include <cstring>
#include <iostream>
#include <ratio>
#include <string>
#include <system_error>
#include <thread>
#include <vector>

std::mutex g_iostream_mutex;

class TestDeleteBase {
  int task_num_ = 0;
  std::atomic<int> counter_;

 public:
  virtual void Run() = 0;
  void Init(int task_num) {
    task_num_ = task_num;
    counter_.store(0, std::memory_order_relaxed);
  }
  void RunParallel() {
    int local = counter_;
    if (counter_.fetch_add(1, std::memory_order_acq_rel) == task_num_ - 1) {
      delete this;
      {
        std::lock_guard<std::mutex> guard(g_iostream_mutex);
        std::cout << std::this_thread::get_id() << " deleted this\n";
      }
      return;
    } else {
      {
        std::lock_guard<std::mutex> guard(g_iostream_mutex);
        std::cout << std::this_thread::get_id() << " not delete \n";
      }
      std::this_thread::sleep_for(std::chrono::seconds(1));
    }
    {
      std::lock_guard<std::mutex> guard(g_iostream_mutex);
      std::cout << std::this_thread::get_id() << " Access " << local << '\n';
      std::cout << std::this_thread::get_id() << " Still alive\n";
    }
  }
  virtual ~TestDeleteBase() {}
};

class TestDelete : public TestDeleteBase {
  void Run() override {}
};

int main(int argc, char* argv[]) {
  TestDeleteBase* obj = new TestDelete();
  obj->Init(5);
  std::vector<std::thread> threads;
  for (int i = 0; i < 5;   i) {
    threads.emplace_back(&TestDeleteBase::RunParallel, obj);
  }
  for (auto&& thread : threads) {
    thread.join();
  }
}

CodePudding user response：

The problem is having counter_ equal to task_num_ - 1 does not mean all threads are finished. It just mean that the fetch_add call has been executed by all threads here. The thing is the == in the expression counter_.fetch_add(1, std::memory_order_acq_rel) == task_num_ - 1 is parsed from left to right. Thus, the fetch_add must be executed before task_num_ - 1 so the threads must access to the task_num_ member attribute after the fetch_add (especially because of the memory_order_acq_rel memory ordering). The point is the attribute can be deleted because the last thread can delete the operation meanwhile (which is very unlikely but still possible).

Besides, local = counter_ do not guarantee to get the last value so the printing will likely be incorrect. You can extract the correct value by storing the result of fetch_add in local.

Object self destruction if generally a very bad idea because it is very bug prone as said in the question link. Even when you succeed to make the code work, the code is not easy to maintain: another developer might not consider this issue when modifying the code (or even you several month/years after). The best thing to do is to delay delete and the general solution to self destruction is to define an entity responsible for the deletion of the object. In this case, the main function can be responsible for that because it is the one doing the thread.join. All threads are guaranteed to be freed after the loop by design.