Cleanup resources of base class before destructing derived class

user_966599 0
Viewed 182

I implemented an own Runnable to call threads:

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

class Runnable {
public:
    Runnable(): running_thread_(nullptr) {}

    void run() {
        if(running_thread_)
            return;

        running_thread_ = std::unique_ptr<std::thread>(new std::thread(&Runnable::run_thread, this));

    }

    virtual ~Runnable() {
        stop();
    }

    void stop() {
        if (running_thread_ && running_thread_->joinable()) {
            running_thread_->join();
        }
        running_thread_ = nullptr;
    }

protected:
    void run_thread()
    {
        std::cout << "a" << std::endl;
        start();
        std::cout << "b" << std::endl;
    }

    virtual bool start() = 0;

private:
    std::unique_ptr<std::thread> running_thread_;
};

class TestRunner : public Runnable {
public:
    virtual ~TestRunner() {
    };

protected:
    bool start() override {
        return true;
    }
};

int main() {
    auto testRunner = std::make_shared<TestRunner>();
    testRunner->run();
}

If I run the code I get following output:

/home/vagrant/tmp/clionTestProject/cmake-build-debug/clionTestProject 
a 
pure virtual method called 
terminate called without an active exception

Process finished with exit code 134 (interrupted by signal 6: SIGABRT)

It seems that start() is called after the destructor of TestRunner. The call of stop() on the destructor of Runnable() does not help as the destructor of TestRunner is executed before. At least I found that I can repair it by calling stop in the destructor of TestRunner:

    virtual ~TestRunner() {
        stop();
    };

Using this destructor everything works as expected. But I would not like to care in the derived classes about calling the stop() methods, all the thread handling shall be covered in Runnable. Do you have an idea how to solve this?

So, what I am searching: Is there any way to prevent from Runnable that the destructor of TestRunner is called before the thread is done?

c++11 c++
3 Answers

An alternative could be to not let TestRunner inherit from Runnable but to let Runnable have a TestRunner member variable. This avoids the problem completely.

I've removed the smart pointers here because I think they are only taking focus from the real problem that the derived class object may get destroyed under the feet of the thread. That is, before the virtual member functions gets called and also while the thread is running and working on member variables in the derived class.

#include <atomic>
#include <iostream>
#include <thread>
#include <type_traits>
#include <utility>

// A base class for TestRunner
class RunnerBase {
public:
    void stop() { terminate_ = true; }
    bool terminated() const { return terminate_; }

    virtual void operator()() = 0;

private:
    std::atomic<bool> terminate_ = false;
};

The Runnable constructor now forwards its arguments to the member variable (which is of type TestRunner in our case).

template<class T>
class Runnable final {
public:
    static_assert(std::is_base_of_v<RunnerBase, T>, "T must inherit from RunnerBase");

    template<class... Args>     // constructor forwarding to the runner
    Runnable(Args&&... args) : runner{std::forward<Args>(args)...} {}

    void run() {
        if(running_thread_.joinable()) return;
        running_thread_ = std::thread(&Runnable::run_thread, this);
    }

    ~Runnable() { stop(); }

    void stop() {        
        if (running_thread_.joinable()) {
            runner.stop();
            running_thread_.join();
        }
    }

protected:
    void run_thread() {
        std::cout << "a" << std::endl;
        runner();
        std::cout << "b" << std::endl;
    }

private:
    std::thread running_thread_;
    T runner;
};

TestRunner inherits from RunnerBase and can check terminated() if it's time to terminate.

#include <vector>

class TestRunner : public RunnerBase {
public:
    TestRunner(size_t data_size) : foo(data_size) {}

    void operator()() override {
        while(!terminated()) {
            // work on data that only exists in the derived class:
            for(auto& v : foo) ++v;            
            std::cout << '.';
        }
    }

private:    
    // Some data that the thread works on that wóuld get destroyed before
    // the base class destructor could call stop() if inheriting `Runnable`
    std::vector<int> foo; 
};

The creation is just slightly different.

int main() {
    Runnable<TestRunner> testRunner(1024U);
    testRunner.run();
}
user_75822470

One method piggy-backs on your use of std::shared_ptr<TestRunner>. I don't like it very much, but it works (until you decide to get rid of the shared pointer).

class Runnable : public std::enable_shared_from_this<Runnable>{
  ...
      running_thread_ = std::unique_ptr<std::thread>(new std::thread(&Runnable::run_thread, shared_from_this()));
  ...
  static void run_thread(std::shared_ptr<Runnable> s)
  {
      std::cout << "a" << std::endl;
      s->start();
      std::cout << "b" << std::endl;
  }
  ...

This guarantees that your Runnable object won't be destroyed while run_thread is executing.

user_7758060

The problem is that your main() method returns (and therefore your TestRunner object gets deleted) before run_thread() can execute in the other thread. Therefore, when run_thread() does get executed in the other thread, it's being called on an object that has already been partially destroyed (i.e. ~TestRunner() has already been called by the main thread), invoking Undefined Behavior so bad things happen.

If you add testRunner->stop() to the end of main(), you can avoid the problem, because that ensures that main() will not return (and therefore the TestRunner object will not be destroyed) until after the spawned thread has exited.

It would be nice to be able to rely on the stop() call in ~Runnable() to handle it for you automagically, but by the time ~Runnable() executes, it's already too late, your object's subclass-layers have already been destroyed, and your thread is already trying to use a broken object.

user_1319300
Related