'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!

Could an update call in the real application cause the window to Dispose? In that case, the background thread could have a lock, and the UI thread could end up in Dispose locking on the same object the background thread is holding. — Joachim Isaksson, Jan 28 '12 at 14:39
Where are you getting the variable InvokeRequired from, it should be called on the control you are wanting to update, ie: if (label.InvokeRequired) { // } — Lloyd, Jan 28 '12 at 15:01

shf301 · Answer 1 · 2012-01-28T17:15:08.387

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

The `Invoke` call can always fail. The code will still cause an unhnadled exception to occur when Invoke is called at the precise time the Control is being disposed — JaredPar, Jan 28 '12 at 15:56
@usr What I was trying to describe was that Invoke is asynchronous with respect to the UI thread. It is synchronous with respect to the background thread. I'll try to make my answer more clear — shf301, Jan 28 '12 at 17:14

score 2 · Answer 2 · answered Jan 28 '12 at 15:43

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

Background Thread: Queues a call back with Invoke
Foreground Thread: Dispose the Control on which the background called Invoke
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";
}

score 1 · Answer 3 · answered Mar 18 '15 at 10:27

Another deadlocking scenario arises when calling Dispatcher.Invoke (in a WPF application) or Control.Invoke (in a Windows Forms application) while in possession of a lock. If the UI happens to be running another method that’s waiting on the same lock, a deadlock will happen right there. This can often be fixed simply by calling BeginInvoke instead of Invoke. Alternatively, you can release your lock before calling Invoke, although this won't work if your caller took out the lock. We explain Invoke and BeginInvoke in Rich Client Applications and Thread Affinity.

source: http://www.albahari.com/threading/part2.aspx

score 1 · Accepted Answer · answered Jan 28 '12 at 14:53

1

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.

answered Jan 28 '12 at 14:53

ken2k

48,145
10
116
176

@CodeInChaos I don't think the complexity of lock, override of Dispose...etc. is better than simply catching the exception. Maybe you could explain **why** you think it's a bad idea... – ken2k Jan 28 '12 at 14:58
@ken2k That's the wrong place to catch the exception. The exception is happens on the UI thread on the line `label.Text = "blah";` – shf301 Jan 28 '12 at 15:11
Thank you, I will go with try-catch. I agree with others that it is not the prettiest solution, but it works, and I got a deadline tomorrow. Thank you! – amulware Jan 28 '12 at 18:01

score 1 · Answer 5 · answered Jan 28 '12 at 16:43

1

Why don't you just use BeginInvoke rather than Invoke - this won't block the background thread. It doesn't look like there is any specific reason why the background thread needs to wait for the UI update to occur from what you have shown

answered Jan 28 '12 at 16:43

Richard Blewett

6,089
1
18
23

Yahia · Answer 6 · 2012-01-28T15:12:21.800

IMO Dispose is too late...

I would recommend putting some code into FormClosing which is called before Dispose happens AFAIK.

For such a case I usually tend to use a different (atomic) pattern for your check - for example via the Interlocked class.

private long _runnable = 1;

private void update(object sender, EventArgs e)
{
    if (InvokeRequired)
    {
        EventHandler handler = new EventHandler(update);
        if (Interlocked.Read (ref _runnable) == 1) Invoke(handler);
        return;
    }

    label.Text = "blah";
}

In FormClosing you just call Interlocked.Increment (ref _runnable) .

score 0 · Answer 7 · answered Jan 28 '12 at 15:10

Just incase none of the other answers are the culprit, is there other code that terminates the thread that isn't posted? I'm thinking you might be using plain Threads and not a BackgroundWorker, and might have forgotten to set Thread.isBackround to true

'Deadlock' with only one locked object?

7 Answers7

Linked