Blocking Queue implementation: Where's the race condition?

Question

It's me and my BlockingQueue again... I rewrote it according to this article and this question. It sends some items and then crashes with an access violation. Here's the code:

template <typename T>
bool DRA::CommonCpp::CTBlockingQueue<T>::Push( T pNewValue ){
    volatile long oldSize;
    ::InterlockedExchange( &oldSize, m_Size );
    CTNode* pNewNode = new CTNode();
    pNewNode->m_pValue = pNewValue;
    {//RAII block
        CGuard g( m_TailCriticalSection );
        m_pTailNode->m_pNext = pNewNode;
        m_pTailNode = pNewNode;
        ::InterlockedIncrement( &m_Size );
    }
    if( oldSize == 0 )
        m_eAtLeastOneElement.set();
    return true;
}

template <typename T>
bool DRA::CommonCpp::CTBlockingQueue<T>::Pop( T& pValue ){
    CTNode* pCurrentNode;
    {//RAII block
        CGuard g( m_HeadCriticalSection );
        pCurrentNode = m_pHeadNode;
        CTNode* pNewHeadNode = m_pHeadNode->m_pNext;
        if( pNewHeadNode == NULL ){
            CEvent* pSignaledEvent;
            CEvent::waitForPair( m_eAtLeastOneElement, m_eFinished, pSignaledEvent );
            if( pSignaledEvent == &m_eFinished )
                return false;
            pNewHeadNode = m_pHeadNode->m_pNext;
        }
        pValue = pNewHeadNode->m_pValue;
        m_pHeadNode = pNewHeadNode;
        ::InterlockedDecrement( &m_Size );
    }
    delete pCurrentNode;
    return true;
}

It always crashes in a call to Pop(), in the line after the if, the one which says:

pValue = pNewHeadNode->m_pValue

It blows up cos' pNewHeadNode is NULL. But how can this happen?

Edit: Forgot initialization code:

template <typename T>
DRA::CommonCpp::CTBlockingQueue<T>::CTBlockingQueue():
        m_HeadCriticalSection("CTBlockingQueue<T>::m_Head"),
        m_TailCriticalSection("CTBlockingQueue<T>::m_Tail"){
    CTNode* pDummyNode = new CTNode();
    m_pHeadNode = pDummyNode;
    m_pTailNode = pDummyNode;
    m_Size = 0; //Dummy node doesn't count
}

Why on earth you need interlocked increment/decrement when you have a mutex? — , Jul 14 '11 at 18:36
To ask for the queue size. I didn't post it, but there's a getter method for that — dario_ramos, Jul 14 '11 at 18:39
Wait, there's another, more important reason: to wake up pending Pop calls who blocked on an empty queue. See the final lines of Push() — dario_ramos, Jul 14 '11 at 18:45
You don't need that interlocked magic. You can get that size w/o interlocked operation inside the critical section that you hit anyway. — , Jul 14 '11 at 19:07
... and you crash because you don't check if `pNewHeadNode` is not NULL in case your queue is empty :) — , Jul 14 '11 at 19:09
Ok, but the Interlocked calls don't do any harm, do they? I'll try removing them once I get this working — dario_ramos, Jul 14 '11 at 19:09
That's why I added the pNewHeadNode = m_pHeadNode->m_pNext; line at the end of the if, but it didn't solve the issue... — dario_ramos, Jul 14 '11 at 19:11
They are damn slow to use them when you don't have to. As for the crash - you wake up, queue is empty - `m_pHeadNode` is NULL and you dereference it by accessing `m_pNext`. So there you go. After all, why can't you catch SIGSEGV in gdb and see exactly where you crash? Or Windows is in the way? — , Jul 14 '11 at 19:20
@dario_ramos let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/1478/discussion-between-vlad-lazarenko-and-dario-ramos) — , Jul 14 '11 at 19:21

Chad · Answer 1 · 2011-07-14T18:41:32.740

1

My assumption would have to be the fact that the event is set outside of the critical section, meaning push could potentially notify the event twice. Have you tried with setting the event inside the critical section?

edited Jul 14 '11 at 18:41

answered Jul 14 '11 at 18:15

Chad

18,706
4
46
63

I added the initialization code in my answer: I create a dummy node to avoid this scenario – dario_ramos Jul 14 '11 at 18:17
I see, I've updated my answer with why `m_pHeadNode` is causing the exception. – Chad Jul 14 '11 at 18:21
pCurrentNode is deleted after m_pHeadNode is changed to point somewhere else, I don't see a problem there – dario_ramos Jul 14 '11 at 18:29
You're right, that isn't the problem. My answer is not correct. – Chad Jul 14 '11 at 18:30
Edited again, this time with a stab in the dark :( – Chad Jul 14 '11 at 18:41
Sorry, it blows up just the same – dario_ramos Jul 14 '11 at 18:53

score 0 · Accepted Answer · edited May 23 '17 at 12:04

In the end, I went back to my original, less efficient implementation, the one I posted here, with the addition of a Finish() method so that the producer can signal the consumers to end elegantly, and a Restart() method to start producing again without destroying and recreating the queue:

//Template definitions
template<class Element>
DRA::CommonCpp::CTBlockingQueue<Element>::CTBlockingQueue( unsigned int maxSize ):
    m_csFinished( "CTBlockingQueue::m_csFinished" ),
    m_csQueue( "CTBlockingQueue::m_csQueue" ),
    m_semElementCount( 0, maxSize ),
    m_bFinished(false){
}

template<class Element>
DRA::CommonCpp::CTBlockingQueue<Element>::~CTBlockingQueue(){
    Finish();
}

template<class Element>
void DRA::CommonCpp::CTBlockingQueue<Element>::Push( Element newElement ){
    {//RAII block
        CGuard g( m_csQueue );
        m_Queue.push( newElement );
    }
    m_semElementCount.Signal();
}

template<class Element>
bool DRA::CommonCpp::CTBlockingQueue<Element>::Pop( Element& element ){
    m_semElementCount.Wait();
    {//RAII block
        CGuard g( m_csFinished );
        if( m_bFinished ){
            CGuard g( m_csQueue );
            if ( m_Queue.size() == 0 )
                return false;
        }
    }
    {//RAII block
        CGuard g( m_csQueue );
        element = m_Queue.front();
        m_Queue.pop();
    }
    return true;
}

template<class Element>
void DRA::CommonCpp::CTBlockingQueue<Element>::Finish(){
    {//RAII block
        CGuard g( m_csFinished );
        m_bFinished = true;
    }
    {//RAII block
        CGuard g( m_csQueue );
        m_semElementCount.Signal();
    }
}

template<class Element>
void DRA::CommonCpp::CTBlockingQueue<Element>::Restart(){
    {//RAII block
        CGuard g( m_csFinished );
        m_bFinished = false;
    }
}

This ain't the fastest way to go, but works fast enough for me.

Blocking Queue implementation: Where's the race condition?

2 Answers2