Thread-safe queue

Question

This is what I have which is mostly based on code I've found on Stack Overflow. How good is it? Can you suggest better implementation?

#pragma once

#include <mutex>
#include <condition_variable>
#include <deque>

template <typename T>
class BlockingQueue
{
private:
    std::mutex              d_mutex;
    std::condition_variable d_condition;
    std::deque<T>           d_queue;
public:
    void push(T const& value) {
        {
            std::unique_lock<std::mutex> lock(this->d_mutex);
            d_queue.push_front(value);
        }
        this->d_condition.notify_one();
    }
    T pop() {
        std::unique_lock<std::mutex> lock(this->d_mutex);
        this->d_condition.wait(lock, [=]{ return !this->d_queue.empty(); });
        T rc(std::move(this->d_queue.back()));
        this->d_queue.pop_back();
        return rc;
    }
    bool tryPop (T & v, std::chrono::milliseconds dur) {
        std::unique_lock<std::mutex> lock(this->d_mutex);
        if (!this->d_condition.wait_for(lock, dur, [=]{ return !this->d_queue.empty(); })) {
            return false;
        }
        v = std::move (this->d_queue.back());
        this->d_queue.pop_back();
        return true;
    }
    int size() {
        return d_queue.size();
    }
};

Answer 1

Overall the code looks functional to me. Everytime you pop it will wait on the condition variable if the queue is empty. Everytime you push, a notification will be sent to any thread waiting in the pop method because the queue is empty and let it know that the queue is not empty anymore. I can't speak for your intentions but that is generally how people want their queues to work.

A few minor points I noticed:

The std::move in this line is useless since deque::back() will give you an rvalue anyway.

 std::move (this->d_queue.back());

It looks like someone on SO trying to look clever not actually understanding the code.

This will suffice.

v = this->d_queue.back();

In the lambda functions, capturing everything by value is completely unnecessary.

[=]{ return !this->d_queue.empty(); }

Capture only the this pointer or only the deque by reference.

[this]{ return !this->d_queue.empty(); }

[&d_queue]{ return !d_queue.empty(); }

Both approaches have the same overhead.

Answer 2

What @user2675345 said.

Also don't force a std::deque on your users. There is already a concept of a queue in C++. http://www.cplusplus.com/reference/queue/queue/ Use the same technique and default to std::deque but allow other types.

template <typename T, Container = std::deque<T>>
class BlockingQueue
{
private:
    std::mutex              d_mutex;
    std::condition_variable d_condition;
    Container               d_queue;

It might be useful to support the same API as a std::queue

Answer 3

I think you should also lock the size() function. The return type should also be size_t (or even deque<T>::size_type).