16

The topic says it. I don't understand why the std::queue (or in general: any queue) is not thread-safe by its nature, when there is no iterator involved as with other datastructures.

According to the common rule that

  • at least one thread is writing to ...
  • and another thread is reading from a shared resource

I should have gotten a conflict in the following example code:

#include "stdafx.h"
#include <queue>
#include <thread>
#include <iostream>

struct response
{
    static int & getCount()
    {
        static int theCount = 0;
        return theCount;
    }

    int id;
};


std::queue<response> queue;

// generate 100 response objects and push them into the queue
void produce()
{
    for (int i = 0; i < 100; i++)
    {
        response r; 
        r.id = response::getCount()++;
        queue.push(r);
        std::cout << "produced: " << r.id << std::endl;
    }
}

// get the 100 first responses from the queue
void consume()
{
    int consumedCounter = 0;
    for (;;)
    {       
        if (!queue.empty())
        {
            std::cout << "consumed: " << queue.front().id << std::endl;
            queue.pop();
            consumedCounter++;
        }

        if (consumedCounter == 100)
            break;
    }
}

int _tmain(int argc, _TCHAR* argv[])
{

    std::thread t1(produce);
    std::thread t2(consume);

    t1.join();
    t2.join();

    return 0;
}

Everything seems to be working fine: - No integrity violated / data corrupted - The order of the elements in which the consumer gets them are correct (0<1<2<3<4...), of course the order in which the prod. and cons. are printing is random as there is no signaling involved.

netik
  • 1,736
  • 4
  • 22
  • 45
  • Maybe if you run it for several days instead of a few microseconds you might get a different result. Also, a single producer and single consumer thread isn't a particularly stressful thread safety test. – Michael Burr Apr 21 '16 at 07:17
  • Ok that's a point. But I'm trying to understand why might I even get a problem. I mean, data is inserted in the back while data is read from the front of the queue, so how can there even be a problem? – netik Apr 21 '16 at 07:18
  • 3
    Simple example: call `q.empty()`. What does the result mean if something else can add or remove elements from the queue concurrently? The interface is not designed for concurrent use. – juanchopanza Apr 21 '16 at 07:20
  • The standard is worded like this to give implementors of "push" and "pop" the latitude to optimize their implementations without taking thread safety into consideration. While any particular implementation might not have any race conditions or other thread safety issues this will not imply other conformant c++ implementations don't. – Chris Becke Apr 21 '16 at 07:20
  • 2
    Thread safety typically has a looooooot of overhead that goes completely unused without threads. Plus, as [Java found with synchronizing vector](http://stackoverflow.com/questions/1386275/why-is-java-vector-class-considered-obsolete-or-deprecated), most of the time it is transactions, not individual accesses, that needs to be protected. – user4581301 Apr 21 '16 at 07:22
  • 3
    For one thing, even if `std::queue()` were thread safe as far as it's methods were concerned, how could it possibly protect against an item being added to the queue between calls to `front()` and `pop()`? If you run your program long enough, you should at least see missing IDs in the output simply because while the ID for one element is being printed, another one gets added by the producer thread, then that one gets popped before it's ID can be printed (and you'd see a duplicate of the ID you just printed, since it wouldn't get popped at the appropriate time). – Michael Burr Apr 21 '16 at 07:22
  • 1
    If everything that seemed to be working fine actually did, we wouldn't need regular software updates. – molbdnilo Apr 21 '16 at 07:29
  • just for the sake of reading. the following article shows a solution without mutex (even if it does not directly use std::queue). have a look it might be interesting for you: http://blogs.msmvps.com/vandooren/2007/01/05/creating-a-thread-safe-producer-consumer-queue-in-c-without-using-locks/ – Stefano Apr 21 '16 at 07:29
  • 1
    @Stefano: it should be noted that the technique in that article relies on a non-standard detail of how MSVC++ treats `volatile` variables, and even with MSVC++ may also depend on the target and/or a compiler options being used: https://msdn.microsoft.com/en-us/library/12a04hfd.aspx – Michael Burr Apr 21 '16 at 07:37
  • A small and not very technical answer... more on the intuitive side... As you said it is normally considered safe to have only one thread reading and one writing. In your case the "reading thread" performs a "pop" which if you think it well modified the queue... so in the end you have 2 threads who make modification to the same object and this is not safe. – Stefano Apr 21 '16 at 07:38
  • @Michael... true... since i was just reading the article yesterday and i found this related i thought it might interest Netik ... so i just shared it... but as you said this is kind of a corner case – Stefano Apr 21 '16 at 07:40
  • Are we all doomed to write boilerplate code? Is there any standard library solution? – decadenza Jun 15 '20 at 16:20

1 Answers1

30

Imagine you check for !queue.empty(), enter the next block and before getting to access queue.first(), another thread would remove (pop) the one and only element, so you query an empty queue.

Using a synchronized queue like the following

#pragma once

#include <queue>
#include <mutex>
#include <condition_variable>

    template <typename T>
    class SharedQueue
    {
    public:
        SharedQueue();
        ~SharedQueue();

        T& front();
        void pop_front();

        void push_back(const T& item);
        void push_back(T&& item);

        int size();
        bool empty();

    private:
        std::deque<T> queue_;
        std::mutex mutex_;
        std::condition_variable cond_;
    }; 

    template <typename T>
    SharedQueue<T>::SharedQueue(){}

    template <typename T>
    SharedQueue<T>::~SharedQueue(){}

    template <typename T>
    T& SharedQueue<T>::front()
    {
        std::unique_lock<std::mutex> mlock(mutex_);
        while (queue_.empty())
        {
            cond_.wait(mlock);
        }
        return queue_.front();
    }

    template <typename T>
    void SharedQueue<T>::pop_front()
    {
        std::unique_lock<std::mutex> mlock(mutex_);
        while (queue_.empty())
        {
            cond_.wait(mlock);
        }
        queue_.pop_front();
    }     

    template <typename T>
    void SharedQueue<T>::push_back(const T& item)
    {
        std::unique_lock<std::mutex> mlock(mutex_);
        queue_.push_back(item);
        mlock.unlock();     // unlock before notificiation to minimize mutex con
        cond_.notify_one(); // notify one waiting thread

    }

    template <typename T>
    void SharedQueue<T>::push_back(T&& item)
    {
        std::unique_lock<std::mutex> mlock(mutex_);
        queue_.push_back(std::move(item));
        mlock.unlock();     // unlock before notificiation to minimize mutex con
        cond_.notify_one(); // notify one waiting thread

    }

    template <typename T>
    int SharedQueue<T>::size()
    {
        std::unique_lock<std::mutex> mlock(mutex_);
        int size = queue_.size();
        mlock.unlock();
        return size;
    }

The call to front() waits until it has an element and locks the underlying queue so only one thread may access it at a time.

iwbnwif
  • 327
  • 2
  • 13
user66875
  • 2,772
  • 3
  • 29
  • 55
  • 2
    I like your implementation and i think i will us this. But in your explanation you are assuming having multiple consumer. he has only one consumer so nobody else will call empty, first or pop. At worst he could have a push in between – Stefano Apr 21 '16 at 08:00
  • 4
    `front()` implements a leaky abstraction. It returns a reference into the private container, but does nothing to protect the element. If thread A calls `front()` and thread B calls `pop_front()` things stop looking good. A reference is a liability. The code ignores this. – IInspectable Jan 10 '20 at 07:45
  • 2
    You cannot have `pop` and `pop_front` be separate either. Thread A and B could be calling `front` each first and then both `pop_front`. Then you processed one element twice and one not at all. – walnut Jan 10 '20 at 08:53
  • https://stackoverflow.com/questions/15278343/c11-thread-safe-queue better answer here – vacing Jul 13 '20 at 12:25