CODEWITHSUNDEEP
c#winformsmultithreadingthread-safetybackgroundworker
BlueRaja - Danny Pflughoeft
BlueRaja - Danny Pflughoeft

Reputation: 85966

Using events and BackgroundWorker to update UI causing race conditions

I have two classes, Apple and Dog. Both of these classes have a method called upon startup, DoLotsOfWork() which publish an event ProgressChanged.

public class Apple
{
    //For progress bars
    public event EventHandler<ProgressChangedEventArgs> ProgressChanged;
    private void OnProgressChanged(double progress)
    {
        if(ProgressChanged != null)
            ProgressChanged(this, new ProgressChangedEventArgs((int)(progress * 100), null));
    }

    //Takes a long time to run
    public void DoLotsOfWork()
    {
        for(lots of things)
        {
            ...
            OnProgressChanged(percentage);
        }
    }
}
//Dog is similar

To keep the UI from locking up, I have these run using a BackgroundWorker. I have Apple.ProgressChanged and Dog.ProgressChanged call BackgroundWorker.ReportProgress (which calls the BackgroundWorker.ProgressChanged event) to update a label and a progress bar, so the user knows what's going on.

public class MainForm : Form
{
    private Apple _apple;
    private Dog _dog;
    private bool _isAppleCompleted;

    ...

    //Set the ProgressChanged callbacks and start the BackgroundWorker
    private void MainForm_Load(object sender, EventArgs e)
    {
        _apple.ProgressChanged += (a, args) => backgroundWorker1.ReportProgress(args.ProgressPercentage);
        _dog.ProgressChanged += (a, args) => backgroundWorker1.ReportProgress(args.ProgressPercentage);
        backgroundWorker1.RunWorkerAsync();
    }

    //Invoke the UI thread to set the status/progress
    private void SetStatus(string status)
    {
        lblStatus.Invoke((Action)(() => lblStatus.Text = status));
    }
    private void SetProgress(int progress)
    {
        progressBar.Invoke((Action)(() => progressBar.Value = progress));
    }

    private void backgroundWorker1_DoWork(object sender, DoWorkEventArgs e)
    {
        _isAppleCompleted = false;
        SetStatus("Apple (Step 1/2)");
        _apple.DoLotsOfWork();

        //Thread.Sleep(1); //This *sometimes* fixes the problem!?

        _isAppleCompleted = true;
        SetStatus("Dog (Step 2/2)");
        _dog.DoLotsOfWork();
    }

    private void backgroundWorker1_ProgressChanged(object sender, ProgressChangedEventArgs e)
    {
        //_apple.DoLotsOfWork should cause the progress bar to go from 0 to 50
        //_dog.DoLotsOfWork should cause the progress bar to go from 50 to 100
        int progress = (_isAppleCompleted ? 50 : 0) + e.ProgressPercentage/2;
        SetProgress(progress);
    }

    private void backgroundWorker1_RunWorkerCompleted(object sender, RunWorkerCompletedEventArgs e)
    {
        //stuff
    }
}

What I expect to happen: The text "Apple (Step 1/2)" as the progress bar moves from 0% to 50%. Then the phrase "Dog (Step 2/2)" is displayed as the progress bar moves from 50% to 100%.

What actually happens: Only the text "Dog (Step 2/2)" is ever displayed. The progress bar goes from 0% to 100%, then goes back to 50% and moves up to 100%.

Since I thought events handlers are run on the same thread as the event-caller; and I thought Control.Invoke() blocks until the Action is completed, I don't see how there could be any race conditions, since everything is essentially happening synchronously. Does anyone have any idea why this would be happening, and how to fix it?

And yes, I checked that 0 <= e.ProgressPercentage <= 100, and progressBar.Maximum = 100.

Upvotes: 3

Views: 1891

Answers (2)

roken
roken

Reputation: 3994

Your assumptions at the bottom are correct, though you should note that the BackgroundWorker is going to raise ProgressChanged on the thread that the control was created on, so calling backgroundWorker1.ReportProgress from the worker thread invokes the ProgressChanged handler on the UI thread (assuming that's where you created the backgroundworker), so not everything is happening synchronously. It also means the Invoke in your private SetProgress method is unnecessary.

Upvotes: 1

user804018
user804018

Reputation:

By calling the Invoke() method in your background worker, you're defeating the purpose of having the background worker. To update your progress: 1. the BackgroundWorker should call the ReportProgress method.
2. in the BackgroundWorker control on the WinForm, add a ProgressChanged handler to update your progress

Upvotes: 0

Related Questions