Thread notification isn't working for my consumer

Question

I have an attempt at a producer/consumer

Producer

#pragma once
#ifndef PRODUCER_H
#define PRODUCER_H

#include <thread>
#include "Mailbox.h"

class Producer
{
private:
    std::thread producer;
    Mailbox& mailbox;
public:
    Producer(Mailbox& newmailbox);
    ~Producer();
    void start();
    void run();
};

Producer::Producer(Mailbox& newMailbox) : mailbox(newMailbox) {}

Producer::~Producer() {}

void Producer::start()
{
    producer = std::thread(&Producer::run, this);
}

void Producer::run()
{
    mailbox.inc();
}

#endif

Consumer

#pragma once
#ifndef CONSUMER_H
#define CONSUMER_H

#include "Mailbox.h"
#include <thread>
#include <iostream>

class Consumer
{
private:
    Mailbox& mailbox;
    std::thread consumer;
public:
    Consumer(Mailbox& newMailbox);
    ~Consumer();
    void start();
    void run();
};

Consumer::Consumer(Mailbox& newMailbox) : mailbox(newMailbox) {}


Consumer::~Consumer() {}

void Consumer::start()
{
    consumer = std::thread(&Consumer::run, this);
}

void Consumer::run()
{
    mailbox.read();
}

#endif

Mailbox

#pragma once
#ifndef MAILBOX_H
#define MAILBOX_H

#include <mutex>
#include <iostream>

class Mailbox
{
private:
    int& mailbox;
    int init_val;
    std::mutex mmutex;
    std::condition_variable condition;
public:
    Mailbox();
    ~Mailbox();
    void inc();
    void read();
};

Mailbox::Mailbox() : mailbox(init_val), init_val(0) {}

Mailbox::~Mailbox()
{

}

void Mailbox::inc()
{   
    int count = 0;
    while (count < 10)
    {

        std::unique_lock<std::mutex> lock(mmutex);
        std::cout << "Producer increment\n";
        mailbox += 1;
        lock.unlock();

        count += 1;
    }

}

void Mailbox::read()
{
    int count = 0;
    while (count < 10)
    {

        std::unique_lock<std::mutex> lock(mmutex);

        condition.wait(lock, [this](){return get_cflag(); });

        condition.notify_one();

        count += 1;

    }

}

#endif

Main

int main()
{

    Mailbox* mailbox = new Mailbox();
    Consumer* consumer = new Consumer(*mailbox);
    Producer* producer = new Producer(*mailbox);

    consumer->start();
    producer->start();

    return 0;
}

Mutex locking works albeit asynchronously because I have no control over when a std::thread will start so I decided to implement a semi-synchronous methodology using std::unique_lock in addition to std::mutex.

Problem is, the Consumer waits and the Producer flies on ahead with no notification at least that is what the debugger is telling me and the last Producer iteration result sin a n abort() so something is going wrong here.

Nowhere do any of your threads ever check whether or not they need to wait for anything. Specifically, your `read` function calls `wait` without checking whether it needs to wait and then proceeds after `wait` returns without checking whether it needs to wait. Condition variables are stateless -- it's your job to check whether you need to wait both before deciding to call `wait` and before deciding to proceed. — David Schwartz, Oct 13 '17 at 00:16
@DavidSchwartz Given your comment, I did some more research and read through `C++ Concurrency In Action (Anthony Williams) an approachable solution. — Mushy, Oct 14 '17 at 16:24
@WorldSEnder I corrected the int mailbox reference to int& mailbox and the copy assignment I haven't completed yet. — Mushy, Oct 14 '17 at 17:40
@Mushy instead of letting the mailbox have a reference to int, have the consumer and producer take a reference to a mailbox. This is because you need to reuse the same mutex (which is not copyable) to safeguard the access to the mailbox-storage — WorldSEnder, Oct 14 '17 at 17:40
@WorldSEnder They are, I just haven't taken updates on my local and posted them here -- I will now — Mushy, Oct 14 '17 at 17:43

score 0 · Answer 1 · edited Oct 14 '17 at 18:00

0

I’m not a C++ guy, but if these condition variables work the way I think they do, you’ll only get notified if a signal arrives while you’re waiting. If the signal arrived before you started waiting, you’ll block indefinitely.

After you acquire the lock in 'Mailbox::read`, you should check to see if an item is available, and only wait on the condition variable if there isn’t one. If there is, go ahead and take it:

int Mailbox::read()
{
    std::unique_lock<std::mutex> lock(m);
    while (mailbox <= 0)
        condition.wait(lock);
    return mailbox--;
}

edited Oct 14 '17 at 18:00

milleniumbug

15,379
3
47
71

answered Oct 13 '17 at 00:01

Mike Strobel

25,075
57
69

His `read` functions calls `wait` even if the thing he is waiting for has already happened. And when `wait` returns, it assumes that the thing he was waiting for has happened. You *MUST* check -- the condition variable does not know whether or not the thing you are waiting for has happened. It's your job to track that. – David Schwartz Oct 13 '17 at 00:15
I changed the producer and consumer functions to incorporate lambda expressions with wait using a simple flag to signal readiness or wait. Thank you Mike and David. – Mushy Oct 14 '17 at 16:25

Mushy · Answer 2 · 2017-10-14T17:30:30.350

Based upon David Schwartz's comment, insight from Mike Strobel, and additional research, I changed the producer and consumer functions

Producer

void Mailbox::inc()
{   
    int count = 0;
    while (count < 10)
    {

        std::unique_lock<std::mutex> lock(mmutex);
        std::cout << "Producer increment\n";
        mailbox += 1;
        lock.unlock();
        set_cflag(true); // signal to the consumer data is ready
        condition.notify_one();

        {
            std::unique_lock<std::mutex> lock(mmutex);
            condition.wait(lock, [this]() {return get_pflag(); });
        }

        set_pflag(false);

        count += 1;
    }

}

Consumer

 void Mailbox::read()
{
    int count = 0;
    while (count < 10)
    {

        std::unique_lock<std::mutex> lock(mmutex);

        condition.wait(lock, [this](){return get_cflag(); });

        std::cout << "Consumer: " << mailbox << "\n";
        lock.unlock();

        set_pflag(true);
        condition.notify_one();

        count += 1;

        set_cflag(false);

    }

}

Mailbox

class Mailbox
{
private:
    int& mailbox;
    int cflag, pflag;
    int init_val;
    std::mutex mmutex;
    std::condition_variable condition;
public:
    Mailbox();
    ~Mailbox();
    int get_cflag() { return cflag; }
    void set_cflag(int newFlag) { cflag = newFlag; }
    int get_pflag() { return pflag; }
    void set_pflag(int newFlag) { pflag = newFlag; }
    void inc();
    void read();
};

Mailbox::Mailbox() : mailbox(init_val), init_val(0), cflag(0), pflag(0) {}

Mailbox::~Mailbox()
{

}

The output upon execution is as I desired

int main()
{

    Mailbox* mailbox = new Mailbox();
    Consumer* consumer = new Consumer(*mailbox);
    Producer* producer = new Producer(*mailbox);

    consumer->start();
    producer->start();

    fgetc(stdin);

    return 0;
}

Producer increment

Consumer: 1

Producer increment

Consumer: 2

Producer increment

Consumer: 3

Producer increment

Consumer: 4

Producer increment

Consumer: 5

Producer increment

Consumer: 6

Producer increment

Consumer: 7

Producer increment

Consumer: 8

Producer increment

Consumer: 9

Producer increment

Consumer: 10

I am unable to call `std::thread.join()` as `std::thread.terminate()` may have already been called with the class threads exiting the functions and the program terminating — Mushy, Oct 14 '17 at 16:58
I was unclear on why the producer and consumer threads were `join`ing themselves. What effect could that have, other than a no-op or a deadlock? Typically, the thread that _spawns_ worker threads does the joining. — Mike Strobel, Oct 14 '17 at 17:25

Thread notification isn't working for my consumer

2 Answers2