Can ProgressBar cause the application to go into racing condition?

Question

Description (.Net framework 3.5/Windows7/VS12) : 1) Operation is a type that can be run asynchronously. It notifies other objects through it's event named Executed , as follows :

_Operation.Executed += OperationExecuted;

2) Inside this event, we call the StopProgress() as follows :

private void OperationExecuted (object sender, OperationEventArgs e)
{
    StopProgress(); 
}

3) The StopProgress() is as shown below :

public void StopProgress()
{
    if (InvokeRequired)
    {
        Invoke(new MethodInvoker(StopProgress));
        return;
    }

    lblTemp.Text = "operation complete. ";// 1
    //progressBar1.Visible = false; // 2

    // with added locks the app totally hangs
    //lock (progressBar1)
    //{
    //    progressBar1.Visible = false;
    //}
}

When commenting line marked "1" (inside StopProgress() ), and uncommenting "2" (which is the desired behavior), we run into racing condition occasionally(after running the app for 5-10 times, we encounter racing conditions). With line "1", it never happens. There is no exceptions thrown(caught/uncaught) either. We are assuming the problem is related to the "ProgressBar" itself. If not, what could probably be the case here . Any suggestions on how to track down the racing conditions (vulnerable code sections) are also very much appreciated. Thanks.

Any memory accessed by multiple threads is vulnerable to racing condition. Tracking them down is a matter of investigating your code. When you need to coordinate threads, `locks` are really easy to use. — Candide, Jun 24 '13 at 09:24
@Candide I tried adding the lock, but then my application goes hangs(maybe a deadlock [not sure though]) !!! — MrClan, Jun 24 '13 at 09:30
Can you update your question with the code with locks you wrote? — Candide, Jun 24 '13 at 09:45
@Candide yeah, there it is. But more importantly, the app never hangs with lblTemp.Text, but then why is there an issue with ProgressBar ??? — MrClan, Jun 24 '13 at 09:50
Everything looks good to me, I even went so far to test the situation out with this: http://ideone.com/e7FQpv and I cannot find any kind of issues that arise from concurrent access to the `Visible` property, however, the issue still exists in your system. — Candide, Jun 24 '13 at 10:02
@Candide yes, and not just on my system, it occurs on all other systems of our team as well. It occurs randomly though (sometimes the app may have to run for about 15-20 times to regenerate this situation). — MrClan, Jun 24 '13 at 10:10

score 2 · Accepted Answer · edited May 23 '17 at 10:25

Hard to explain this one, but there are some strong anti-patterns in this code that can induce deadlock.

First off, you are using Control.Invoke() to ensure that the progress bar update occurs on the UI thread. It is therefore completely unnecessary to use the lock statement since you are already know that updates only ever happen on one thread. So remove the lock.

Using Control.Invoke() is a bad anti-pattern. It is particularly prone to cause deadlock since it cannot complete until the UI thread has executed the delegate target. Deadlock will occur when there's other code somewhere that runs on the UI thread that waits for the thread to complete. That will never happen, the thread is stuck in the Invoke() call which can't complete until the UI thread goes idle. The UI thread cannot get idle, it is stuck waiting for the thread to complete. Deadlock city. Use Invoke() only when absolutely necessary and fear its ability to cause deadlock. Always favor BeginInvoke() instead, it cannot cause deadlock since it doesn't wait. And you don't need Invoke() here, you don't need to know its return value.

Using Control.InvokeRequired is an anti-pattern as well. You almost always know that a method is called from a worker thread. So no point in testing InvokeRequired, you expect it to be true. Very Bad Things happen when it is false. Which is very possible, it will happen when the user closes the form and the worker thread is allowed to continue executing. This will normally cause your code to crash with a ObjectDisposedException. But your code doesn't get there, it is liable to deadlock before that happens, either on the Invoke call or the lock. You should use InvokeRequired, but throw an InvalidOperationException when it is false. Now you added a diagnostic that gives you a chance to find out what is going wrong.

Get ahead by using a .NET class that already takes care of these things. BackgroundWorker does everything you are trying to do by hand. You will now also have a shot at dealing with the problem of the user closing the form with the worker thread still executing. The subject of this answer.

Quoting : "It is particularly prone to cause deadlock since it cannot complete until the UI thread has executed the delegate target. Deadlock will occur when there's other code somewhere that runs on the UI thread that waits for the thread to complete." Thank you very much. Now we are using timer instead. Hope that's not anti-pattern, or is it ??? :) — MrClan, Jun 24 '13 at 11:43
A timer? Timers are not good at showing progress, sounds like you are opening the door to a threading race. Use a BGW as I recommended. — Hans Passant, Jun 24 '13 at 11:46
Today, we implemented the same using BGW, but we again encountered the same old situation. — MrClan, Jun 25 '13 at 08:21

score 0 · Answer 2 · answered Jun 24 '13 at 09:23

0

To avoid this, most probabbly, you can put the code

progressBar1.Visible = false

just inside StopProgress(..) method.

The point here, imo, is that in case of InvokeRequired, the StopProgress(..) will be called and contemporary (it's multithreading) you may run the code progressBar1.Visible = false;. This may produce undesirable results.

So to avoid this "destribution", move progressBar1.Visible = false; into method itself, if this is possible.

Hope this helps.

answered Jun 24 '13 at 09:23

Tigran

61,654
8
86
123

It is already inside the StopProgress() method. Do you mean, that I should place it before the check for InvokeRequired ??? If so, I get cross-thread exception. – MrClan Jun 24 '13 at 09:32
@MrClan: yes, that's right. Or do not make recursive calls, but create anothe method and call Invoke on *that* method. – Tigran Jun 24 '13 at 09:49
Importantly, the app never hangs with lblTemp.Text, but then why is there an issue with ProgressBar ??? – MrClan Jun 24 '13 at 09:52
@MrClan: this *may* be related to ProgressBar control internals itself. How it's implemented. – Tigran Jun 24 '13 at 09:56

Can ProgressBar cause the application to go into racing condition?

2 Answers2