Skip to main content
Spellfix
Source Link
Toby Speight
  • 81.7k
  • 14
  • 101
  • 308

You have three constructors, a default one and two that initialize the container based on existing data. But the constructor of std::deque has many more forms. Picking only a few of them is inconsitentinconsistent, and having to copy all of them is a lot of work. Instead, consider using perfect forwarding:

I see you have a destructor that calls wake(), but that is very unsafe. No thread should be in pop_wait() when the destructor is called. Consider what happens after the destructor returns and those other threads have woken up: they assume they still have a live queue object! So the only time it is safe to call the destructor is if no threads are using the queue object anymoreany more, and thus there is no need to call wake() in the destructor.

You have three constructors, a default one and two that initialize the container based on existing data. But the constructor of std::deque has many more forms. Picking only a few of them is inconsitent, and having to copy all of them is a lot of work. Instead, consider using perfect forwarding:

I see you have a destructor that calls wake(), but that is very unsafe. No thread should be in pop_wait() when the destructor is called. Consider what happens after the destructor returns and those other threads have woken up: they assume they still have a live queue object! So the only time it is safe to call the destructor is if no threads are using the queue object anymore, and thus there is no need to call wake() in the destructor.

You have three constructors, a default one and two that initialize the container based on existing data. But the constructor of std::deque has many more forms. Picking only a few of them is inconsistent, and having to copy all of them is a lot of work. Instead, consider using perfect forwarding:

I see you have a destructor that calls wake(), but that is very unsafe. No thread should be in pop_wait() when the destructor is called. Consider what happens after the destructor returns and those other threads have woken up: they assume they still have a live queue object! So the only time it is safe to call the destructor is if no threads are using the queue object any more, and thus there is no need to call wake() in the destructor.

Source Link
G. Sliepen
  • 61.7k
  • 3
  • 61
  • 152

Consider using perfect forwarding to initialize the container

You have three constructors, a default one and two that initialize the container based on existing data. But the constructor of std::deque has many more forms. Picking only a few of them is inconsitent, and having to copy all of them is a lot of work. Instead, consider using perfect forwarding:

template<typename... Args>
explicit queue(Args&&... args)
: container(std::forward<Args>(args)...) {}

The destructor should not call wake()

I see you have a destructor that calls wake(), but that is very unsafe. No thread should be in pop_wait() when the destructor is called. Consider what happens after the destructor returns and those other threads have woken up: they assume they still have a live queue object! So the only time it is safe to call the destructor is if no threads are using the queue object anymore, and thus there is no need to call wake() in the destructor.

For a task pool it is important that you can signal the worker threads that they should stop doing any work. That has to be done in some other way; for example by setting a flag that signals that work has to stop. The destructor of the thread pool should look like:

class thead_pool {
    queue q;
    std::vector<std::thread> threads;
    …
    ~thread_pool() {
        q.stop(); // signal queue that no more items will be pushed
        for (auto& thread: threads)
            threads.join(); // not necessary if you use std::jthread
        // after the destructor exits, q will be destroyed
    }
};

So queue should have something like:

template<…>
class queue {
public:
    …
    void stop() {
        auto lock = std::unique_lock<std::mutex>{mutex};
        stopped = true;
        wake();
    }

private:
    std::mutex mutex;
    …
    bool stopped = false;
};

Always wait using a predicate

std::condition_variable::wait() can wake up spuriously (i.e., without notify_one() or notify_all() having been called), and in more complex programs it's hard to see when a condition variable can be notified. So it's almost always better to use wait() with a predicate:

bool pop_wait(value_type& value)
{
    auto lock = std::unique_lock<std::mutex>{mutex};
    cond.wait(lock, [&]{return !container.empty() || stopped;});

    if (container.empty()) // can only happen if stopped == true
        return false;

    value = container.front();
    container.pop_front();
    return true;
}

Consider returning std::optional<T>

Instead of passing a reference and returning a bool, consider returning a std::optional<T>. This is much nicer and safer to use.

Don't pass a duration in a specific unit

Instead of passing the time to wait as std::chrono::milliseconds, make it a generic type, just like the duration argument of std::condition_variable::wait_for() itself.

No need to delete the copy constructor/assignment operator

Because std::mutex and std::condition_variable are not copyable, your class will automatically be non-copyable, and you don't have to delete the copy constructor and copy assignment operator.

Allow moving data into and out of the queue

You are copying data when you push to and pop from the queue. That can be inefficient for large types, and it also prevents you from storing non-copyable types in the queue. Consider using r-value references and/or perfect forwarding to push things into the queue, i.e. by having a push() with the same overloads as std::deque::push_back(), and an emplace() with the same parameters as std::deque::emplace_back(). Use std::move() when popping an item from the queue into a temporary variable (you don't need to move it when returning the temporary).

Notify without holding the lock

It's slightly more efficient to call notify_one() and notify_all() without having the mutex locked, otherwise there is a possibility the other thread(s) will wake, see that the mutex is still locked, then they have to do a system call to wait again until the mutex is unlocked.