2

The following code is a struct I have written to allow me to queue up work to run on multiple worker threads, and which blocks the main thread until a new worker thread is available.

struct WorkQueue{
    const std::size_t size;
    std::vector<uint8_t> threadActive;
    std::condition_variable cond;
    std::mutex mu;

    std::vector<std::jthread> threads;

    WorkQueue(std::size_t size_ = 1) :
        size{size_}
    {
        threads.resize(size);
        threadActive.resize(size);
        std::fill(std::begin(threadActive), std::end(threadActive), false);
    }

    template<typename Function>
    void enqueue(Function&& f){
        auto numActiveThreads = [&](){
            return std::count(std::begin(threadActive), std::end(threadActive), true);
        };
        auto availableThread = [&](){
            return std::find(std::begin(threadActive), std::end(threadActive), false) - std::begin(threadActive);
        };

        std::unique_lock<std::mutex> lock{mu};

        //wait until a thread becomes available
        if(numActiveThreads() == size){
            cond.wait(lock, [&](){ return numActiveThreads() != size; });
        }

        //get an available thread and mark it as active
        auto index = availableThread();
        threadActive[index] = true;

        //start the new thread and swap it into place
        std::jthread thread{[this, index, fn = std::forward<Function>(f)](){
            fn();
            threadActive[index] = false;
            cond.notify_one();
        }};
        threads[index].swap(thread);
    }
};

I have two questions about the code in the lambda executed by the jthread:

  1. Is it necessary to lock the mutex mu when setting threadActive[index] = false?

  2. Is the compiler allowed to reorder the code and execute cond.notify_one() before setting threadActive[index] = false?

I think it is necessary to lock the mutex if and only if the compiler is allowed to reorder the statements. Is this correct?

Ben
  • 470
  • 1
  • 6
  • 18
  • 1
    1. Yes. As written, your program has a race condition on elements of `threadActive`, where one thread reads them while another modifies them, with no synchronization. 2. It doesn't much matter. Condition variables are allowed to wake up spuriously, it is possible that one wakes up right before `threadActive[index] = false` is executed, which would have the same effect as reordering it with `notify_one`. Your program should be written to work correctly anyway. – Igor Tandetnik Mar 14 '21 at 00:16
  • 1
    Even without spurious wakeup or reordering: imagine that a thread in slot 1 finishes executing `fn()`, sets `threadActive[1]=false` and calls `cond.notify_one()`. The main thread then wakes up and calls `numActiveThreads()` - this accesses `threadActive[0]` first, and only then `threadActive[1]`. Which is clearly a race with the thread in slot 0, which could be executing `threadActive[0] = false` at that exact time. – Igor Tandetnik Mar 14 '21 at 00:25
  • @IgorTandetnik Thanks. So to fix the problem, it should be enough to replace `threadActive[index] = false;` with `{ std::lock_guard lock{mu}; threadActive[index] = false; }`, yes? If you write these comments as an answer, I will accept it. – Ben Mar 14 '21 at 00:35

0 Answers0