ManualResetEvent - how might a race condition occur here?

Question

I am trying to determine whether I can use a ManualResetEvent here to ensure that in a concurrent environment, the inner actions of myMethod() are never invoked concurrently.

    static volatile bool _syncInitialized = false;  

    static ManualResetEvent _syncEvent = new ManualResetEvent(false);

    static object _syncLock = new object();

    void myMethod()
    {
        lock (_syncLock)       
        {
            if (!_syncInitialized)          // sync hasn't started, so 
            {
                _syncInitialized = true;    
                _syncEvent.Set();           // signal for the first time only 
            }
        }

        if (_syncEvent.WaitOne())           // wait for signal
        {
            _syncEvent.Close(); 
            _syncEvent = new ManualResetEvent(false); // reset to false 

            // actions that should be forced to run sequentially
        }
    }

EDIT - Note I am using ManualResetEvent instead of just lock() because I want to be able to add a timeout, potentially.

But lock is supposed to run your code one thread at a time, why use ManualResetEvent? — Christian, Dec 29 '12 at 20:34
An MRE is only useful for signaling, it does **not** provide exclusion. Use the *lock* keyword, Mutex or Semaphore for that. — Hans Passant, Dec 29 '12 at 20:37
@Christian, good question. Using ManualResetEvent so that I can use a timeout on WaitOne(), if necessary. — Sean Thoman, Dec 29 '12 at 20:37

score 3 · Accepted Answer · answered Dec 29 '12 at 20:58

You have at least one opportunity for a race condition. Consider:

Thread #1 executes the _syncEvent.WaitOne() and succeeds, then gets swapped out before it can execute the _syncEvent.Close(). Thread #2 comes along and executes the WaitOne(), and also succeeds.

Another problem is that you're calling Close() followed by constructing a new instance, apparently as a way to reset. Imagine, then, that you call Close(), the thread is swapped out, the next thread comes along and tries to do WaitOne(), and throws an exception because the object has been closed.

If you want to reset the event, call Reset().

You probably can't make this work with a ManualResetEvent. As others have said, ManualResetEvent is used for signaling, not mutual exclusion.

You say that you'll want to implement a timeout in the future. If you just want a thread to wait on the lock for a period of time and then exit if it can't obtain the lock, use one of the Monitor.TryEnter overloads that accept a timeout value. For example:

private object _syncObject = new Object();
void MyMethod()
{
    if (!Monitor.TryEnter(_syncObject, TimeSpan.FromSeconds(5)))
    {
        return; // couldn't get the lock
    }
    try
    {
        // got the lock. Do stuff here
    }
    finally
    {
        Monitor.Exit(); // release the lock
    }
}

There is some debate over whether you really want to release the lock in the finally. If the code throws an exception, then it's possible (likely?) that the resource you were protecting is now in an incomplete or otherwise corrupt state. In that case, you might not want to let other threads act on it. Whether you release the lock in the face of exceptions is a design decision that you'll have to make, consistent with your application's requirements.

ManualResetEvent - how might a race condition occur here?

1 Answers1