Guarding against vtable data race in derived destructor

Question

Suppose I have the following code

#include <thread>
#include <iostream>
#include <atomic>

struct FooBase {
    void start(){
        run_condition_ = true;
        t_ = std::thread([this](){
            thread_handler();
        });
    }

    virtual ~FooBase(){
        run_condition_ = false;
        if(t_.joinable())
            t_.join();
    }

protected:
    virtual void thread_handler() = 0;
    std::atomic_bool run_condition_{false};
private:
    std::thread t_;
};


struct Foo : FooBase {
    void thread_handler() override {
        while(run_condition_){
            std::cout << "Foo derived thread.." << std::endl;
        }
    }
};


int main(){
    Foo f;
    f.start();

    getchar();

    return 0;
}

Here I think because the destructor of the derived class Foo is called before FooBase the thread_handler vtable lookup happens in the base class IF the thread has not yet joined (still running) when the destructor of Foo is done. Since FooBase::thread_handler is pure virtual I am essentially guranteed a sigabort.

How do I guard against this? I hack my way through by not having thread_handler as pure virtual

virtual void thread_handler(){}

But I am lost as to how I can guard against this in the baseclass itself, I can implement a join_thread interface in the base class and call this from every derived class, but this seems cumbersome.

Answer 1

There's two issues here, neither of which match precisely what you described.

1. Your thread only gets stopped in ~FooBase(). This means that if Foo::thread_handler ever reads or writes to any of its members, they will get destroyed out from under it before the thread is stopped.

2. It you get to the destructor fast enough, it's possible that start() won't have actually invoked thread_handler() on the new thread by the time Foo gets destroyed - which will lead to the pure virtual call.

Either way, you need to ensure that by the time Foo is destroyed, anything related to thread_handler is done. This implies that every derived class from FooBase has to have, in its destructor:

run_condition_ = false;
if (t_.joinable()) {
    t_join();
}

Setting aside that this directly doesn't work because t_ is private (you could wrap that into a protected stop()), it's an awkward design if all of your derived classes need to do something special just to work. You could instead put FooBase into its own class that just takes an arbitrary callable as an argument:

class joining_thread {
public:
    joining_thread() = default;
    ~joining_thread() { stop(); }

    bool running() const { return run_condition_.load(); }

    template <typename... Args>
    void start(Args&&... args) {
        run_condition_ = true;
        t_ = std::thread(std::forward<Args>(args)...);
    }

    void stop() {
        run_condition_ = false;
        if (t_.joinable()) t_.join();
    }
private:
    std::thread t_;
    std::atomic_bool run_condition_{false};
};

And then your Foo can just have that as a member:

class Foo {
public:
    void start() {
        t_.start([this]{
            while (t_.running()) { ... }
        });
    }

private:
    // just make me the last member, so it's destroyed first
    joining_thread t_;
};

That's still a little awkward with the whole running() thing, but hopefully the idea makes sense.