How to prevent deadlocks in the following C# code?

Question

The following C# class is used in a multithreaded enviroment. I removed very much of the actual code. The problem occurs when calling MethodA and MethodB almost simultaneously. The order of the lock in the IsDepleted property doesn't solves the problem. Removing lock(WaitingQueue) from the IsDepleted property solves the deadlock, but this solution causes a problem when another thread adds/removes an item from the WaitingQueue between the WaitingQueue.Count == 0 and Processing.Count == 0 statements.

using System.Collections.Generic;

class Example
{
    bool IsDepleted
    {
        get
        {
            lock (Processing)
            {
                lock (WaitingQueue)
  {
                   return WaitingQueue.Count == 0
    && Processing.Count == 0;
  }
            }
        }
    }

    private readonly List<object> Processing = new List<object>();
    private readonly Queue<object> WaitingQueue = new Queue<object>();

    public void MethodA(object item)
    {
        lock (WaitingQueue)
        {
            if (WaitingQueue.Count > 0)
            {
                if (StartItem(WaitingQueue.Peek()))
                {
                    WaitingQueue.Dequeue();
                }
            }
        }
    }

    public void MethodB(object identifier)
    {
        lock (Processing)
        {
            Processing.Remove(identifier);
            if (!IsDepleted)
            {
                return;
            }
        }
    //Do something...
    }

 bool StartItem(object item)
 {
  //Do something and return a value
 }
}

Answer 1

Take the Processing lock in method A and the WaitingQueue lock in method B (in other words, make it look like the first block of code). That way, you always take the locks in the same order and you'll never deadlock.

Answer 2

It depends if you want a quick fix or a rigorous fix.

A quick fix would be just to use one lock object in all cases.

e.g. private readonly object _lock = new object();

And then just lock on that. However, depending on your situation, that may impact performance more than you can accept.

I.e. your code would become this:

using System.Collections.Generic;

class Example
{
    private readonly object _lock = new object();

    bool IsDepleted
    {
        get
        {
            lock (_lock)
            {
                return WaitingQueue.Count == 0
                 && Processing.Count == 0;
            }
        }
    }

    private readonly List<object> Processing = new List<object>();
    private readonly Queue<object> WaitingQueue = new Queue<object>();

    public void MethodA(object item)
    {
        lock (_lock)
        {
            if (WaitingQueue.Count > 0)
            {
                if (StartItem(WaitingQueue.Peek()))
                {
                    WaitingQueue.Dequeue();
                }
            }
        }
    }

    public void MethodB(object identifier)
    {
        lock (_lock)
        {
            Processing.Remove(identifier);
            if (!IsDepleted)
            {
                return;
            }
        }
        //Do something...
    }

    bool StartItem(object item)
    {
        //Do something and return a value
    }
}

Answer 3

Simplify your code and use only a single object to lock on. You could also replace your locks with:

Monitor.TryEnter(Processing,1000)

this will give you a 1 second timeout. So essentially:

        if (Monitor.TryEnter(Processing, 1000))
        {
            try
            {
                //do x
            }
            finally
            {
                Monitor.Exit(Processing);
            }
        }

Now you won't stop the deadlock but you can handle the case where you don't get a lock.