1

Environment

  • Windows 7
  • Visual Studio
  • C#

What I'm trying to do

I'm trying to build an app to evaluate company products. For security, the description below is made abstract to some extent.

What this app does is changing a certain parameter in the product and see how a certain value of the product changes. So I need to do two things.

  1. Change the parameter at a certain interval
  2. Display the value in a textbox at a certain interval

The diagram is like this.

enter image description here

These tasks should be repeated until a cancel button is pressed.

The UI has these controls:

  • button1 : start button
  • button2 : cancel button
  • textbox1 : to show values obtained from the device

So here is the code I wrote.

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

    CancellationTokenSource cts = new CancellationTokenSource();

    private async void button1_Click(object sender, EventArgs e)
    {
       
        await Task1();
        await Task2();
 
    }


    private async Task Task1()
    {
        while (!cts.IsCancellationRequested)
        {
            Thread.Sleep(500);
            ChangeParameter(0);
            Thread.Sleep(1000);
            ChangeParameter(10);
            Thread.Sleep(500);
            ChangeParameter(0);
        }
    }

    private void ChangeParameter(double param)
    {
        // change device paremeter
        Console.WriteLine("devicep parameter changed : " + param);
    }

    private async Task Task2()
    {
        while (!cts.IsCancellationRequested)
        {
            Thread.Sleep(100);
            int data = GetDataFromDevice();
            UpdateTextBoxWithData(data);
        }

        cts.Token.ThrowIfCancellationRequested();
    }

    private int GetDataFromDevice()
    {
        //pseudo code
        var rnd = new Random();
        return rnd.Next(100);
    }

    private void UpdateTextBoxWithData(int data)
    {
        textBox1.AppendText(data.ToString() + "\n");
        // debug
        Console.WriteLine("data : " + data);
    }


    private void button2_Click(object sender, EventArgs e)
    {
        cts.Cancel();
    }


}

Issues

However, there are two issues in this code.

  1. UI freezes.
  2. Task2 is never executed.

The second issue is derived from await since it executes tasks one by one. I could have used Task.Run() but this doesn't allow adding values to textBox since it's different from the UI thread.

How can I solve these issues? Any help would be appreciated.

Community
  • 1
  • 1
dixhom
  • 2,419
  • 4
  • 20
  • 36
  • 6
    Replace `Thread.Sleep(500)` with `await Task.Delay(500)` – Callum Linington Jun 29 '16 at 08:35
  • 2
    _Always_ start new threads for non-UI tasks, otherwise they run on the UI thread and take away processing power from the UI refresh. They can alert the UI when they're done running using invoke. – Nyerguds Jun 29 '16 at 08:35
  • 1
    in respect to "doesn't allow adding values to textBox since it's different from the UI thread" [this](http://stackoverflow.com/questions/661561/how-to-update-the-gui-from-another-thread-in-c) might help – Mong Zhu Jun 29 '16 at 08:36
  • 2
    @Nyerguds *don't* start a new thread for non-UI tasks. Use Tasks, `async/await`, PLINQ or Dataflow and let the framework schedule the job – Panagiotis Kanavos Jun 29 '16 at 08:36
  • 1
    Anything that touches the UI must happen on the UI thread. Look up begininvoke for examples. – Carlos Jun 29 '16 at 08:37
  • 2
    @CallumLinington no you *don't* need Invoke, if you use `async/await` and IProgress – Panagiotis Kanavos Jun 29 '16 at 08:38
  • 1
    [this](http://stackoverflow.com/questions/17870314/how-do-i-access-variables-from-a-different-thread) might also help – Mong Zhu Jun 29 '16 at 08:38
  • 1
    @PanagiotisKanavos Why would you run non-UI tasks on the UI thread though? That's simply not what it's for. – Nyerguds Jun 29 '16 at 08:39
  • 2
    @Nyerguds you are mixing up threads and async/await. If the OP had any asynchronous code, `await` would make sure that execution would return to the UI thread *after* the asynchronous code finished. If you use `await` and need to use Invoke, you are doing it wrong – Panagiotis Kanavos Jun 29 '16 at 08:41
  • 2
    @Stanley there is nothing wrong with async/await and StartNew isn't better than Task.Run. The problem is *misuse* - trying to update the UI from inside the task. Don't. Either return the result from Task.Run, or use IProgress to singal progress – Panagiotis Kanavos Jun 29 '16 at 08:42
  • 1
    @PanagiotisKanavos this [answer](http://stackoverflow.com/questions/19028374/accessing-ui-controls-in-task-run-with-async-await-on-winforms) doesn't seem to back your points up – Callum Linington Jun 29 '16 at 08:44
  • 1
    @PanagiotisKanavos I'm not mixing them up; I'd simply stick to threading. – Nyerguds Jun 29 '16 at 08:45
  • 2
    @CallumLinington on the contrary, it's exactly what I'm saying. `WriteAsync` *is* an asynchronous operation. Execution *does* return to the original UI thread *after* `await`. That's why `Invoke` isn't used in the answer – Panagiotis Kanavos Jun 29 '16 at 08:47
  • 1
    @PanagiotisKanavos Doesn't that mean it can't report progress to the UI without Invoke, though? – Nyerguds Jun 29 '16 at 08:48
  • 1
    @PanagiotisKanavos Sriram Sakthivel says that "async/await doesn't guarantee that it will run in UI thread" which is true, because if configureawait was used then you're changing the contexts – Callum Linington Jun 29 '16 at 08:49
  • 1
    @Nyerguds the answer *does* show updating the UI. Besides, reporting progress, ie communicating with another thread, is the job of the IProgress interface, used in a lot of asynchronous methods. – Panagiotis Kanavos Jun 29 '16 at 08:49
  • @PanagiotisKanavos Well, post an answer. I'm curious now :) – Nyerguds Jun 29 '16 at 08:51
  • @CallumLinington and if you wanted, you could run on a completely different TaskScheduler. But that's not what the OP asked and isn't relevant to the simple scenario of `async/await` – Panagiotis Kanavos Jun 29 '16 at 08:51
  • Who is the person going mad up-voting every single comment? Ser Pounce? –  Jun 29 '16 at 08:51
  • @PanagiotisKanavos is correct. The whole point of the new `async/await` paradigm is to make using threads easier **without** the need of `Invoke` –  Jun 29 '16 at 08:53
  • @MickyD in fact, there are so many duplicates it's hard to pick one. – Panagiotis Kanavos Jun 29 '16 at 08:55
  • @dixhorn: Pay attention to your compiler warnings. In this case, the compiler itself will tell you exactly what's wrong with the code. – Stephen Cleary Jun 29 '16 at 14:16

3 Answers3

3

The problem is you not using await in your tasks so they executing synchronously.

You should use something like this to maintain your UI responsive (NOTE this is not production code, I'm just showing an idea):

private void button1_Click(object sender, EventArgs e)
{
    try
    {
        await Task.WhenAll(Task1(cts.Token), Task2(cts.Token));
    }
    catch (TaskCancelledException ex)
    {
    }
}

private async Task Task1(CancellationToken token)
{
    while (true)
    {
        token.ThrowIfCancellationRequested();
        await Task.Delay(500, token); // pass token to ensure delay canceled exactly when cancel is pressed
        ChangeParameter(0);
        await Task.Delay(1000, token);
        ChangeParameter(10);
        await Task.Delay(500, token);
        ChangeParameter(0);
    }
}

private async Task Task2(CancellationToken token)
{
    while (true)
    {
        token.ThrowIfCancellationRequested();
        await Task.Delay(100, token);
        int data = await Task.Run(() => GetDataFromDevice()); //assuming this could be long running operation it shouldn't be on ui thread
        UpdateTextBoxWithData(data);
    }
}

Basically, when you need to run something on background you should wrap that in Task.Run() and then await for result. Simply adding async to your method won't make this method asynchronous.

To make your code clearer, I suggest you to move methods like GetDataFromDevice or ChangeParameter to services layer. Also, take a look at IProgress as comments suggests to update your UI according to progress of some process.

Maxim Kosov
  • 1,930
  • 10
  • 19
3

There are many issues with this code:

  1. async/await doesn't make the code asynchronous automagically. It allows you to await the results of already asynchronous operations. If you want to run something in the background that isn't already asynchronous, you need to use Task.Run or a similar method to start a Task.
  2. await returns execution to the original synchronization context. In this case, the UI thread. By using Thread.Sleep, you are freezing the UI thread
  3. You can't update the UI from another thread and that goes for Tasks too. You can use the IProgress interface though to report progress. A lot of BCL classes use this interface, just like CancellationToken

Maxim Kosov already cleaned up the code and shows how to properly use async/await and Task.Run, so I'll just post how to use IProgress< T> and its impelementation, Progress< T>

IProgress is used to publich a progress update with the IProgress< T>.Report method. Its default implementation, Progress, raises the ProgressChanged event and/or calls the Action<T> passed to its constructor, on the UI thread. Specifically, on the synchronization context captured when the class was created.

You can create a progress object in your constructor or your button click event, eg

private async void button1_Click(object sender, EventArgs e)
{
    var progress=new Progress<int>(data=>UpdateTextBoxWithData(data));

    //...
    //Allow for cancellation of the task itself
    var token=cts.Token;
    await Task.Run(()=>MeasureInBackground(token,progress),token);

}


private async Task MeasureInBackground(CancellationToken token,IProgress<int> progress)
{
    while (!token.IsCancellationRequested)
    {
        await Task.Delay(100,token);

        int data = GetDataFromDevice(); 
        progress.Report(data);
    }
}

Note that using Thread.Sleep inside a task is not a good idea because it wastes a threadpool thread doing nothing. It's better to use await Task.Delay() which requires that the signature of the method change to async Task. There is a Task.Run(Func) overload just for this purpose.

The method is a bit different from Maxim Kosov's code to show that IProgress really communicates across threads. IProgress can handle complex classes, so you could return both a progress percentage and a message, eg:

private async Task MeasureInBackground(CancellationToken token,IProgress<Tuple<int,string>> progress)
{
    while(!token.IsCancellationRequested)
    {
        await Task.Delay(100,token);
        int data = GetDataFromDevice(); 
        progress.Report(Tuple.Create(data,"Working"));
    }
    progress.Report(Tuple.Create(-1,"Cancelled!"));
}

Here I'm just being lazy and return a Tuple<int,string>. A specialized progress class would be more appropriate in production code.

The advantage of using an Action is that you don't need to manage event handlers and the objects are local to the async method. Cleanup is performed by .NET itself.

If your device API provides truly asynchronous calls, you don't need Task.Run. This means that you don't have to waste a Task in a tigh loop, eg:

private async Task MeasureInBackground(CancellationToken token,IProgress<Tuple<int,string>> progress)
{
    while(!token.IsCancellationRequested)
    {
        await Task.Delay(100, token);
        int data = await GetDataFromDeviceAsync(); 
        progress.Report(Tuple.Create(data,"Working"));
    }
    progress.Report(Tuple.Create(-1,"Cancelled!"));
}

Most drivers perform IO tasks using an OS feature called completion ports, essentially callbacks that are called when the driver completes an operation. This way they don't need to block while waiting for a network, database or file system response.

EDIT

In the last example, Task.Run is no longer needed. Just using await would be enough:

await MeasureInBackground(token,progress);
Panagiotis Kanavos
  • 120,703
  • 13
  • 188
  • 236
  • I don't get why `await Task.Run(()=>MeasureInBackground(token,progress),token);`. Since `MeasureInBackground` is `async`, initial Task will end with the very first `await` inside MeasureInBackground. You either need to use `async` lambda or just remove `Task.Run` and simply call `await MeasureInBackground` – Maxim Kosov Jun 29 '16 at 09:37
  • @MaximKosov not in this example. As I said, the purpose was to show how `IProgress` worked from a different thread. If you could use `await` inside `MeasureInBackground`, you don't strictly need `IProgress< T>`. The *second* method doesn't need `Task.Run`, just `await MeasureInBackground(...);` – Panagiotis Kanavos Jun 29 '16 at 09:41
  • You actually use await in the first example in `await Task.Delay()` :) Don't this should be `Thread.Sleep`? – Maxim Kosov Jun 29 '16 at 09:45
  • `Task.Run` isn't needed, unless you want it to run on a background thread – Callum Linington Jun 29 '16 at 09:47
  • @MaximKosov no, `Thread.Sleep` would waste a pool thread. `await Task.Delay()` releases the thread for other tasks to use – Panagiotis Kanavos Jun 29 '16 at 09:48
  • @CallumLinington what do you mean? The default behaviour of tasks is to run on background threads from a threadpool. You can change the TaskSchedulre used but that's *not* common usage. In fact, there are reasons to prefer `Task.Run` over `Task.Factory.StartNew`. If the method does IO work and an async API is available, you don't need to start a thread at all – Panagiotis Kanavos Jun 29 '16 at 09:54
  • I believe, the first example won't compile, since you using `await` in non-async method. But If you change this to `async` method why just not await for this method instead of some Task which execution will end after couple of milliseconds? – Maxim Kosov Jun 29 '16 at 09:59
  • See Steztrics [answer](http://stackoverflow.com/questions/5209591/are-tasks-created-as-background-threads) – Callum Linington Jun 29 '16 at 10:01
  • There are only few differences between Task.Run and Task.Factory.StartNew. Because actually, Task.Run calls the same impl as Task.Factory.StartNew Under the hood with only a few properties difference – Callum Linington Jun 29 '16 at 10:02
  • See [here](https://msdn.microsoft.com/en-us/library/mt674882.aspx#Anchor_4) It says "The async and await keywords don't cause additional threads to be created." – Callum Linington Jun 29 '16 at 10:23
  • @MaximKosov oops, thanks. As I said, I was trying to show IProgress. I always use the third example, ie use natively asynchronous methods so I seldom have to use `Task.Run` – Panagiotis Kanavos Jun 30 '16 at 08:30
  • @MaximKosov `Task.Run` accepts a `Func` so `MeasureInBackground` can be made asynchronous. – Panagiotis Kanavos Jun 30 '16 at 08:40
3

First of all, async methods can be illusive as they won't turn your methods magically asynchronous. Instead, you can consider an async method as a setup for a state machine (see a detailed explanation here), where you schedule the chain of operations by the await calls.

For that reason, your async methods must execute as fast as possible. Do not do any blocking operation in such a setup method. If you have a blocking operation, which you want to execute in the async method, schedule it by an await Task.Run(() => MyLongOperation()); call.

So for example this will return immediately:

private async Task Task1()
{
    await Task.Run(() =>
    {
        while (!cts.IsCancellationRequested)
        {
            Thread.Sleep(500);
            ChangeParameter(0);
            Thread.Sleep(1000);
            ChangeParameter(10);
            Thread.Sleep(500);
            ChangeParameter(0);
        }
    }
}

A small remark: others may suggest to use Task.Delay instead of Thread.Sleep. I would say that use Task.Delay only if it is the part of the configuration of your state machine. But if the delay is intended to be used as a part of the long-lasting operation, which you don't want to split up, you can simply stay at the Thread.Sleep.

Finally, a remark for this part:

private async void button1_Click(object sender, EventArgs e)
{
    await Task1();
    await Task2();
}

This configures your tasks to be executed after each other. If you want to execute them parallel, do it like this:

private async void button1_Click(object sender, EventArgs e)
{
    Task t1 = Task1();
    Task t2 = Task2();
    await Task.WhenAll(new[] { t1, t2 });
}

Edit: An extra note for long-lasting tasks: By default, Task.Run executes the tasks on pool threads. Scheduling too many parallel and long lasting tasks might cause starvation and the whole application may freeze for long seconds. So for long-lasting operation you might want to use Task.Factory.StartNew with TaskCreationOptions.LongRunning option instead of Task.Run.

// await Task.Run(() => LooongOperation(), token);
await Task.Factory.StartNew(() => LooongOperation(), token, TaskCreationOptions.LongRunning, TaskScheduler.Default);
György Kőszeg
  • 17,093
  • 6
  • 37
  • 65
  • Using Thread.Sleep is a *bad* idea because it wastes a threadpool thread for a long time. Avoiding it isn't just a suggestion. Besides, `Thread.Sleep` *guarantees* execution is split up, if by that you mean that the thread will be evicted and the CPU is used by other code. Only a spinwait would prevent that – Panagiotis Kanavos Jun 29 '16 at 09:56
  • @PanagiotisKanavos: The OP wants to use long-lasting tasks with endless loops so the threads will not be freed anyway. The spinwait consumes more CPU than sleeping and does not guarantee the context change in single-core environment (unless if you use the `SpinWait` structure). – György Kőszeg Jun 29 '16 at 09:59
  • I think the only problem with `Thread.Sleep` is the actual cancel would be only after 2 seconds after button click and this could lead to bad user experience. We could add `IsCancellationRequested` calls after each small sleep, but this won't change the fact that cancel would be much later that with the `Task.Delay(token)` – Maxim Kosov Jun 29 '16 at 10:09
  • @MaximKosov: Yes, this is a better argument. More than 100ms delay is already noticeable for a user. – György Kőszeg Jun 29 '16 at 10:39
  • This isn't about arguments, this is a serious issue. In a common desktop application thread leaks (because that's what this is) may not be noticeable. In a web application, or one that performs a lot of asynchronous work (like reading data from many devices), this results in a lot of CPU waste. This means that a single server ends up serving *fewer* requests, or results in unexpected application pool restarts. An IoT device ends up burning *more* power than it should. – Panagiotis Kanavos Jun 30 '16 at 08:56