Is this implementation of a Blocking Queue safe?

Question

I'm trying to implement a queue which blocks on the Pop operation if it's empty, and unblocks as soon as a new element is pushed. I'm afraid I might have some race condition; I tried to look at some other implementation, but most I found were done in .NET, and the few C++ I found depended too much on other library classes.

template <class Element>
class BlockingQueue{
    DRA::CommonCpp::CCriticalSection    m_csQueue;
    DRA::CommonCpp::CEvent              m_eElementPushed;
    std::queue<Element>                 m_Queue;
public:
    void Push( Element newElement ){
        CGuard g( m_csQueue );
        m_Queue.push( newElement );
        m_eElementPushed.set();
    }
    Element Pop(){
        {//RAII block
            CGuard g( m_csQueue );
            bool wait = m_Queue.empty();
        }
        if( wait )
            m_eElementPushed.wait();
        Element first;
        {//RAII block
            CGuard g( m_csQueue );
            first = m_Queue.front();
            m_Queue.pop();
        }
        return first;
    }
};

Some explanations are due:

• CCriticalSection is a wrapper for a Windows Critical Section, methods Enter and Leave are private, and CGuard is its only friend
• CGuard is a RAII wrapper for CCriticalSection, enters critical section on constructor, leaves it on destructor
• CEvent is a wrapper for a Windows Event, wait uses the WaitForSingleObject function
• I don't mind that Elements are passed around by value, they are small objects
• I can't use Boost, just Windows stuff (as I have already been doing with CEvent and CGuard)

I'm afraid there might be some weird race condition scenario when using Pop(). What do you guys think?

UPDATE: Since I'm working on Visual Studio 2010 (.NET 4.0), I ended up using the unbounded_buffer class provided by the C++ runtime. Of course, I wrapped it in a class using the Pointer to Implementation Idiom (Chesire Cat) just in case we decide to change the implementation or need to port this class to another environment

Answer 1

It’s not thread safe:

    {//RAII block
        CGuard g( m_csQueue );
        bool wait = m_Queue.empty();
    }
    /// BOOM! Other thread ninja-Pop()s an item.
    if( wait )
        m_eElementPushed.wait();

Notice the location of the BOOM comment. In fact, other locations are also thinkable (after the if). In either case, the subsequent front and pop calls will fail.