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.

22
  • 6
    Replace Thread.Sleep(500) with await Task.Delay(500) Commented Jun 29, 2016 at 8: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. Commented Jun 29, 2016 at 8:35
  • 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 Commented Jun 29, 2016 at 8:36
  • 2
    @CallumLinington no you don't need Invoke, if you use async/await and IProgress<T> Commented Jun 29, 2016 at 8:38
  • 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 Commented Jun 29, 2016 at 8:41

3 Answers 3

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);
Sign up to request clarification or add additional context in comments.

5 Comments

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
@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).
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)
@MaximKosov: Yes, this is a better argument. More than 100ms delay is already noticeable for a user.
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.
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.

Comments

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);

12 Comments

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
@MaximKosov not in this example. As I said, the purpose was to show how IProgress<T> 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(...);
You actually use await in the first example in await Task.Delay() :) Don't this should be Thread.Sleep?
Task.Run isn't needed, unless you want it to run on a background thread
@MaximKosov no, Thread.Sleep would waste a pool thread. await Task.Delay() releases the thread for other tasks to use
|

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.