C# Deadlock Calling Locked Methods

Question

The following code causes deadlock when I hit rbtn1 followed by rbtn2. Rbtn2 is asynchronous and when I only click rbtn1 multiple times it is ok. Rbtn2 synchronous and when I only click rbtn2 multiple times it is also ok. But when I mix them, the deadlock occurs. Why is this?

    private void rbtn1_Click(object sender, EventArgs e)
    {
        Task.Run(() => UpdateDisplayLock("a"));
    }

    private void radButton2_Click(object sender, EventArgs e)
    {
        UpdateDisplayLock("a");
    }

    private object _lockKey = new object();
    private void UpdateDisplayLock(string i)
    {
        lock (_lockKey)
        {
            Interlocked.Increment(ref _uniqueId);
            var uniqueId = _uniqueId;
            Invoke((Action)delegate
            {
                rlblDisplay.Text += Environment.NewLine + uniqueId + string.Format(":{0} Start;", i);
            });
            Thread.Sleep(5000);
            Invoke((Action)delegate
            {
                rlblDisplay.Text += Environment.NewLine + uniqueId + string.Format(":{0} End;", i);
            });
        }
    }

How do I resolve this? Or is this just a bad practice to have a method be called both asynchronously and synchronously? If so, is there a way to limit methods with locks to be used asynchronously only?

Answer 1

In UpdateDisplayLock you're invoking to the UI thread. So when you call this method from another thread after pressing the first button it needs to periodically access the UI thread to be able to continue on.

When you call UpdateDisplayLock when clicking the second button it hits the lock, and since the background process is holding it, it's just going to sit there and wait. You're now blocking the UI thread until the first process is done (so it can release the lock).

When the background thread running UpdateDisplayLock goes to invoke an action in the UI thread it sits there waiting for the work to be scheduled in the UI thread. The second button click is sitting there waiting on you, blocking the UI thread.

You now have two threads, each waiting on the other. Deadlock.

---

As for how to address the problem, the best solution is to make UpdateDisplayLock an inherently asynchronous operation, rather than an inherently synchronous operation that you may or may not call from another thread:

private async Task UpdateDisplayLock(string i)
{
    _uniqueId++;
    var uniqueId = _uniqueId;
    rlblDisplay.Text += Environment.NewLine + 
        uniqueId + string.Format(":{0} Start;", i);
    await Task.Delay(TimeSpan.FromSeconds(5));
    rlblDisplay.Text += Environment.NewLine + 
        uniqueId + string.Format(":{0} End;", i);
}

Note that in this implementation it will allow multiple calls to interweave their start and end calls, but since the increment and the UI manipulation is all in the UI thread, there won't be any threading errors. If you don't want any subsequent calls to be able to start their operation/logging until the previous one(s) finish, then you can use a SemaphoreSlim to do that asynchronosly:

private SemaphoreSlim semaphore = new SemaphoreSlim(1);
private async Task UpdateDisplayLock(string i)
{
    await semaphore.WaitAsync();
    try
    {
        _uniqueId++;
        var uniqueId = _uniqueId;
        rlblDisplay.Text += Environment.NewLine +
            uniqueId + string.Format(":{0} Start;", i);
        await Task.Delay(TimeSpan.FromSeconds(5));
        rlblDisplay.Text += Environment.NewLine +
            uniqueId + string.Format(":{0} End;", i);
    }
    finally
    {
        semaphore.Release();
    }
}

You can then await this asynchronous method form your event handlers, if you have stuff to do, or you can just call it if you don't need to do anything after it finishes:

private async void rbtn1_Click(object sender, EventArgs e)
{
    await UpdateDisplayLock("a");
    DoSomethingElse();
}

private void radButton2_Click(object sender, EventArgs e)
{
    var updateTask = UpdateDisplayLock("a");
}

Answer 2

If you press button two the thread hits the sleep and has the lock, if you now press button one it hits the lock statment keeping the GUI thread locked down (the GUI is dead now) now the first thread (which has the lock) finishes the sleep statement starting the Invoke. The Invoke does now wait untill the GUI thread runs the action. Here the deadlock occures, Invoke does block the current thread untill the GUI thread is able to process your request. Which will never happen cause you locked the GUI thread down.

Easiest way to get around the deadlock is using BeginInvoke, which does not block the current thread until the Action is completed. But will still result in strange behavior as your GUI update may be delayed by the time the operation runs, leading to unexpected behavior.

The real problem lies in the fact that you run a 5 second operation in the GUI thread. This leads to awful user experience. And problems when mixing threads with invoke. Better implement the UpdateDisplayLock as Async and use a SemaphoreSlim to synchronize multi threading as Servy posted in his answer.