HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppModerate

Thread-safe queue

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
queuethreadsafe

Problem

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 
#include 
#include 

template 
class BlockingQueue
{
private:
    std::mutex              d_mutex;
    std::condition_variable d_condition;
    std::deque           d_queue;
public:
    void push(T const& value) {
        {
            std::unique_lock lock(this->d_mutex);
            d_queue.push_front(value);
        }
        this->d_condition.notify_one();
    }
    T pop() {
        std::unique_lock 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 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();
    }
};

Solution

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.

Code Snippets

std::move (this->d_queue.back());
v = this->d_queue.back();
[=]{ return !this->d_queue.empty(); }
[this]{ return !this->d_queue.empty(); }

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

Context

StackExchange Code Review Q#49820, answer score: 11

Revisions (0)

No revisions yet.