2

I try to perform an easy task in an other backgroundthread, so the UI doesn't get blocked, but it still gets blocked. Did I forget anything?

public partial class backgroundWorkerForm : Form
{
    public backgroundWorkerForm()
    {
        InitializeComponent();
    }

    private void doWorkButton_Click(object sender, EventArgs e)
    {
        if (backgroundWorker.IsBusy != true)
        {
            // Start the asynchronous operation.
            backgroundWorker.RunWorkerAsync();
        }
    }

    private void backgroundWorker_DoWork(object sender, DoWorkEventArgs e)
    {
        //BackgroundWorker worker = sender as BackgroundWorker;
        if (textBoxOutput.InvokeRequired)
        {
            textBoxOutput.Invoke(new MethodInvoker(delegate
            {
                for (int i = 0; i < 10000; i++)
                {
                    textBoxOutput.AppendText(i + Environment.NewLine);
                }
            }));
        }
    }
}

While the textBox gets filled, the UI is blocked:

enter image description here

eMi
  • 5,540
  • 10
  • 60
  • 109
  • 1
    No wonder, you have the loop that does the work inside your Invoke (which gets invoked on the UI thread) – Icepickle Jan 05 '15 at 13:05
  • 3
    The only thing your background worker does is dispatch the entire loop to the UI thread an execute it there. – Ben Robinson Jan 05 '15 at 13:06
  • `textBoxOutput.Invoke` will execute the delegate synchronously in UI thread and thus it blocks. It's not clear what you're trying to achieve but apparently you don't need a `BackgroundWorker` at all. – Sriram Sakthivel Jan 05 '15 at 13:07
  • I think you need to separate background working processing and UI updation. Why can't use OnRunWorkerCompleted for UI updation? – Amit Jan 05 '15 at 13:21
  • One idea is to generate the string in memory and update it once to `TextBox`. That will perform better but hard to answer without knowing what you're trying to achieve. – Sriram Sakthivel Jan 05 '15 at 13:23

3 Answers3

4

Your app wants to repeatedly send updates from the background thread to the UI. There is a built-in mechanism for this: the ProgressChanged event for the background worker. A ReportProgress call is triggered in the background, but executes on the UI thread.

I do change one thing, however. Performance can degrade with too many cross-thread calls. So instead of sending an update every iteration, I instead will batch them into 100.

    private void backgroundWorker1_DoWork(object sender, DoWorkEventArgs e)
    {
        const int maxIterations = 10000;
        var progressLimit = 100;
        var staging = new List<int>();
        for (int i = 0; i < maxIterations; i++)
        {
            staging.Add(i);
            if (staging.Count % progressLimit == 0)
            {
                // Only send a COPY of the staging list because we 
                // may continue to modify staging inside this loop.
                // There are many ways to do this.  Below is just one way.
                backgroundWorker1.ReportProgress(staging.Count, staging.ToArray());
                staging.Clear();
            }
        }
        // Flush last bit in staging.
        if (staging.Count > 0)
        {
            // We are done with staging here so we can pass it as is.
            backgroundWorker1.ReportProgress(staging.Count, staging);
        }
    }

    // The ProgressChanged event is triggered in the background thread
    // but actually executes in the UI thread.
    private void backgroundWorker1_ProgressChanged(object sender, ProgressChangedEventArgs e)
    {
        if (e.ProgressPercentage == 0) return;
        // We don't care if an array or a list was passed.
        var updatedIndices = e.UserState as IEnumerable<int>;
        var sb = new StringBuilder();
        foreach (var index in updatedIndices)
        {
            sb.Append(index.ToString() + Environment.NewLine);
        }
        textBoxOutput.Text += sb.ToString();
    }

EDIT:

This requires you set the background worker's WorkerReportsProgress property to true.

It's not important that you pass a count with the ReportProgress call. I do so just to have something and to quickly check if I can return.

One really should keep in mind about how many events are being invoked and queued up. Your original app had 10,000 cross thread invocations and 10,000 changed text events for textBoxOutput. My example uses 100 cross thread calls since I use a page size of 100. I could still have generated 10,000 changed text events for the textbox, but instead use a StringBuilder object to hold a full page of changes and then update the textbox once for that page. That way the textbox only has 100 update events.

EDIT 2

Whether or not your app needs paging is not the main deal. The biggest take away should be that the background worker really should use ReportProgress when trying to communicate info back to the UI. See this MSDN Link. Of particular note is this:

You must be careful not to manipulate any user-interface objects in your DoWork event handler. Instead, communicate to the user interface through the ProgressChanged and RunWorkerCompleted events.

Rick Davin
  • 1,010
  • 8
  • 10
  • this seems fine, for this particular test program that I posted, but in my real application I cannot use this approach, as I don't know how many update events will happen. – eMi Jan 05 '15 at 15:55
  • Using my approach (1) the background worker does not block the UI, and (2) minimizes cross thread calls as well as textbox TextChanged calls particularly in cases where you don't know how many update events will happen. How is this not helpful? – Rick Davin Jan 05 '15 at 16:05
  • I don't have (and can't have) variables: `maxIterations` , `progressLimit`, nor I need to report any progress. It is maybe helpful for this particularly piece of code I posted, but I cannot adapt your approach into my application. Anyway I'l give you a +1 for your effort, thanks – eMi Jan 05 '15 at 16:13
2

Your invocation code should be outside the loop. Everything in the invoked codeblock, will be executed on the UI thread, thus blocking it.

    private void backgroundWorker_DoWork(object sender, DoWorkEventArgs e)
    {
        //BackgroundWorker worker = sender as BackgroundWorker;
        for (int i = 0; i < 10000; i++)
        {
            // do long-running task

            //if (textBoxOutput.InvokeRequired)
            //{
                textBoxOutput.Invoke(new MethodInvoker(delegate
                {
                    textBoxOutput.AppendText(i + Environment.NewLine);
                }));
            //}
        }
    }
John
  • 3,627
  • 1
  • 12
  • 13
  • 1
    Not sure why this was upvoted. This code is not going to improve anything, rather it makes it even worse. -1 – Sriram Sakthivel Jan 05 '15 at 13:21
  • 1
    @sriram-sakthivel: you should try the code. It doesn't block the UI any more. – John Jan 05 '15 at 13:31
  • 1
    Out of interest, why do you call `InvokeRequired` inside the loop? What do you believe will change between each loop iteration? – Daniel Kelley Jan 05 '15 at 13:38
  • Maybe [newer version of operating systems does this](http://stackoverflow.com/questions/25919845/why-does-my-form-not-lock-up), but this code still uses UI thread to update. Will not work in all operating systems – Sriram Sakthivel Jan 05 '15 at 13:43
1

an easier way would be to do completely create your output text, and then paste the full output into the TextBox, then you only need one invocation

protected delegate void SetTextDelegate(TextBox tb, string Text);

protected void SetText(TextBox tb, string Text)
{
    if (tb.InvokeRequired) {
        tb.Invoke(new SetTextDelegate(SetText), tb, Text);
        return;
    }
    tb.Text = Text;
}

and then inside your dowork

private void backgroundWorker_DoWork(object sender, DoWorkEventArgs e)
{
    StringBuilder sb = new StringBuilder();
    //BackgroundWorker worker = sender as BackgroundWorker;
    for (int i = 0; i < 10000; i++)
    {
         sb.AppendLine(i.ToString());
    }
    SetText(textBoxOutput, sb.ToString());
}
Icepickle
  • 12,689
  • 3
  • 34
  • 48
  • 2
    What does this changes and how does this answers the question? – Sriram Sakthivel Jan 05 '15 at 13:09
  • @SriramSakthivel , if you put a `Thread.Sleep(100);` in my code after the `AppendText`, the UI will be blocked, and I have to wait until all 10000 numbers are added, but with this code it doesnt block – eMi Jan 05 '15 at 13:13
  • Try running your code and see whether UI blocks or not. This code does exactly what OP did in a rather complicated way. – Sriram Sakthivel Jan 05 '15 at 13:14
  • @SriramSakthivel what changes is the fact that it gets invoked 10000 times (2 threads) and so it should run smoother, where as in the OP start post, the full loop was executed in one invoke (thus the UI thread didn't have a chance to update) – Icepickle Jan 05 '15 at 13:14
  • actually I have to implement this in my real application, this was just a small testapplication, so I can get the idea, I will write when tested – eMi Jan 05 '15 at 13:18
  • Ofcourse in a real world app with that many repeats, you need either some delay, or simply paste at once the full generated text into the Textbox, the loop is still main blocking point :) – Icepickle Jan 05 '15 at 13:19
  • 1
    This code won't help OP in anyway. This still invokes 10000 iterations in UI thread. To be honest this will take more time than OP's code due to 10000 Invoke calls. – Sriram Sakthivel Jan 05 '15 at 13:20
  • @SriramSakthivel This i also indicated inside the comments, i think here i mainly pointed out that it is false to invoke your work method inside a background thread, as it is sent back to the background thread, there are ofcourse better ways to reach the endresult, like assembling the full text at once, and sending it at once to the textbox, i agree, but that was also not what was asked – Icepickle Jan 05 '15 at 13:22
  • @SriramSakthivel, alright, updated it to make it as it could/should be – Icepickle Jan 05 '15 at 13:26
  • Alright, I removed my downvote reconsidering the answer. – Sriram Sakthivel Jan 05 '15 at 13:29