8
\$\begingroup\$

I am trying to implement a thread-safe deque in C++.
ThreadSafeDeque will be used by a FileLogger class.
When threads call the log() function of FileLogger the messages will be push_back()ed to ThreadSafeDeque and return almost immediately. In a separate thread the FileLogger will pop_front() messages and write them to a file in its own pace.

Am I doing things correctly and efficient below?

#pragma once
#include <deque>
#include <mutex>
template<class T>
class ThreadSafeDeque {
public:
    void pop_front_waiting(T &t) {
        // unique_lock can be unlocked, lock_guard can not
        std::unique_lock<std::mutex> lock{ mutex }; // locks
        while(deque.empty()) {
            condition.wait(lock); // unlocks, sleeps and relocks when woken up  
        }
        t = deque.front();
        deque.pop_front();
    } // unlocks as goes out of scope

    void push_back(const T &t) {
        std::unique_lock<std::mutex> lock{ mutex }; 
        deque.push_back(t);
        lock.unlock();
        condition.notify_one(); // wakes up pop_front_waiting  
    }
private:
    std::deque<T>               deque;
    std::mutex                  mutex;
    std::condition_variable condition;
};  
\$\endgroup\$
1
  • \$\begingroup\$ The use case you present calls for a queue. \$\endgroup\$
    – greybeard
    Commented Jan 4, 2022 at 12:46

2 Answers 2

8
\$\begingroup\$

It's probably worth accepting a second template argument for an Allocator to be passed through to the std::deque:

template<class T, class Allocator = std::allocator<T>>
class ThreadSafeDeque {
    std::deque<T, Allocator> deque;
};

Technically, we do need to include <condition_variable>; we're not allowed to assume that <mutex> always pulls it in.

I don't like the interface to pop_front_waiting() - why does the caller have to pass an lvalue reference, instead of simply being returned the value? Return-value optimisation will ensure there's no unnecessary copy.

The locking logic all looks good to me. We can avoid the lock.unlock() by using a simple lock-guard with smaller scope:

void push_back(const T &t)
{
    {
        std::lock_guard<std::mutex> lock{ mutex }; 
        deque.push_back(t);
    }
    condition.notify_one(); // wakes up pop_front_waiting  
}

From C++17, we can use constructor deduction and omit the template parameter from std::lock_guard and std::unique_lock, making the code a little easier to read.

It's helpful to show that members are intentionally default-constructed, and it pacifies g++ -Weffc++:

std::deque<T> deque = {};
std::mutex mutex = {};
std::condition_variable condition = {};
\$\endgroup\$
5
  • \$\begingroup\$ The pop_front_waiting interface is necessary to provide exception safety with types that are not nothrow movable. Named return value optimization is not guaranteed, so there may be a move construction into the return value that could throw, in which case the object would already be removed from the queue and lost. \$\endgroup\$
    – user207766
    Commented Mar 5, 2020 at 4:33
  • \$\begingroup\$ Arguably move constructors shouldn't throw though, so a static_assert(std::is_nothrow_move_constructible_v<T>) test might be preferable to using the weird interface. \$\endgroup\$
    – user207766
    Commented Mar 5, 2020 at 4:39
  • \$\begingroup\$ Where would there be a non-guaranteed copy elision? AFAICS, that would only be possible in old (pre-C++17) platforms. Problems would be endemic if T could throw when moved, as std::deque is allowed to move elements (e.g. in shrink_to_fit()). As walnut says, probably better to avoid accepting such types. \$\endgroup\$ Commented Mar 5, 2020 at 8:17
  • \$\begingroup\$ Copy elision in a return statement was made mandatory only from prvalues, not from operands naming local variables and you need an intermediate variable to store the copy from deque.front(). See cppreference. deque::shrink_to_fit() does not have strong exception safety guarantees if the move constructor throws, but all the modifiers at the beginning and end of the deque are guaranteed to have no effect if any exceptions at all are thrown, see [deque.modifiers]. \$\endgroup\$
    – user207766
    Commented Mar 5, 2020 at 13:36
  • \$\begingroup\$ So if the interface was kept the way OP has it, the class would work with strong exception guarantee on both member functions, even with non-nothrow movable types. \$\endgroup\$
    – user207766
    Commented Mar 5, 2020 at 13:37
3
\$\begingroup\$

Other answers cover a lot but I also would suggest replacing

while(deque.empty()) {
    condition.wait(lock); // unlocks, sleeps and relocks when woken up  
}

with

condition.wait(lock, [this]{return !deque.empty();});

because it means the same and is more compact (readability being the same or also better).

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Why would you suggest this change? What improvements does it bring? Can you edit your answer to describe the rationale? \$\endgroup\$ Commented Mar 4, 2020 at 17:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.