Conditional variable wakes up before it's been notified

Question

I need a thread safe queue that yields data if it's not empty, and waits for a data to arrive. With a timeout. I have it like this

void ThreadsafeQueue::enqueue(data_t& data)
{
    std::lock_guard<std::mutex> lock(m_mutex);
    m_queue.push(data);

    m_condvar.notify_one();
}

boost::optional<data_t> ThreadsafeQueue::dequeue()
{
    std::unique_lock<std::mutex> lock(m_mutex);
    const std::chrono::seconds DEFAULT_DELAY(10);
    if ((!m_queue.empty()) || m_condvar.wait_for(lock, DEFAULT_DELAY) == std::cv_status::no_timeout)
    {
        const auto data = m_queue.front();
        m_queue.pop();
        return data;
    }

    return boost::none;
}

But, for some reason sometimes it sometimes enters the if statement with no_timeout when actually it wasn't notified. And tries to use front() on an empty queue.

Am I doing it wrong?

[This `wait_for` reference](https://en.cppreference.com/w/cpp/thread/condition_variable/wait_for) says that is can spuriously return. — Some programmer dude, Jan 31 '19 at 11:43

score 2 · Answer 1 · answered Jan 31 '19 at 11:41

2

Conditional variables are sometimes awoken by the system even when not notified. That is why, after awoken, you need to check if the condition was actually met or otherwise wait again.

answered Jan 31 '19 at 11:41

Attersson

4,755
1
15
29

paxdiablo · Accepted Answer · 2019-01-31T12:02:42.430

2

Since it's possible that multiple threads may be awoken by kicking a condition variable, or that spurious wake-ups can sometimes happen⁽¹⁾, you should check the predicate on wake-up to ensure it's true.

In other words, your dequeue code would be better off as something like:

boost::optional<data_t> ThreadsafeQueue::dequeue() {
    std::unique_lock<std::mutex> lock(m_mutex);
    static const std::chrono::seconds DEFAULT_DELAY(10);

    // Only wait on empty queue, return none on timeout.
    if (m_queue.empty())
        if (m_condvar.wait_for(lock, DEFAULT_DELAY) == std::cv_status::timeout)
            return boost::none;

    // Catch spurious wake-up.
    if (m_queue.empty())
        return boost::none;

    // Have item in queue, extract and return it.
    const auto data = m_queue.front();
    m_queue.pop();
    return data;
}

⁽¹⁾ As per here (my emphasis):

Atomically releases lock, blocks the current executing thread, and adds it to the list of threads waiting on *this. The thread will be unblocked when notify_all() or notify_one() is executed, or when the relative timeout rel_time expires. It may also be unblocked spuriously.

edited Jan 31 '19 at 12:02

answered Jan 31 '19 at 11:44

paxdiablo

854,327
234
1,573
1,953

But it's gonna wait even if the queue is empty. And I don't want it to wait if the queue is not empty :D – Eugene Mart Jan 31 '19 at 11:48
@Eugene: made modification to take that into account. – paxdiablo Jan 31 '19 at 11:50
Shouldn't it be ||, instead of &&? :D – Eugene Mart Jan 31 '19 at 11:54
@Eugene: yes but I've split the checks apart for readability so they're okay now. – paxdiablo Jan 31 '19 at 11:56
if (m_queue.empty()) return boost::none; why does it return the boost::none if it's empty? I mean, the first time – Eugene Mart Jan 31 '19 at 11:57
@Eugene, apologies, I think I understand now. I thought you were returning none if the queue was empty. Now the wait will only happen if the queue is empty when you call the function, otherwise it will extract head immediately. – paxdiablo Jan 31 '19 at 12:04

Conditional variable wakes up before it's been notified

2 Answers2