What is unsafe in this extreme simple thread Monitor implementation?

Question

I am new in thread syncronization. I was reading many implementations of conditional variables, like boost::threads and pthread for win32. I just implemented this quite simple monitor with wait/notify/noifyall, and i guess that there are many hidden problems with it, that I would like to discover from more experienced people. Any suggestion?

class ConditionVar
{

public :
    ConditionVar () : semaphore ( INVALID_HANDLE_VALUE ) , total_waiters (0)
    {
        semaphore = ::CreateSemaphoreA ( NULL , 0 /* initial count */ , LONG_MAX /* max count */ , NULL );
    }

    ~ConditionVar ()
    {
        ::CloseHandle ( semaphore ) ;       
    }


public :
    template <class P>
    void Wait ( P pred )
    {
        while ( !pred() ) Wait();
    }

public :

    void Wait ( void )
    {
        INTERLOCKED_WRITE_RELEASE(&total_waiters,total_waiters + 1 );
        ::WaitForSingleObject ( semaphore , INFINITE );
    }

    //! it will notify one waiter
    void Notify ( void )
    {
        if ( INTERLOCKED_READ_ACQUIRE(&total_waiters) )
        {
            Wake (1);
        }
    }

    void NotifyAll (void )
    {
        if ( INTERLOCKED_READ_ACQUIRE(&total_waiters) )
        {
            std::cout << "notifying " << total_waiters ;
            Wake ( total_waiters );
        }
    }

protected :
    void Wake ( int count )
    {
        INTERLOCKED_WRITE_RELEASE(&total_waiters,total_waiters - count );
        ::ReleaseSemaphore ( semaphore , count , NULL );
    }

private :
    HANDLE semaphore;
    long    total_waiters;
};

Answer 1

if your using WinAPI functions, its probably better to use InterlockedIncrement(...) and InterlockedDecrement(...) instead of INTERLOCKED_WRITE_RELEASE(&total_waiters,total_waiters + 1 ); and INTERLOCKED_WRITE_RELEASE(&total_waiters,total_waiters - count ); respectively.

Answer 2

I think bad things will happen if you copy your instance since both copies will use the same sempahore. This isn't necessarily a bad thing, but it might confuse people if the semantics aren't entirely clear.

You can easily fix this with a similar approach to what boost::noncopyable uses (or use boost).

Answer 3

I prefer Boost's implementation of wait(), because it takes an RAII lock object to make sure access to the shared condition state is synchronized. RAII makes it easier to write exception-safe code.

I have annotated the sample code found here:

boost::condition_variable cond;
boost::mutex mut;
bool data_ready;

void process_data();

void wait_for_data_to_process()
{
    boost::unique_lock<boost::mutex> lock(mut);  // Mutex acquired here (RAII).
    while(!data_ready) // Access to data_ready is sync'ed via mutex 'mut'
    {
        // While we are blocking, the mutex is released so
        // that another thread may acquire it to modify data_ready.
        cond.wait(lock);

        // Mutex is acquired again upon exiting cond.wait(lock)
    }
    process_data();

    // Mutex is released when lock goes out of scope.
}

Answer 4

(self answer)

i found a big mistake.

CondVars have an external lock... and this hasnt.