views:

154

answers:

2

My particular scenario: - Main thread starts a worker thread. - Main thread needs to block itself until either worker thread is completed (yeah funny) or worker thread itself informs main thread to go on

Alright, so what I did in main thread:

wokerThread.Start(lockObj);
lock(lockObj)
 Monitor.Wait(lockObj);

Somewhere in worker thread:

if(mainThreadShouldGoOn)
 lock(lockObj)
  Monitor.Pulse(lockObj);

Also, at the end of worker thread:

lock(lockObj)
 Monitor.Pulse(lockObj);

So far, it's working perfect. But is it a good solution? Is there a better one?

EDIT:

What if I do it like this in main thread:

Monitor.Enter(lockObj);
workerThread.Start(lockObj);
Monitor.Wait(lockObj);

And worker looks like this:

void Worker(object lockObj)
{
 Monitor.Enter(lockObj);
 ...
 ...
 ...
 if(mainThreadShouldGoOn)
 {
  Monitor.Pulse(lockObj);
  Monitor.Exit(lockObj);
 }
 ...
 ...
 ...
 if(!mainThreadShouldGoOn)
 {
  Monitor.Pulse(lockObj);
  Monitor.Exit(lockObj);
 }
}
+1  A: 

Why do you start a worker thread just to go to sleep right after? You could simply do the work in the main thread!

But assuming you have a reason to do so...

I think your solution is conceptually ok, but there might be a problem. Consider the case when a context switch occurs after the main thread has started the worker but before it would acquire the lock. Then the worker would quickly finish its job within a single time slice. In this case, Pulse is called before Wait, thus later the main thread will never wake up from the wait. Here's an illustration:

class Program
{
    static object theLock = new object();

    static void Main(string[] args)
    {
        Thread worker = new Thread(WorkerMain);
        worker.Start();
        Thread.Sleep(1);
        lock (theLock)
        {
            Monitor.Wait(theLock);
        }
        Console.WriteLine("Main done");
        Console.ReadLine();
    }

    static void WorkerMain()
    {
        lock (theLock)
        {
            Monitor.Pulse(theLock);
        }
    }
}

Most of the time this code will hang. You can fix the problem by moving worker.Start inside the scope of the lock.

One more thing to be careful about is making sure that Pulse is called, even in exceptional cases. Use try finally's right and you'll be fine.

Or consider separating the task of the worker in two parts: one that is always to be carried out, and the rest that right now follows after the if(mainThreadShouldGoOn) {...} check. Then you could execute the first part in a worker thread started by main, and the second part in another worker thread started from the first worker if neccessary. Then you can use Thread.Join() to wait for the first worker to be done. This would make synchroniozation simpler and less error prone by sacrificing a little performance.

kicsit
Hey, thanks for your detailed reply. Based a bit on your suggestions, I revised my solution. Please see edit in my original post.
Griever
+2  A: 

This code is not correct, you run the risk of permanently blocking the main thread if the worker completes too soon. The proper synchronization object to use here is a ManualResetEvent (or auto, doesn't matter). Call its Wait() method in the main thread, its Set() method in the worker. As long as it is a Thread instead of a thread pool thread, using Thread.Join() would work just as well.

Hans Passant
Please see edit in my original post.
Griever