Is this implementation of a Blocking Queue safe?

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::Co开发者_开发问答mmonCpp::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

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.

Condition variables should be helpful if you are targetting newest Windows versions. This typically makes implementing blocking queues simpler.

See here for design of a similar queue using Boost - even if you cannot use Boost or condition variables, the general guidance and follow-up discussion there should be useful.