views:

286

answers:

6

I use BackgroundWorker most of the time in the win form apps to show progress as I'm getting data. I was under impression that Work_completed is guaranteed to be executed on Main UI thread but it's not. If we create a thread and call the worker.RunWorkerAsync within it, it breaks if we try to update any gui control. Here is an example

private void StartButton_Click(object sender, EventArgs e)
{
    Thread thread1 = new Thread(new ThreadStart(PerformWorkerTask));
    _worker = new BackgroundWorker();
    thread1.Start();
}

public void PerformWorkerTask()
{
    _worker.DoWork += delegate
    {
     for (int i = 0; i < 10; i++)
     {
      Thread.Sleep(100);
     }
    };

    _worker.RunWorkerCompleted += delegate
    {
     // this throws exception
     MessageLabel.Text = "Completed";
    };
    _worker.RunWorkerAsync();

 }

How can we make backgroundworker work in this case?

A: 

In the code you have presented here, you're adding the delegates for the BackgroundWorker events in a separate thread from the UI thread.

Try adding the event handlers in the main UI thread, and you should be okay.

Mike
A: 

I believe BackgroundWorker is designed to automatically utilize a new thread. Therefore creating a new thread just to call RunWorkerAsync is redundant. You are creating a thread just to create yet another thread. What's probably happening is this:

  1. You create a new thread from thread 1 (the GUI thread); call this thread 2.
  2. From thread 2, you launch RunWorkerAsync which itself creates yet another thread; call this thread 3.
  3. The code for RunWorkerCompleted runs on thread 2, which is the thread that called RunWorkerAsync.
  4. Since thread 2 is not the same as the GUI thread (thread 1), you get an illegal cross-thread call exception.

(The below suggestion uses VB instead of C# since that's what I'm more familiar with; I'm guessing you can figure out how to write the appropriate C# code to do the same thing.)

Get rid of the extraneous new thread; just declare _worker WithEvents, add handlers to _worker.DoWork and _worker.RunWorkerCompleted, and then call _worker.RunWorkerAsync instead of defining a custom PerformWorkerTask function.

EDIT: To update GUI controls in a thread-safe manner, use code like the following (more or less copied from this article from MSDN):

delegate void SetTextCallback(System.Windows.Forms.Control c, string t);

private void SafeSetText(System.Windows.Forms.Control c, string t)
{
  if (c.InvokeRequired)
  {
    SetTextCallback d = new SetTextCallback(SafeSetText);
    d.Invoke(d, new object[] { c, t });
  }

  else
  {
    c.Text = t;
  }
}
Dan Tao
I used thread1 just to mimic a thread that is performing some background work. Yes in this example it's not doing anything but in real world, it's a call from a seperate component that is informing me and is initiated from a non ui thread.
Sheraz
See my edit above. I think this code should achieve the behavior you're looking for.
Dan Tao
+1  A: 

RunWorkerAsync does its thread-synchronization magic by getting the SynchronizationContext from the thread that it is called on. It then guarantees that the events will be executed on the correct thread according to the semantics of the SynchronizationContext it got. In the case of the WindowsFormsSynchronizationContext, which is what is automatically used if you're using WinForms, the events are synchronized by posting to the message queue of the thread that started the operation. Of course, this is all transparent to you until it breaks.

EDIT: You MUST call RunWorkerAsync from the UI thread for this to work. If you can't do it any other way, your best bet is to invoke the beginning of the operation on a control so that the worker is started on the UI thread:

private void RunWorker()
{
    _worker = new BackgroundWorker();

    _worker.DoWork += delegate 
    {
        // do work
    };

    _worker.RunWorkerCompleted += delegate 
    {
        MessageLabel.Text = "Completed";
    };

    _worker.RunWorkerAsync();
}

// ... some code that's executing on a non-UI thread ...
{
    MessageLabel.Invoke(new Action(RunWorker));
}
Neil Williams
No it's not redundant. It's just to represent the case in simple way. Please read my comments I left in Dan's response.
Sheraz
Already edited ;)
Neil Williams
A: 

You could probably make your existing code work by doing:

this.Dispatcher.BeginInvoke(() => MessageLabel.Text = "Completed")

instead of

MessageLabel.Text = "Completed"

You're probably having cross-thread data access issues, so you have to ensure that you access properties of MessageLabel on your UI thread. This is one way to do that. Some of the other suggestions are valid too. The question to ask yourself is: why are you creating a thread that does nothing other than create a BackgroundWorker thread? If there's a reason, then fine, but from what you've shown here there's no reason you couldn't create and start the BackgroundWorker thread from your event handler, in which case there would be no cross-thread access issue because the RunWorkerCompleted event handler will call its delegates on the UI thread.

Ben Collins
A: 

It sounds like the issue is just that you want to make a change to a GUI component and you aren't actually sure if you're on the GUI thread. Dan posted a valid method of setting a GUI component property safely, but I find the following shortcut method the simplest:

            MessageLabel.Invoke(
                (MethodInvoker)delegate
                {
                    MessageLabel.Text = "Hello World";
                });

If there are any issues with this approach, I'd like to know about them!

genki
no there is no issue with this approach but the only thing is that it's not a generalized approach. I'm in the middle of writing a general component that'd do the job without any knowledge of anything (just making life easier)
Sheraz
A: 

The best way to deal with these generic problems is to deal it once. Here I'm posting a small class that wraps the backgroupdworker thread and makes sure that the workcompleted always gets executed on the UI thread.


using System.Windows.Forms; namespace UI.Windows.Forms.Utilities.DataManagment { public class DataLoader { private BackgroundWorker _worker; private DoWorkEventHandler _workDelegate; private RunWorkerCompletedEventHandler _workCompleted; private ExceptionHandlerDelegate _exceptionHandler; public static readonly Control ControlInvoker = new Control();
     public DoWorkEventHandler WorkDelegate
    {
        get { return _workDelegate; }
        set { _workDelegate = value; }
    }

    public RunWorkerCompletedEventHandler WorkCompleted
    {
        get { return _workCompleted; }
        set { _workCompleted = value; }
    }

    public ExceptionHandlerDelegate ExceptionHandler
    {
        get { return _exceptionHandler; }
        set { _exceptionHandler = value; }
    }

    public void Execute()
    {
        if (WorkDelegate == null)
        {
            throw new Exception(
                "WorkDelegage is not assinged any method to execute. Use WorkDelegate Property to assing the method to execute");
        }
        if (WorkCompleted == null)
        {
            throw new Exception(
                "WorkCompleted is not assinged any method to execute. Use WorkCompleted Property to assing the method to execute");
        }

        SetupWorkerThread();
        _worker.RunWorkerAsync();
    }

    private void SetupWorkerThread()
    {
        _worker = new BackgroundWorker();
        _worker.WorkerSupportsCancellation = true;
        _worker.DoWork += WorkDelegate;
        _worker.RunWorkerCompleted += worker_RunWorkerCompleted;

    }

    void worker_RunWorkerCompleted(object sender, RunWorkerCompletedEventArgs e)
    {
        if(e.Error !=null && ExceptionHandler != null)
        {
            ExceptionHandler(e.Error);
            return;
        }
        ControlInvoker.Invoke(WorkCompleted, this, e);
    }
}

}

And here is the usage. One thing to note is that it exposes a static property ControlInvoker that needs to be set only once (you should do it at the beginning of the app load)

Let's take the same example that I posted in question and re write it

DataLoader loader = new DataLoader(); loader.ControlInvoker.Parent = this; // needed to be set only once

private void StartButton_Click(object sender, EventArgs e) {

Thread thread1 = new Thread(new ThreadStart(PerformWorkerTask));
_worker = new BackgroundWorker();
thread1.Start();

}

public void PerformWorkerTask() {

        loader.WorkDelegate = delegate { 
                                         // get any data you want 
                                         for (int i = 0; i < 10; i++)
                                        {
                                            Thread.Sleep(100);
                                        }

                                       };
        loader.WorkCompleted = delegate
                                   {
                                       // access any control you want 
                                       MessageLabel.Text = "Completed";

                                   };
        loader.Execute();

}

Cheers

Sheraz