How to design a base class that needs to join a thread in its destructor, which operates on the same class instance?

Question

I just got a juicy race condition. Consider the following classes:

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

class A
{
    std::thread th;
    std::atomic_bool stop = false;

  public:
    A() = default;
    A(const A &) = delete;
    A &operator=(const A &) = delete;

    ~A()
    {
        stop.store(true);
        th.join();
    }

    virtual void Step() = 0;

    void Start()
    {
        th = std::thread([this]
        {
            while (true)
            {
                if (stop.load())
                    return;

                // Just to help reproduce the race condition.
                std::this_thread::sleep_for(std::chrono::milliseconds(50));

                Step();
            }
        });
    }
};

struct B : A
{
    void Step() override
    {
        std::cout << "Step!" << std::endl;
    }
};

int main()
{
    B b;

    b.Start();

    // Just to help reproduce the race condition.
    std::this_thread::sleep_for(std::chrono::milliseconds(110));
}

This crashes due to a pure virtual function call (if we ignore the data race UB).

Right before we enter the body of ~A(), before we can stop the thread, the vtable pointer is changed to point to A's vtable, so the next call of Step() crashes, now being a pure virtual call.

This can be fixed by stopping the thread in ~B(), but that means every derived class must remember to do it, or bad things will happen.

I wish I could design A to take care of this by itself. Is it possible?

Bomar

Nope, this cannot be done. You're running into core fundamental principles of how C++ objects work. Been there, done that. Either the class hierarchy needs to be redesigned, or every derived class must be responsieble for invoking a base class's method (that stops the execution thread) in its destructor (and the base class method needs to be smart about being invoked multiple times). – Massey

Do you need inheritance? i.e move virtual void Step() from A to an interface IJob and take a std::unique<IJob>. Alternatively, use CRTP... – Completion

@Completion IJob sounds like a good idea. I considered CRTP, but I think it's going to suffer the same problem? – Bomar

Indeed, CRTP shouldn't solve the issue. – Completion

How about stub the callback/function instead of captureing this in Start()? Since Step() works in destructor, it should be stateless? – Stagg

If you add to your program void A::Step() {}, then there will not be crash due to pure virtual function call. – Wiegand

@Wiegand Yep, but there would still a data race UB on the vtable pointer,. – Bomar

@LouisGo What do you mean by a stub? Regarding making Step() stateless, I'd prefer derived classes to not have to think about it, and just be able to put anything in there. – Bomar

If B::Step() accesses fields of B then the thread must be stopped before running ~B(). So it shall be a responsibility of child class. – Wiegand

@Bomar Sorry stub is confusing. I meant A::Step() should be something like void Step(){ m_callback() }. Where m_callback is a function pointer or std::function. All the derived class should set the callback before executes the thread or derived destructor executes. Then the callback itself should only capture variables that lifetime is longer than derived class. Though it could only be taken care by the writer of derived class... – Stagg

In another way, I suggest the design to include an interface like ITeardown to take care of this case before destructor. like void tearDown(){ stop.store(true); th.join();} That makes the design simpler without dealing with object lifetimes.. – Stagg