Search code examples
c#.netmultithreadingmanualresetevent

Is it safe to catch ObjectDisposedException on ManualResetEvent.WaitOne()?


This is closely related to Is it safe to signal and immediately close a ManualResetEvent? and might provide one solution to that problem.

Say I have a bunch of threads that potentially want to do the same work, but only one should be allowed to do it, and the others should wait until the worker is done and use its result.

So basically do I want work to be done only once.

Update: Let me add that this is not an initialization problem that could be addressed using .net 4's Lazy<T>. By once I mean once per task, and those tasks are determined at runtime. This might not be clear from the simplified example below.

Modifying the simple example from Hans Passant's answer to aforementioned question a bit, I guess the following would be safe. (It's slightly different from the use case just described, but in terms of threads and their relations, it is equivalent)

static void Main(string[] args)
{
    ManualResetEvent flag = new ManualResetEvent(false);
    object workResult = null;
    for (int ix = 0; ix < 10; ++ix)
    {
        ThreadPool.QueueUserWorkItem(s =>
        {
            try
            {
                flag.WaitOne();
                Console.WriteLine("Work Item Executed: {0}", workResult);
            }
            catch (ObjectDisposedException)
            {
                Console.WriteLine("Finished before WaitOne: {0}", workResult);
            }
        });
    }
    Thread.Sleep(1000);
    workResult = "asdf";
    flag.Set();
    flag.Close();
    Console.WriteLine("Finished");
}

I guess the core of my question is:

Is a call to WaitOne, aborted due to a ObjectDisposedException, equivalent to a successful call to WaitOne, in terms of memory barriers?

That should ensure safe access to the variable workResult by those other threads.

My guess: It would have to be safe, otherwise how could WaitOne safely figure out that the ManualResetEvent object had been closed in the first place?


Solution

  • Here is what I see:

    • You are getting ObjectDisposedException because your code exhibits the following race condition:
      • flag.close can be called before all threads have managed to invoke flag.waitOne.

    How you handle this depends on how important the execution of the code is after flag.waitOne.

    Here is one approach:

    If all the threads that have been spun up really should execute, you could have some extra synchronisation before calling flag.close. You could achieve this by using the StartNew on Task.Factory instead of Thread.QueueUserWorkItem. Tasks can be waited upon for completion and then you would invoke the flag.close thus eliminating the race condition and the need to handle the ObjectDisposedException

    Your code would then become:

        static void Main(string[] args)
        {
            ManualResetEvent flag = new ManualResetEvent(false);
            object workResult = null;
            Task[] myTasks = new Task[10];
            for (int ix = 0; ix < myTasks.Length; ++ix)
            {
                myTasks[ix] = Task.Factory.StartNew(() =>
                {
                    flag.WaitOne();
                    Console.WriteLine("Work Item Executed: {0}", workResult);
                });
            }
    
            Thread.Sleep(1000);
            workResult = "asdf";
            flag.Set();
            Task.WaitAll(); // Eliminates race condition
            flag.Close();
            Console.WriteLine("Finished");
        }
    

    As you can see above, Tasks allow for extra synchronization that would eliminate the race condition that you are seeing.

    As an extra note, ManualResetEvent.waitOne performs a memory barrier, so the workresult variable will be the latest one updated without requiring any further memory barriers or volatile reads.

    So to answer your question, if you reaaaaally must avoid additional synchronisation and handle an ObjectDisposed exception by going with your approach, I would argue that a disposed object has not performed a memory barrier for you, you would have to call Thread.MemoryBarrier in your catch block to ensure that the latest value has been read.

    But exceptions are expensive and if you can avoid them for normal program execution I do believe that it is prudent to do so.

    Good luck!