1
\$\begingroup\$

I'm trying to practice some C++ and I wrote a simple queue to solve the Producer-Consumer Problem.

class Queue
{

public:
Queue() : mutex(), cv(), queue(), process(true), consumer(&Queue::dequeue, this)
{
}

~Queue()
{
    process = false;
    consumer.join();
}

void enqueue(int i)
{
    std::lock_guard<std::mutex> lock(mutex);
    queue.push(i);
    cv.notify_all();
}

void dequeue()
{
    while(process)
    {
        std::unique_lock<std::mutex> lock(mutex);
        while(queue.empty())
        {
            cv.wait(lock);
        }

        if(!process)
        {
            break;
        }

        int i = queue.front();
        queue.pop();
        std::cout << "Got " << i << '\n';
    }
}

private:
std::mutex mutex;
std::condition_variable cv;
std::queue<int> queue;
bool process;
std::thread consumer;

};

Is my implementation good? Is there anything I could've done better?

\$\endgroup\$
  • \$\begingroup\$ Are you asking for your code to be explained to you? Please clarify. \$\endgroup\$ – Phrancis Sep 7 '16 at 5:49
  • \$\begingroup\$ If by "How does that work?", you mean that you are asking us to explain why your code works, then the question would be off-topic. If what you meant to ask was "How well do you think I did?", then that would be OK. \$\endgroup\$ – 200_success Sep 7 '16 at 6:07
  • \$\begingroup\$ I was asking if my implementation was correct, and if I was using the proper techniques or not. I clarified the question. \$\endgroup\$ – user3666471 Sep 7 '16 at 12:43
5
\$\begingroup\$

Dead Lock Bug

If the queue is empty and the consumer thread is waiting in the condition variable. If you Queue goes out of scope the main thread will enter the detructor and perform join(). At this point you are locked. As both threads are waiting for the other.

Use of condition can be simplified.

    while(queue.empty())
    {
        cv.wait(lock);
    }

Can be replaced with:

    cv.wait(lock, [&queue](){return !queue.empty();});

When poping things from a queue use move.

// this performs a copy.
int i = queue.front();

// this should move the value from the que.
int i = std::move(queue.front());

Note: It makes little difference for an integer but if there is an object in the queue then it might.

Formatting

Your code is horribly formatted. Please do a better job in the future. Formatting is a critical part of coding. This is because your code will be read fro decades aftr you have written it by other humans.

\$\endgroup\$
4
\$\begingroup\$

Just a few nit-picky things:

  1. You are using C++, so it would be wise to split up your code into a header and source file.
  2. Showing your include statements would be helpful.
  3. I see no documentation on your methods. Unless it's something very straightforward (like a programming contest entry), methods in your public code should generally have some form of documentation provided for them.
  4. This thing is NOT a producer-consumer solution! This is a queue data structure that implements producer-consumer ideology to function. A producer-consumer solution can use a queue, but I've never seen it work the other way around. If you want to make a queue, then make it like a legitimate data structure.

Now onto the code:

  1. The first major thing I see wrong with your implementation is having the whole thing dependent on that process boolean. A data structure should be doing loads of things in the background free from the user's input. When given something, the structure will work with it. What you have now lowers the integrity of your code.

  2. The second major issue is the fact that this is neither a proper producer-consumer solution or data structure. Conveniently I've experimented with producers and consumers in C++11 before here on CR. Here's my question and solutions. There's another post linked to that one I'd highly encourage you to read.

\$\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.