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

Boost condition variable wrapper

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

Problem

I would like to create a wrapper for a Boost conditional variable:

class Cond_wrap
{
private:
    boost::condition_variable cond;
    boost::mutex              mutex;
    bool work_to_do;
public:
    Cond_wrap()
    {
        work_to_do = false; 
    }
    void notify_all()
    {
        boost::mutex::scoped_lock lock(mutex);
        work_to_do = true;
        cond.notify_all();
    }
    void wait()
    {
        boost::mutex::scoped_lock lock(mutex);
        while(!work_to_do)
        {
            cond.wait(lock);
            work_to_do = false;
        }
    }
    bool timed_wait(unsigned int timeout)
    {
        boost::mutex::scoped_lock lock(mutex);
        if(!work_to_do)
        {
            if(cond.timed_wait(lock, boost::chrono::milliseonds(timeout)))
            {
                work_to_do = false;
                return true;
            }
            else
            {
                return false;
            }
        }
        else
        {
            return false;
        }
};

Cond_wrap condition_wrap;

void worker_func()
{
    {
        condition_wrap.notify_all();
    }
    std::cout << "After notify" << std::endl;
}

int main()
{
    boost::thread work(worker_func);
    work.detach();

    {
        boost::this_thread::sleep_for(boost::chrono::milliseonds(500));
        condition_wrap.wait();
        //there is work to do
    }
    return 0;
}


What is the main goal of that wrapper? I would like to avoid a situation in which a condition will be notified before I call waiter. Regarding this, I want to provide a help variable bool which should remember if a condition was previously set.

Small example of what I mean:

```
boost::condition_variable cond;
boost::mutex mutex;

void worker_func()
{
cond.notify_all();
std::cout << "After notify" << std::endl;
}

void main()
{
boost::mutex::soped_lock lock(mutex);
boost::thread work(worker_func);
boost::this_thread::sleep_for(boost::chrono::milliseonds(500

Solution

The short answer here is: don't do this. It does not make boost::condition_variable easier to use. You're actually just limiting what you can do with it. You're not even exposing the entire interface...

But if you insist on doing such a thing, your wrapper is close. You're not handling spurious wakeup in timed_wait() - that can return true even without being signaled. You should just take advantage of the fact that the various wait() overloads also take a predicate:

void wait()
{
    boost::mutex::unique_lock lock(mutex);
    cond.wait(lock, [this]{ return work_to_do; });
    work_to_do = false;
}

bool timed_wait(unsigned int timeout)
{
    boost::mutex::unique_lock lock(mutex);
    if (cond.timed_wait(lock, boost::chrono::milliseconds(timeout),
                        [this]{ return work_to_do; })
    {
        work_to_do = false;
        return true;
    }
    else
    {
        return false;
    }
}


Really adding predicates is the way to handle calling wait() after the notify was called. The wrapper is not a good solution to this in my opinion.

Code Snippets

void wait()
{
    boost::mutex::unique_lock lock(mutex);
    cond.wait(lock, [this]{ return work_to_do; });
    work_to_do = false;
}

bool timed_wait(unsigned int timeout)
{
    boost::mutex::unique_lock lock(mutex);
    if (cond.timed_wait(lock, boost::chrono::milliseconds(timeout),
                        [this]{ return work_to_do; })
    {
        work_to_do = false;
        return true;
    }
    else
    {
        return false;
    }
}

Context

StackExchange Code Review Q#96927, answer score: 3

Revisions (0)

No revisions yet.