pending std::future get of std::async with shared_from_this argument blocks destruction of this

Question

I wanted to create a class that would represent a task that can be started running asynchronously and will run continuously (effectively in a detached thread) until a stop signal is received. The usage for the sake of this question would look like this:

auto task = std::make_shared<Task>();
task->start(); // starts the task running asynchronously
... after some time passes ...
task->stop(); // signals to stop the task
task->future.get(); // waits for task to stop running and return its result

However, a key feature of this Task class is that I cannot guarantee that the future will be waited/got... i.e. the last line may not get called before the shared pointer is destroyed.

A stripped-down toy version of the class I wrote is as follows (please ignore that everything is in public, this is just for this example's simplicity):

class MyClass : public std::enable_shared_from_this<MyClass> {
public:
    ~MyClass() { std::cout << "Destructor called" << std::endl; }

    void start() {
        future = std::async(std::launch::async, &MyClass::method, this->shared_from_this());
    }
    void stop() { m_stop = true; }

    void method() {
        std::cout << "running" << std::endl;
        do {
            std::this_thread::sleep_for(std::chrono::seconds(1));
        } while(m_stop == false);
        std::cout << "stopped" << std::endl;
        return;
    }
    std::future<void> future;
    std::atomic<bool> m_stop = false;

};

However, I discovered an undesirable feature of this code: if instead of get on the future, I just wait (e.g. if I don't care about the result of method, which in this case is a void anyway), then when task is deleted, the instance doesn't get destroyed.

I.e. doing task->future.get() gives:

running
stopped
Destructor called

But task->future.wait() gives:

running
stopped

From reading answer to What is the lifetime of the arguments of std::async? I believe the problem here is the this->shared_from_this() argument to std::async won't be destroyed until the future from the async has been made invalid (through get or destruction or otherwise). So this shared_ptr is keeping the class instance alive.

---

Solution Attempt 1:

Replace the line in start with:

future  = std::async(std::launch::async, [this]() {
            return this->shared_from_this()->method();
        });

This ensures shared_ptr it creates is destroyed when the method completes, but I have been worried that there's nothing to stop this being destroyed between the time of it being captured by the lambda capture (which happens at this line, correct?) and the time the lambda is executed in the new thread. Is this a real possibility?

---

Solution Attempt 2:

To protect the this (task) being destroyed before the lambda function runs, I add another member variable std::shared_ptr<MyClass> myself then my start method can look like this:

myself = this->shared_from_this();
future  = std::async(std::launch::async, [this]() {
            auto my_ptr = std::move(this->myself);
            return myself->method();
});

Here the idea is that myself will ensure that if I delete the task shared_ptr, I don't destroy the class. Then inside the lambda, the shared_ptr is transferred to the local my_ptr variable, which is destroyed on exit.

---

Are there issues with this solution, or have I overlooked a much cleaner way of achieving the sort functionality I'm after?

Thanks!

Answer 1

Solution attempt 2 I found in some scenarios would generate a deadlock exception. This appears to come from the async thread simultaneously trying to destroy the future (by destroying the instance of the class) while also trying to set the value of the future.

---

Solution attempt 3 - this seems to pass all my tests so far:

myself = this->shared_from_this();
std::promise<void> p;
future = p.get_future();
std::thread([this](std::promise<void>&& p) {
            p.set_value_at_thread_exit( myself->method() );
            myself.reset();
}, std::move(p)).detach();

The logic here is that it is safe to destroy myself (by resetting the shared pointer) once the method call is finished - its safe to delete the future of a promise before the promise has set its value. No deadlock occurs because the future is destroyed before the promise tries to transfer a value.

Any comments on this solution or potentially neater alternatives would be welcome. In particular it would be good to know if there are issues I've overlooked.

Answer 2

I would suggest one of the following solutions:

Solution 1, Use std::async with this instead of shared_from_this:

class MyClass /*: public std::enable_shared_from_this<MyClass> not needed here */ {
public:
    ~MyClass() { std::cout << "Destructor called" << std::endl; }

    void start() {
        future = std::async(std::launch::async, &MyClass::method, this);
    }
    void stop() { m_stop = true; }

    void method() {
        std::cout << "running" << std::endl;
        do {
            std::this_thread::sleep_for(std::chrono::seconds(1));
        } while(m_stop == false);
        std::cout << "stopped" << std::endl;
        return;
    }

    std::atomic<bool> m_stop = false;
    std::future<void> future; // IMPORTANT: future constructed last, destroyed first
};

This solution would work even if not calling wait or get on the future because the destructor of a future returned by std::async blocks until the termination of the task. It is important to construct the future last, so that it is destroyed (and thus blocks) before all other members are destroyed. If this is too risky, use solution 3 instead.

---

Solution 2, Use a detached thread like you did:

 void start() {
    std::promise<void> p;
    future = p.get_future();
    std::thread(
        [m = this->shared_from_this()](std::promise<void>&& p) {
          m->method();
          p.set_value();
        },
        std::move(p))
        .detach();
  }

One drawback of this solution: If you have many instances of MyClass you will create a lot of threads maybe resulting in contention. So a better option would be to use a thread pool instead of a single thread per object.

---

Solution 3, Separate the executable from the task class e.g:

class ExeClass {
 public:
  ~ExeClass() { std::cout << "Destructor of ExeClass" << std::endl; }
  void method() {
    std::cout << "running" << std::endl;
    do {
      std::this_thread::sleep_for(std::chrono::seconds(1));
    } while (m_stop == false);
    std::cout << "stopped" << std::endl;
    return;
  }

  std::atomic<bool> m_stop = false;
};

class MyClass {
 public:
  ~MyClass() { std::cout << "Destructor of MyClass" << std::endl; }

  void start() {
    future = std::async(std::launch::async, &ExeClass::method, exe);
  }
  void stop() { exe->m_stop = true; }

  std::shared_ptr<ExeClass> exe = std::make_shared<ExeClass>();
  std::future<void> future;
};

Like solution 1 this would block when the future is destroyed, however you don't need to take care of the order of construction and destruction. IMO this is the cleanest option.