Is this a safe way to update the UI Thread in a Windows Forms application?

Question

I'm not super familiar with multithreading, and want to know if this is the safest way to update the UI thread from a different thread. The workflow of my code is as follows:

// this is the button click action
private async void button_Click(object sender, EventArgs e)
{
    //do some things to local variables

    await create();
}

// this task creates the thing and does all the heavy processing
public Task create()
{
    return Task.Run(() => 
    {
        try
        { 
            //some code

            consoleOut(string);
        }
        catch (Exception e)
        {
            //do things
        }
    }
}

// custom logging that prints formatted stuff out to a ListBox
public void consoleOut(String str)
{
    if (this.InvokeRequired)
    {
        this.Invoke(
            new MethodInvoker(
            delegate() { consoleOut(str); }));
    }
    else
    {
        //print stuff
        ListBox.Items.Add(str);
    }
}

Is this the safest way to update the contents of my ListBox from create Task?

For reference I combined things from these previous questions, but there wasn't a lot of explanation on what the code did, hence my question about thread safety and if there are better ways:

Make wpf UI responsive when button click is performed Cross-thread operation not valid

Async/await is [not about multithreading](http://blogs.msdn.com/b/ericlippert/archive/2010/10/29/asynchronous-programming-in-c-5-0-part-two-whence-await.aspx). You happen to invoke multithreading because you explicitly use `Task.Run()`, but I doubt you should. — GSerg, Sep 01 '15 at 15:47
What would an alternative to `Task.Run()` be in this scenario then? I only have it written this way because it compiled :) — zakparks31191, Sep 01 '15 at 15:50
Keep the `button_Click` as is, make `create` `async`, remove `return Task.Run(() => {})`, put actual code in there (which should consist of calls to other `async` methods that you will `await`), followed by `ListBox.Items.Add(str);`. Delete `consoleOut` completely. — GSerg, Sep 01 '15 at 15:55
Hmm that makes sense, but `consoleOut` is called multiple times in the program and is called from methods other then `create`. To remove it completely would result in lots of duplicate code in the program. I only put the `ListBox.Items.Add(str);` in the OP for brevity, it does more then that, timestamps, formatting, etc, and it is also an overloaded method — zakparks31191, Sep 01 '15 at 15:58
Then keep `consoleOut`, but remove the invoking code from it, only keep the actual `.Add()` etc. — GSerg, Sep 01 '15 at 15:59
Originally that's how i had it, but without the invoking code i get `Cross-thread operation not valid: Control 'ListBox' accessed from a thread other than the thread it was created on.` — zakparks31191, Sep 01 '15 at 16:03
Because you had `Task.Run()`. Remove that and only use `await`. — GSerg, Sep 01 '15 at 16:07
Sorry for being so noob-y, but i'm not following what you're suggesting should be removed and what should stay. Can you post your intention as an answer? Should `create` be void, or return `Task` still, etc? — zakparks31191, Sep 01 '15 at 16:09

score 0 · Answer 1 · answered Sep 01 '15 at 15:55

This is how I multithread which has worked 100% great for me... though someone will probably get on here and say it's the worst ever...

//start a second thread with parameters in this case
Thread filterThd = new Thread(() => filterLike(FilterTextBox.Text.ToLower(),column));
                filterThd.Start();

//somewhere in your thread, it updates the ui like this
Form2 f2= (Form2)System.Windows.Forms.Application.OpenForms["Form2"];
        f2.Invoke((MethodInvoker)(() => f2.DataGrid.DataSource = null));

score 0 · Answer 2 · answered Sep 01 '15 at 16:14

IMHO, there are two problems with the approach you're taking:

You're overcomplicating things - more on that later,
Depending on what the //some code section does you could end-up with a frozen application.

Now, let's tear each part in its own bits.

The over complication is due to the fact that you're mixing two different ways of doing basically the same thing - namely, the Invoke method and a Task, although the Task-based approach is incomplete.

However, the biggest problem is the part with // some code; as I said before, if that part is heavy on the resources (i.e. takes long to run) you could end up with a frozen application because the thread on which that method is running is the UI thread which consumes the UI resources otherwise allocated for the application to process messages and draw controls.

I would split the code into two logical parts:

one that does the processing and
the other one that logs the string to UI

The code should look like this:

private async void button_Click(object sender, EventArgs e)
{
    //do some things to local variables
    await Task.Run(() => 
    {
        // some code
    })
    .ContinueWith(p => ListBox.Items.Add(str),
         TaskScheduler.FromCurrentSynchronizationContext());
}

Aside from removing the clutter the code above splits the work into two tasks which can be scheduled differently: the first one can be executed on a background thread and will not affect the UI while the continuation will run on the UI thread due to the restriction passed via TaskScheduler.FromCurrentSynchronizationContext() allowing you to safely access controls and even though it is executed on the UI thread its execution time is very small thus the application won't freeze.

Thanks for this. So two things: 1) `//some code` is indeed processing intensive and will freeze/lag the gui completely (which started this mess). 2) I don't think I can have `ListBox.Add(str)` hard coded like that, because my original `ConsoleOut` is both overloaded to print various data types in the same manner, and is called multiple times throughout `//some code` — zakparks31191, Sep 01 '15 at 16:18

Is this a safe way to update the UI Thread in a Windows Forms application?

2 Answers2