Clever Geek Handbook

STL Queue with two streams (Producer, Consumer)

I want a critical section to keep the queue safe so that threads don't access the queue at the same time. This code works even if I comment out the lines related to the Critical section. Can someone explain why?

queue<int> que;
CRITICAL_SECTION csection;
int i=0;

DWORD WINAPI ProducerThread(void*)
{

    while(1)
    {
        //if(TryEnterCriticalSection(&csection))
        {
            cout<<"Pushing value "<<i<<endl;
            que.push(i++);
            //LeaveCriticalSection(&csection);
        }
    }
}

//Consumer tHread that pops out the elements from front of queue
DWORD WINAPI ConsumerThread(void*)
{
    while(1)
    {
        //if(TryEnterCriticalSection(&csection))
        {
            if(!que.empty())
            {
                cout<<"Value in queue is "<<que.front()<<endl;
                que.pop();
            }
            else
                Sleep(2000);
            //LeaveCriticalSection(&csection);
        }
    }
}

int _tmain(int argc, _TCHAR* argv[])
{
    HANDLE handle[2];
    //InitializeCriticalSection(&csection);
    handle[0]=NULL;
    handle[1]=NULL;
    handle[0]=CreateThread(0,0,(LPTHREAD_START_ROUTINE)ProducerThread,0,0,0);
    if(handle[0]==NULL)
        ExitProcess(1);

    handle[1]=CreateThread(0,0,(LPTHREAD_START_ROUTINE)ConsumerThread,0,0,0);
    if(handle[1]==NULL)
        ExitProcess(1);

    WaitForMultipleObjects(2,handle,true,INFINITE);

    return 0;
}
+3


source to share


4 answers


In your particular case, cout will take several hundred times longer than "get". and you sleep when the queue is empty, which allows another thread to fill up a lot of the queue before your "consumer" thread chooses any of them.

Run at full speed (no debug prints, no sleep), make sure you run LONG time and check the value at the other end with simple math.

Something like that:

int old_val = val;
while(1)
{
    if(!que.empty())
    {
       int  val = que.front();

       que.pop();
       if (old_val+1 != val)
       {
          /// Do something as things have gone wrong!
       }
     }
}

Note that this also may not go wrong immediately / trivially. You want to run it for a few hours, preferably something else running on the machine - something like a batch file with:



@echo off
:again 
dir c:\ /s > NUL:
goto again

[It's been a while since I wrote batch scripts for Windows, so this may not be 100% correct, but I think you should be able to answer whatever I got, the idea is to "machine].

Also, try running multiple copies of your thread pair with a separate queue per pair - this will force more scheduling activity and may cause a problem.

As Anton says, some of these things are often very difficult to reproduce. I had an issue on a real-time operating system where the queue got messed up - the only REAL sign was that memory eventually ran out during a "stress test" (which does "random" things, including several different sources of interrupts) ... The OS has been tested in hundreds of units in production trials and hit the field as real production systems [and the same code running on a different processor has worked on telephone switches around the world, again, without customer complaints about memory leaks] seemingly , no memory leaks! But one "hole" in the processing of the queue, in the ONE function, which was started. Thinking that the stress test itself, which sometimes faced some strange situation,when the queues were popping up eventually I found the real problem - an interrupt between reading and writing a queue - opening exactly two instructors and only when the interrupt routine was interrupted by another interrupt routine while sending a message ... I would rather not debug this again!

+2


source


This works by accident, presumably for two reasons:

  • It doesn't work, but you never notice. The consumer pulls whatever is in the queue, or whatever he thinks is in the queue. If there is nothing, it sleeps until the manufacturer pushes anything. It “works” because the producer only joins the end, while the consumer only reads from the beginning. Except for the update size

    . You will most likely have a queue in a state that has items, but size

    does not reflect this. This is disgusting, but the opposite, which is likely to happen sooner or later, is even more disgusting. You have no way of knowing. After all, you may know if, for some reason, work items that are queued "disappear" or if you run out of memory, but try to figure out why.
  • You are using printf

    (or std::cout

    equivalently) that is internally blocked by a critical section. This "view" will block access to the queue the way you want it, unless it is not. It will work 99.9% of the time (accidentally, as the consumer will be blocked trying to print, which takes longer than the producer adding the queue). However, when the context switch happens immediately after printing, it fails unexpectedly. Bang, you're dead.


You really absolutely have to protect sections of critical code with a critical section object or mutex. Otherwise, the results are unpredictable. And contrary to what one might believe, "but it works" not very well, this is the worst thing that can happen. Because it only works until it does, and then you don't know why.

However, you can use the I / O completion port, which does whatever works for you very efficiently. You can use GetQueuedCompletionStatus to take the "event" out of the port and use PostQueuedCompletionStatus to post it. The completion port does all of the queue processing, including proper synchronization with multiple consumers for you (and it does it in LIFO order, which is beneficial to avoid context switches and cache invalidation).
Each event contains a pointer to a structure OVERLAPPED

, but the completion port does not use it, you can just pass any pointer (or, if you feel better, pass a pointer to OVERLAPPED

, followed by your own data).

+2


source


One of the biggest problems with bugs like this is CRITICAL_SECTION

preventing them from being damn hard to reproduce. You have to anticipate how it can fail without showing it up.

When you are protecting your own code, instead of swapping unsafe library calls, you can usually trigger a race condition by appending Sleep

somewhere. In the code you posted, there is no way to do this for the producer (any invariants are broken, this is done internally que.push

) and the potential TOCTTOU problem with checking a consumer against an empty queue does not exist when there is only one consumer. If we could add Sleep

queues to the implementation, then we might be doing something wrong in a predictable way.

0


source


It is possible that the queue code is safe for such a pop poll if there is only one producer and one consumer. If the push producer uses a temp / pointer to insert data to the next empty position in the queue, and only stores the added "temp index" in the "next empty" member queue, queue.empty may return true until it is safe to data to be pushed by the consumer. Such an operation may be designed or may occur by accident.

As soon as you have more than one producer or more than one consumer, it will explode sooner or later.

Edit - even if the queue turns out to be safe with one producer and one user, you shouldn't rely on it unless it's documented - some b *** d will change the implementation in the following version :(

-1


source






More articles:


All Articles