Writing a thread that stays alive

Question

I would like to write a class that wraps around std::thread and behaves like a std::thread but without actually allocating a thread every time I need to process something async. The reason is that I need to use multi threading in a context where I'm not allow to dynamically allocate and I also don't want to have the overhead of creating a std::thread.

Instead, I want a thread to run in a loop and wait until it can start processing. The client calls invoke which wakes up the thread. The Thread locks a mutex, does it's processing and falls asleep again. A function join behaves like std::thread::join by locking until the thread frees the lock (i.e. falls asleep again).

I think I got the class to run but because of a general lack of experience in multi threading, I would like to ask if anybody can spot race conditions or if the approach I used is considered "good style". For example, I'm not sure if temporary locking the mutex is a decent way to "join" the thread.

EDIT I found another race condition: when calling join directly after invoke, there is no reason the thread already locked the mutex and thus locks the caller of join until the thread goes to sleep. To prevent this, I had to add a check for the invoke counter.

Header

#pragma once

#include <thread>
#include <atomic>
#include <mutex>

class PersistentThread
{
public:
    PersistentThread();
    ~PersistentThread();

    // set function to invoke
    // locks if thread is currently processing _func
    void set(const std::function<void()> &f);

    // wakes the thread up to process _func and fall asleep again
    // locks if thread is currently processing _func
    void invoke();

    // mimics std::thread::join
    // locks until the thread is finished with it's loop
    void join();

private:

    // intern thread loop
    void loop(bool *initialized);

private:

    bool                            _shutdownRequested{ false };

    std::mutex                      _mutex;

    std::unique_ptr<std::thread>    _thread;
    std::condition_variable         _cond;

    std::function<void()>           _func{ nullptr };
};

Source File

#include "PersistentThread.h"

    PersistentThread::PersistentThread()
{
    auto lock = std::unique_lock<std::mutex>(_mutex);
    bool initialized = false;

    _thread = std::make_unique<std::thread>(&PersistentThread::loop, this, &initialized);

    // wait until _thread notifies, check bool initialized to prevent spurious wakeups
    _cond.wait(lock, [&] {return initialized; });
}

PersistentThread::~PersistentThread()
{
    {
        std::lock_guard<std::mutex> lock(_mutex);

        _func = nullptr; 
        _shutdownRequested = true;

        // wake up and let join
        _cond.notify_one();
    }

    // join thread, 
    if (_thread->joinable())
    {
        _thread->join();
    }   
}

void PersistentThread::set(const std::function<void()>& f)
{
    std::lock_guard<std::mutex> lock(_mutex);
    this->_func = f;
}

void PersistentThread::invoke()
{
    std::lock_guard<std::mutex> lock(_mutex);
    _cond.notify_one();
}

void PersistentThread::join()
{
    bool joined = false;
    while (!joined)
    {
        std::lock_guard<std::mutex> lock(_mutex);
        joined = (_invokeCounter == 0);
    }   
}

    void PersistentThread::loop(bool *initialized)
{

    std::unique_lock<std::mutex> lock(_mutex);
    *initialized = true;
    _cond.notify_one();

    while (true)
    {       
        // wait until we get the mutex again
        _cond.wait(lock, [this] {return _shutdownRequested || (this->_invokeCounter > 0); });

        // shut down if requested
        if (_shutdownRequested) return;

        // process
        if (_func) _func();
        _invokeCounter--;

    }
}

Answer 1

You are asking about potential race conditions, and I see at least one race condition in the shown code.

After constructing a PersistentThread, there is no guarantee that the new thread will acquire its initial lock in its loop() before the main execution thread returns from the constructor and enters invoke(). It is possible that the main execution thread enters invoke() immediately after the constructor is complete, ends up notifying nobody, since the internal execution thread hasn't locked the mutex yet. As such, this invoke() will not result in any processing taking place.

You need to synchronize the completion of the constructor with the execution thread's initial lock acquisition.

EDIT: your revision looks right; but I also spotted another race condition.

As documented in the description of wait(), wait() may wake up "spuriously". Just because wait() returned, doesn't mean that some other thread has entered invoke().

You need a counter, in addition to everything else, with invoke() incrementing the counter, and the execution thread executing its assigned duties only when the counter is greater than zero, decrementing it. This will guard against spurious wake-ups.

I would also have the execution thread check the counter before entering wait(), and enter wait() only if it is 0. Otherwise, it decrements the counter, executes its function, and loops back.

This should plug up all the potential race conditions in this area.

P.S. The spurious wake-up also applies to the initial notification, in your correction, that the execution thread has entered the loop. You'll need to do something similar for that situation, too.