CODEWITHSUNDEEP
c#.netmultithreadingmanualresetevent
Evgeniy Berezovsky
Evgeniy Berezovsky

Reputation: 19228

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?

Upvotes: 2

Views: 2885

Answers (3)

Evgeniy Berezovsky
Evgeniy Berezovsky

Reputation: 19228

Reflecting a bit more on the actual problem I had to solve, which is a bit more complicated than the simplified example, I decided to use Monitor.Wait and Monitor.PulseAll.

Joe Albahari's Threading in C# proved extremely useful, in this particular case the following section applies: http://www.albahari.com/threading/part4.aspx#_Signaling_with_Wait_and_Pulse

Upvotes: 0

Anastasiosyal
Anastasiosyal

Reputation: 6626

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!

Upvotes: 3

Chris Shain
Chris Shain

Reputation: 51339

A few points:

  1. If this is .NET 4, then Lazy is the better way to do this.

  2. Whether it is equivalent from the perspective of memory barriers or not is mostly irrelevant- Exceptions should never be part of the normal code path. I'd assume the behavior is undefined, as this is not an expected use case.

Upvotes: 0

Related Questions