9
\$\begingroup\$

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();
    }
};
\$\endgroup\$
  • \$\begingroup\$ i completely replaced this queue with two - boost spsc and tbb concurrent_bounded_queue. \$\endgroup\$ – javapowered Apr 28 '15 at 17:01
11
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ But isn't won't the compiler be smart enough to capture only the variables that are used? \$\endgroup\$ – Paul Draper Jun 1 '14 at 2:15
  • 10
    \$\begingroup\$ It seems this is pretty much code I posted to stackoverflow, i.e., I'm the person who "wants to look clever not actually understanding the code" (the insulted for short). Incidentally, std::deque<...>::back() returns a reference not a value_type according to 23.3.3.1 [deque.overview] paragraph 2. Thus, your reasoning is wrong and your code change actually doesn't work as std::unique_ptr<T> doesn't have a copy assignment. You might want to check your facts before making smug comments. \$\endgroup\$ – Dietmar Kühl Aug 28 '14 at 15:19
  • \$\begingroup\$ @DietmarKühl, your point about unique_ptr is incorrect, because of how BlockingQueue::push implemented. It will not accept unique_ptr anyway. But part about move is ok. \$\endgroup\$ – Yola Sep 22 '15 at 3:43
6
\$\begingroup\$

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

\$\endgroup\$
5
\$\begingroup\$

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

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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