CODEWITHSUNDEEP
c#
AndrewH
AndrewH

Reputation: 43

C# Progress Reporting

The following code fails to compile: 

async Task Foo (Action<int> onProgressPercentChanged)
{
    return Task.Run (() =>
    {
        for (int i = 0; i < 1000; i++)
        {
            if (i % 10 == 0) onProgressPercentChanged (i / 10);

        }
    });
}

due to the following error:

Severity Code Description Project File Line Suppression State Error CS1997 Since 'Nutshell.Foo(Action)' is an async method that returns 'Task', a return keyword must not be followed by an object expression. Did you intend to return 'Task'?

but this is done exactly as shown in the book. Is there something I'm missing?

Upvotes: 1

Views: 5323

Answers (3)

Stephen Cleary
Stephen Cleary

Reputation: 456407

Your method is marked async Task, so the compiler expects it to not return anything. If you want to return a task, then you should not mark the method async. Alternatively, you could use await inside the async method.

// Option 1
Task Foo (Action<int> onProgressPercentChanged)
{
  return Task.Run (() =>
  {
    for (int i = 0; i < 1000; i++)
    {
      if (i % 10 == 0) onProgressPercentChanged (i / 10);
    }
  });
}

// Option 2
async Task Foo (Action<int> onProgressPercentChanged)
{
  await Task.Run (() =>
  {
    for (int i = 0; i < 1000; i++)
    {
      if (i % 10 == 0) onProgressPercentChanged (i / 10);
    }
  });
}

For more about the difference between these two, see my blog post on eliding async.

On to the other problems...

Use the progress reporting built-in to .NET instead of rolling your own.

.NET uses the IProgress<T> type, so you can just use that:

Task Foo (IProgress<int> progress = null)
{
  return Task.Run (() =>
  {
    for (int i = 0; i < 1000; i++)
    {
      if (progress != null && i % 10 == 0) progress.Report(i / 10);
    }
  });
}

By following the progress conventions, we allow callers to not request progress updates, and we also allow a very natural marshalling to the UI thread using Progress<T>:

// Caller, from UI thread
var progress = new Progress<int>(report =>
{
  // Handle progress report. This code is already on the UI thread! :)
});
await Foo(progress);

Don't use Task.Run in the implementation; use Task.Run to call the method. Using Task.Run to implement fake-asynchronous methods is an antipattern. I write more about this on my blog as well.

Applying this fix makes Foo synchronous, which is exactly correct since it does no actual asynchronous work:

void Foo(IProgress<int> progress = null)
{
  for (int i = 0; i < 1000; i++)
  {
    if (progress != null && i % 10 == 0) progress.Report(i / 10);
  }
}

// Caller
var progress = new Progress<int>(report => ...);
await Task.Run(() => Foo(progress));

And finally, don't throttle progress reports at the source; throttle them at the receiver. If your code ever runs on different CPUs, then any throttling you do at the source (i % 10) is going to be problematic. It's better to use an IProgress<T> implementation that throttles.

This nicely simplifies your code to:

void Foo(IProgress<int> progress = null)
{
  for (int i = 0; i < 1000; i++)
  {
    if (progress != null) progress.Report(i / 10);
  }
}

Upvotes: 8

tdragon
tdragon

Reputation: 3329

Instead of return you should await your Task.Run statement

async Task Foo(Action<int> action)
{
    await Task.Run(() => /*...*/);
}

or, if you prefer to return Task, you should not mark the method as async

Task Foo(Action<int> action)
{
    return Task.Run(() => /*...*/);
}

Upvotes: 2

Vasyl Zvarydchuk
Vasyl Zvarydchuk

Reputation: 3839

Remove return statement in method. Your method shouldn't return anything.

Upvotes: 1

Related Questions