Issue with ManualResetEvent not releasing all waiting threads consistently

Question

I'm trying to implement a class which uses a simple cache for holding data retrieved from an internal service. I'm using a ManualResetEvent to block multiple threads which may try to refresh the cached data at the same time with the first thread to succeed signalling the others to proceed once the data has been retrieved by calling Set() and then Reset(). When testing I've noticed that sometimes all of the threads are released and sometimes 1 or more are not and are left to time out, almost as if I am calling Reset before all of the threads were released. Can someone explain what I am doing wrong?

I've included a cut down version of the code below.

    private bool _updating;
    private const int WaitTimeout = 20000;
    private DateTime _lastRefresh;
    private object _cacheData;
    private readonly ManualResetEvent _signaller = new ManualResetEvent(false);

private void RefreshCachedData()
        {
            Console.WriteLine("ThreadId {0}: Refreshing Cache", Thread.CurrentThread.ManagedThreadId);
        if (_updating)
        {
            Console.WriteLine("ThreadId {0}: Cache Refresh in progress, waiting on signal.", Thread.CurrentThread.ManagedThreadId);

            // another thread is currently updating the cache so wait for a signal to continue
            if (!_signaller.WaitOne(WaitTimeout))
                Console.WriteLine("ThreadId {0}: Thread timed out ({1}s) waiting for a signal that the cache had been refreshed",
                    Thread.CurrentThread.ManagedThreadId,WaitTimeout);

            Console.WriteLine("ThreadId {0}: Signal recieved to use refreshed cache.", Thread.CurrentThread.ManagedThreadId);
        }
        else
        {
            try
            {
                _updating = true;

                var result = _requestHandler.GetNewData();

                if (!result.Success)
                {
                        Console.WriteLine("Failed to retrieve new data.");
                }
                else
                {
                    // switch the cache with the new data
                    _cacheData = result.Data;

                    Console.WriteLine(
                        "ThreadId {0}: Cache refresh success.",
                        Thread.CurrentThread.ManagedThreadId);
                    Thread.Sleep(8000);
                }
            }
            catch (Exception ex)
            {
                Console.WriteLine("Error occured: {0}", ex);
            }
            finally
            {
                // Set the refresh date time regardless of whether we succeded or not
                _lastRefresh = DateTime.Now;
                _updating = false;

                // signal any other threads to to carry on and use the refreshed cache
                Console.WriteLine("ThreadId {0}: Signalling other threads that cache is refreshed.", Thread.CurrentThread.ManagedThreadId);
                _signaller.Set();
                _signaller.Reset();
            }
        }
    }

Phocis

Thread.Sleep(8000) and WaitOne sounds like it wont play along nicely. – Depart

The Thread sleep was there just for the purposes off testing to make sure that all the other threads entered the wait before the thread refreshing the cache released them – Phocis

This may be a red herring but if I put a thread sleep of 50ms between the set and reset, I have yet to see in testing any of the threads not be released. – Phocis

I can reproduce this behaviour here (and I find it quite strange). Another issue with your code might be a race-condition with multiple threads trying to read and write _updating at the same time, maybe leading to multiple threads updating the cache. But that's not the cause here... – Prostatitis

Here's a shorter reproduction of this: ideone.com/nuiyu – Prostatitis

@Prostatitis Thanks for pointing that race condition out – Phocis

Answer 1

Looks like your threads are not getting released from the ResetEvent before it is reset.

You could solve the problem by creating the event open and having the fist thread to enter your method reset it.

Alternatively you can avoid the vagaries of ManualResetEvent's behavior by doing something like this:

private object _latch = new object();
private bool _updating;

private void UpdateIfRequired() 
{
    lock (_latch) 
    {
        if (_updating) 
        {
            //wait here and short circuit out when the work is done
            while (_updating)
                Monitor.Wait(_latch);

            return;
        }

        _updating = true;
    }

    //do lots of expensive work here

    lock (_latch)
    {
        _updating = false;
        Monitor.PulseAll(_latch); //let the other threads go
    }
}

Check out this page for a great explanation as to how this works http://www.albahari.com/threading/part4.aspx#_Signaling_with_Wait_and_Pulse

Anuradhapura