Safely starting and stopping a thread owned by a class

Question

If I have a class running its own thread, I almost always try to write it like the following, with the thread started by the constructor. The reason for this is that an object can never be constructed on multiple threads, so there is never a danger of two threads trying to start the thread. This class prints a message every second until asked to stop, for example:

#include <mutex>
#include <thread>
#include <condition_variable>
#include <iostream>
#include <chrono>
#include <string>

class a_thread
{
public:
    explicit a_thread(const std::string& msg)
    : _msg(msg)
    , _stop(false)
    , _t(&a_thread::thread_main, this)
    {
    }

    ~a_thread()
    {
        stop();
    }

    void stop()
    {
        std::unique_lock<std::mutex> l(_m);
        if (!_stop)
        {
            _stop = true;
            l.unlock();
            _cv.notify_all();
            if (_t.joinable())
                _t.join();
        }
    }

private:
    // Disable copying (and moving)
    a_thread(const a_thread&) = delete;
    a_thread& operator=(const a_thread&) = delete;

    void thread_main()
    {
        using namespace std::chrono;
        auto next_print_time = steady_clock::now();
        std::unique_lock<std::mutex> l(_m);
        while (!_stop)
        {
            const auto now = steady_clock::now();
            if (now >= next_print_time)
            {
                std::cout << _msg << '\n';
                next_print_time = now + seconds(1);
            }
            else
            {
                _cv.wait_until(l, next_print_time);
            }
        }
    }

    std::string _msg;
    bool _stop;
    std::mutex _m;
    std::condition_variable _cv;
    std::thread _t;
};

However, sometimes it is desirable to defer the starting of the thread, which I implement like so:

#include <mutex>
#include <thread>
#include <condition_variable>
#include <iostream>
#include <chrono>
#include <string>

class b_thread
{
public:
    explicit b_thread(const std::string& msg)
    : _msg(msg)
    , _stop(true)
    {
    }

    ~b_thread()
    {
        stop();
    }

    void start()
    {
        std::unique_lock<std::mutex> l(_m);
        if (_stop)
        {
            _stop = false;
            l.unlock();
            _t = std::thread(&b_thread::thread_main, this);
        }
    }

    void stop()
    {
        std::unique_lock<std::mutex> l(_m);
        if (!_stop)
        {
            _stop = true;
            l.unlock();
            _cv.notify_all();
            if (_t.joinable())
                _t.join();
        }
    }

private:
    // Disable copying (and moving)
    b_thread(const b_thread&) = delete;
    b_thread& operator=(const b_thread&) = delete;

    void thread_main()
    {
        using namespace std::chrono;
        auto next_print_time = steady_clock::now();
        std::unique_lock<std::mutex> l(_m);
        while (!_stop)
        {
            const auto now = steady_clock::now();
            if (now >= next_print_time)
            {
                std::cout << _msg << '\n';
                next_print_time = now + seconds(1);
            }
            else
            {
                _cv.wait_until(l, next_print_time);
            }
        }
    }

    std::string _msg;
    bool _stop;
    std::mutex _m;
    std::condition_variable _cv;
    std::thread _t;
};

Until today, I was confident that this code was thread-safe, in the sense that the stop() and start() functions could be safely called from multiple threads on the same instance of the class. However, I think the following code could result in a thread being destroyed without first being joined:

int main()
{
    b_thread b("Hello world!");
    // Let the main thread be called 'Z'
    auto x = std::thread([&b] () { b.start(); }); // Thread X
    auto y = std::thread([&b] () { b.stop(); }); // Thread Y
    b.start();

    x.join();
    y.join();
}

If the timing is right then I believe the program could run as follows:

1. Thread X has assigned to _t, but _t has not yet acquired the mutex in thread_main(). _stop == false at the time X released mutex _m.
2. Thread Y saw _stop == false after acquiring the mutex and therefore it sets _stop = true before releasing the mutex. It is now calling _cv.notify_all().
3. Thread Z is able to acquire the mutex in this state, and sees _stop == true. It assigns to _t, and therefore destroys the thread constructed by X (which hasn't been join()ed yet). Badness ensues.

I also often use an atomic bool when my thread loop has a non-interruptible wait in it (for example, a loop calling poll()), like so. I believe it exhibits the same bug:

void start()
{
    if (_stop.exchange(false))
        _t = std::thread(&b_thread::thread_main, this);
}

void stop()
{
    if (!_stop.exchange(true) && _t.joinable())
        _t.join();
}

I'd appreciate thoughts on the following:

1. The a_thread class. This is a pattern I use a lot so I'd like to know if it could be improved.
2. Am I correct about the bug in b_thread?
3. If so, is there a way to make b_thread safe? What about with the atomic bool variant?

Answer 1

Don't let multiple threads access a std::thread object

Operations on a std::thread object are not thread-safe themselves. If you want to do this, you must hold a mutex such that only one thread can ever have access to it. You do have a mutex, but you unlock it before calling _t.join() and _t = std::thread(…), which in turn can cause undefined behavior.

Of course, not unlocking the mutex inside stop() is problematic, because keeping _m locked while calling _t.join() will cause a deadlock, as main_thread() is also holding the lock when it's not inside the call to wait_until(). To solve this issue, you could consider using two mutexes; one for the public API so only one operation can run at a time, and another one to modify _stop. The latter one you can unlock before calling _t.join().

Even if you have ensured access to _t is properly guarded at all times, I would still not recommend allowing multiple threads access to your a_thread and b_thread classes. The reason is that you will certainly get undeterministic behavior. If one thread calls start() and the other stop(), what will the end result be? A running thread_main() or nothing? In an actual program, this will almost certainly cause a problem.

Consider using std::jthread

You are not the only one that needs a thread object that properly cleans up after itself and that allows you to request the thread to stop itself. C++20 introduced std::jthread for this reason. Consider using that instead if possible. The semantics are a bit different though, and while it has the concept of a "stop token" that allows the thread to check whether it needs to stop, it doesn't provide a way to do timed waits on it like with a condition variable. If you want that, you have to provide your own mutex and condition variable.

If you cannot use C++20 yet, then consider whether reimplementing std::jthread instead of your a_thread and b_thread classes. That way, once you can use C++20, it is trivial to switch from your own class to a standard one.

Using an atomic flag

A std::atomic<bool> or std::atomic_flag only ensures atomic access to that boolean value. If you do:

void start()
{
    if (_stop.exchange(false))
         _t = std::thread(&b_thread::thread_main, this);
}

Then by the time it tries to assign something to _t, other threads might have accessed _stop atomically as well. You might think: but surely, if I just set _stop to false, then no other thread calling start() will be able to get true from the call to exchange()? But that is only true if no thread is calling stop().

So, you still need a proper mutex to ensure only one thread can call start() or stop() at any given time. But you can use an atomic flag to signal a running thread to stop, avoiding the need for the thread to hold any mutex.

Note that since C++20, the atomic types have wait() and notify_all() member functions, so you can still wait, but you don't need a mutex and condition variable.