'Deadlock' with only one locked object?

Question

I am having a problem with multi threading in C#. I use an event to update a label in a form from another thread, for which I need to use the Invoke() command of course. That part is also working fine. However, the user can close the form and here the program can crash if the event is sent at an unfortunate time.

So, I thought I would simply override the Dispose() method of the form, set a boolean to true within locked code, and also check that boolean and invoke the event in locked code.

However, every time I close the form the program freezes completely.

Here are the mentioned parts of the code:

private object dispose_lock = new object();
private bool _disposed = false;

private void update(object sender, EventArgs e)
{
    if (InvokeRequired)
    {
        EventHandler handler = new EventHandler(update);
        lock (dispose_lock)
        {
            if (_disposed) return;
            Invoke(handler); // this is where it crashes without using the lock
        }
        return;
    }

    label.Text = "blah";
}

protected override void Dispose(bool disposing)
{
    eventfullObject.OnUpdate -= update;
    lock (dispose_lock) // this is where it seems to freeze
    {
       _disposed = true; // this is never called
    }
    base.Dispose(disposing);
}

I hope anyone here has any idea what is wrong with this code. Thank you in advance!

Answer 1

What you're not taking into account is that delegate passed to Invoke is called asynchronously on the UI thread. Calling Invoke posts a message to the forms message queue and is picked up some time later.

What happens is not:

UI Thread                   Background Thread
                            Call update()
                            take lock
                            Call Invoke()
Call update()             
                            release lock
Call Dispose()
take lock
release lock

But instead:

UI Thread                   Background Thread
                            Call update()
                              take lock
                               Call Invoke()
                               block until UI Thread processes the message
Process messages
...
Dispose() 
   wait for lock ****** Deadlock! *****
...
Call update()             
                            release lock

Because of this, the background thread can hold be holding the lock while the UI thread is trying to run Dispose

The solution is much simpler than what you tried. Because of the Invoke is posted asynchronously there is no need for a lock.

private bool _disposed = false;

private void update(object sender, EventArgs e)
{
    if (InvokeRequired)
    {
        EventHandler handler = new EventHandler(update);
        Invoke(handler);
        return;
    }

    if (_disposed) return;

    label.Text = "blah";
}

protected override void Dispose(bool disposing)
{
    eventfullObject.OnUpdate -= update;
    _disposed = true; // this is never called
    base.Dispose(disposing);
}

The _disposed flag is only read or written on the UI thread so there is no need for locking. Now you call stack looks like:

UI Thread                   Background Thread
                            Call update()
                               take lock
                               Call Invoke()
                               block until UI Thread processes the message
Process messages
...
Dispose() 
  _disposed = true;
...

Call update()
  _disposed is true so do nothing             

Answer 2

One of the dangers of using Control.Invoke is that it could be disposed on the UI thread at an unfortunate time as you suggested. The most common way this occurs is when you have the following order of events

1. Background Thread: Queues a call back with Invoke
2. Foreground Thread: Dispose the Control on which the background called Invoke
3. Foreground Thread: Dequeues the call back on a disposed Control

In this scenario the Invoke will fail and cause an exception to be raised on the background thread. This is likely what was causing your application to crash in the first place.

With the new code though this causes a dead lock. The code will take the lock in step #1. Then the dispose happens in the UI at step #2 and it's waiting for the lock which won't be freed until after step #3 completes.

The easiest way to deal with this problem is to accept that Invoke is an operation that can and will fail hence needs a try / catch

private void update(object sender, EventArgs e)
{
    if (InvokeRequired)
    {
        EventHandler handler = new EventHandler(update);
        try 
        {
            Invoke(handler); 
        }
        catch (Exception) 
        {
            // Control disposed while invoking.  Nothing to do 
        }
        return;
    }

    label.Text = "blah";
}

Answer 3

I really would go simple here. Instead of implementing tricky thread-safe code, I would simply catch the exception and do nothing if it fails.

Assuming it's a ObjectDisposedException:

try
{
    this.Invoke(Invoke(handler));
}
catch (ObjectDisposedException)
{
    // Won't do anything here as
    // the object is not in the good state (diposed when closed)
    // so we can't invoke.
}

It's simpler and pretty straightforward. If a comment specifies why you catch the exception, I think it's OK.