- Why you got a deadlock
The short answer first: You missed Reset for Set.
I've copied your code (changed the braces to my prefered style) and I'll explain the problem in the comments:
private ManualResetEvent _event = new ManualResetEvent (true);
private void process()
{
//...
lock(_event)
{
_event.WaitOne(); //Thread A is here waiting _event to be set
//...
}
}
internal void Stop()
{
_event.Reset(); //But thread B just did reset _event
lock(_event) //And know thread B is here waiting... nobody is going to set _event
{
//...
}
}
With that part clear, let's move onward to the solution.
- Solving the deadlock
Since we are going to exchange .Reset() with .Set() we will also have to change the default state of the ManualResetEvent from true to false.
So, to solve the deadlock edit the code as follows [tested]:
private ManualResetEvent _event = new ManualResetEvent (false);
private void process()
{
//...
lock(_event)
{
_event.WaitOne(); //Thread A will be here waiting for _event to be set
//...
}
}
internal void Stop()
{
_event.Set(); //And thread B will set it, so thread a can continue
lock(_event) //And when thread a releases the lock on _event thread b can enter
{
//...
}
}
The code above does not only enforce that only one thread can enter the lock at the same time, but also that the thread that enters process will wait until there is a thread that calls Stop.
- But you got a race condition... fixing it.
The job is not done, because the code above suffers from the illness of a race condition. To understand why to imagine what happens in the case where multiple threads call process. Only one thread will enter the lock and will wait until Stop is called and _event is set, after that, it can continue. Now, consider what happens if the thread that calls Stops gets preempted just after it calls _event.Set(), the waiting thread that was at _event.WaitOne() continues and leaves the lock... now you can't tell if another thread that was waiting to enter the lock-in process will enter or if the thread that was preempted in Stop will continue and enter the lock-in that method. That is a race condition, I don't think you want that particular one.
That said I offer you yet a better solution [tested]:
private ManualResetEvent _event = new ManualResetEvent (false);
private ReaderWriterLockSlim _readWrite = new ReaderWriterLockSlim();
private void process()
{
//...
_readWrite.EnterReadLock();
_event.WaitOne();
try
{
//...
}
finally
{
_readWrite.ExitReadLock();
}
}
internal void Stop()
{
//there are three relevant thread positions at the process method:
//a) before _readWrite.EnterReadLock();
//b) before _event.WaitOne();
//c) after _readWrite.EnterReadLock();
_event.Set(); //Threads at position b start to advance
Thread.Sleep(1); //We want this thread to preempt now!
_event.Reset(); //And here we stop them
//Threads at positions a and b wait where they are
//We wait for any threads at position c
_readWrite.EnterWriteLock();
try
{
//...
}
finally
{
_readWrite.ExitWriteLock();
//Now the threads in position a continues...
// but are halted at position b
//Any thread in position b will wait until Stop is called again
}
}
Read the comments in the code to understand how it works. In simple terms, it takes advantage of a Read-Write lock to allow multiple threads to enter the method process but only one to enter Stop. Although additional work was done to ensure that the threads that are calling the method process will wait until a thread calls the method Stop.
- And now you got a reentry problem... fixing it.
The solution above is better... and that doesn't mean perfect. What's wrong with it? Well, if you call Stop recursively or if you call it from two different threads at the same time it will not work correctly because a second call may make threads at process advance while the first call is executing... and I think you don't want that. It did have the appearance that the Read-Write lock was enough to prevent any problems from multiple threads calling the method Stop, but that wasn't the case.
To solve this we need to make sure that Stop only executes once at the time. You can do that with lock:
private ManualResetEvent _event = new ManualResetEvent (false);
private ReaderWriterLockSlim _readWrite = new ReaderWriterLockSlim();
//I'm going to use _syncroot, you can use any object...
// as long as you don't lock on it somewhere else
private object _syncroot = new object();
private void process()
{
//...
_readWrite.EnterReadLock();
_event.WaitOne();
try
{
//...
}
finally
{
_readWrite.ExitReadLock();
}
}
internal void Stop()
{
lock(_syncroot)
{
//there are three relevant thread positions at the process method:
//a) before _readWrite.EnterReadLock();
//b) before _event.WaitOne();
//c) after _readWrite.EnterReadLock();
_event.Set(); //Threads at position b start to advance
Thread.Sleep(1); //We want this thread to preempt now!
_event.Reset(); //And here we stop them
//Threads at positions a and b wait where they are
//We wait for any threads at position c
_readWrite.EnterWriteLock();
try
{
//...
}
finally
{
_readWrite.ExitWriteLock();
//Now the threads in position a continues...
// but are halted at position b
//Any thread in position b will wait until Stop is called again
}
}
}
Why do we need a Read-Write lock? - you may ask - If we are using a lock to ensure that only one thread enters the method Stop...?
Because the Read-Write lock is also allowing the thread at the method Stop to stop newer threads that are calling the method process while allowing those that were there already to execute and wait until they finish.
Why do we need ManualResetEvent? - you may ask - If we already have the Read-Write lock to control the execution of the threads in the method process...?
Because Read-Write lock is not able to prevent the execution of the code in the method process before the method Stop has been called.
So, you wee we need all that... or do we?
Well, that depends on what behaviour do you have, so in case I did solve a problem that is not what you had, I offer some alternative solutions below.
- Alternative solution with an alternative behaviour
A lock is very easy to understand, but it is a bit too much for my taste... in particular if there is no need to make sure that each concurrent call to Stop has a chance to allow the execution of thread at the method process.
IF that is the case then you can rewrite the code as follows:
private ManualResetEvent _event = new ManualResetEvent (false);
private ReaderWriterLockSlim _readWrite = new ReaderWriterLockSlim();
private int _stopGuard;
private void process()
{
//...
_readWrite.EnterReadLock();
_event.WaitOne();
try
{
//...
}
finally
{
_readWrite.ExitReadLock();
}
}
internal void Stop()
{
if(Interlocked.CompareExchange(ref _stopGuard, 1, 0) == 0)
{
//there are three relevant thread positions at the process method:
//a) before _readWrite.EnterReadLock();
//b) before _event.WaitOne();
//c) after _readWrite.EnterReadLock();
_event.Set(); //Threads at position b start to advance
Thread.Sleep(1); //We want this thread to preempt now!
_event.Reset(); //And here we stop them
//Threads at positions a and b wait where they are
//We wait for any threads at position c
_readWrite.EnterWriteLock();
try
{
//...
}
finally
{
_readWrite.ExitWriteLock();
//Now the threads in position a continues...
// but are halted at position b
//Any thread in position b will wait until Stop is called again
}
}
}
Not yet the right behaviour? Ok, let's see another.
- Alternative solution with an alternative behaviour... again
This time we are going to see how to allow multiple thread to enter the method process even before the method Stop was called.
private ReaderWriterLockSlim _readWrite = new ReaderWriterLockSlim();
private int _stopGuard;
private void process()
{
//...
_readWrite.EnterReadLock();
try
{
//...
}
finally
{
_readWrite.ExitReadLock();
}
}
internal void Stop()
{
if(Interlocked.CompareExchange(ref _stopGuard, 1, 0) == 0)
{
//there are two relevant thread positions at the process method:
//a) before _readWrite.EnterReadLock();
//b) after _readWrite.EnterReadLock();
//We wait for any threads at position b
_readWrite.EnterWriteLock();
try
{
//...
}
finally
{
_readWrite.ExitWriteLock();
//Now the threads in position a continues...
// and they will continue until halted when Stop is called again
}
}
}
Not what you want?
Ok, I give up... let's get back to the basics.
- And what you already knew
...for the sake of completeness, if you only need to make sure that the access of both methods is synchronized and you can allow the methods at the process to run at any time, then you can do it with just locks... and you already knew that.
private object _syncroot = new object();
private void process()
{
//...
lock(_syncroot)
{
//...
}
}
internal void Stop()
{
lock(_syncroot)
{
//...
}
}
- Conclusion
We have seen why the deadlock happened in the first place and how to fix it, but we also discovered that the absence of deadlock is not a warranty of thread safety. Finally, we have seen three solutions (points 4, 5, 6 and 7 above) with four different behaviours and complexities. All in all, we can conclude that developing with multithreading can be a very complex task where we need to keep our goals clear and be aware of what can go wrong at every turn. You can say it ok to be a bit paranoid, and that doesn't only apply to multithreading.
