6

I have a method in my view model

private async void SyncData(SyncMessage syncMessage)
{
    if (syncMessage.State == SyncState.SyncContacts)
    {
        this.SyncContacts(); 
    }
}

private async Task SyncContacts()
{
    foreach(var contact in this.AllContacts)
    {
       // do synchronous data analysis
    }

    // ...

    // AddContacts is an async method
    CloudInstance.AddContacts(contactsToUpload);
}

When I call SyncData from the UI commands and I'm syncing a large chunk of data UI freezes. But when I call SyncContacts with this approach

private void SyncData(SyncMessage syncMessage)
{
    if (syncMessage.State == SyncState.SyncContacts)
    {
        Task.Run(() => this.SyncContacts()); 
    }
}

Everything is fine. Should not they be the same? I was thinking that not using await for calling an async method creates a new thread.

nimatra
  • 604
  • 8
  • 19
  • 2
    `async` does not always mean any other threads are involved - see [there is no thread](http://blog.stephencleary.com/2013/11/there-is-no-thread.html). – Charles Mager Aug 12 '15 at 07:04
  • Your SyncData is missing the await keyword, which will tell the compiler to continue with other processing. At the moment your SyncData method should block, which is correct –  Aug 12 '15 at 07:08

3 Answers3

10

Should not they be the same? I was thinking that not using await for calling an async method creates a new thread.

No, async does not magically allocate a new thread for it's method invocation. async-await is mainly about taking advantage of naturally asynchronous APIs, such as a network call to a database or a remote web-service.

When you use Task.Run, you explicitly use a thread-pool thread to execute your delegate. If you mark a method with the async keyword, but don't await anything internally, it will execute synchronously.

I'm not sure what your SyncContacts() method actually does (since you haven't provided it's implementation), but marking it async by itself will gain you nothing.

Edit:

Now that you've added the implementation, i see two things:

  1. I'm not sure how CPU intensive is your synchronous data analysis, but it may be enough for the UI to get unresponsive.
  2. You're not awaiting your asynchronous operation. It needs to look like this:

    private async Task SyncDataAsync(SyncMessage syncMessage)
    {
        if (syncMessage.State == SyncState.SyncContacts)
        {
            await this.SyncContactsAsync(); 
        }
    }
    
    private Task SyncContactsAsync()
    {
        foreach(var contact in this.AllContacts)
        {
           // do synchronous data analysis
        }
    
        // ...
    
        // AddContacts is an async method
        return CloudInstance.AddContactsAsync(contactsToUpload);
    }
    
Yuval Itzchakov
  • 146,575
  • 32
  • 257
  • 321
  • Are you sure it explicitly uses the thread pool? AFAIK, it explicitly uses the default `TaskScheduler` and this _happens_ to wrap the thread pool. We might know it does, but the abstractions aims to hide this detail and makes no guarantees that this may not change in the next version of .net. – Gusdor Aug 12 '15 at 07:12
  • 4
    @Gusdor [`Task.Run`](http://referencesource.microsoft.com/#mscorlib/system/threading/Tasks/Task.cs,89fc01f3bb88eed9) defaults to [`TaskScheduler.Default`](http://referencesource.microsoft.com/#mscorlib/system/threading/Tasks/TaskScheduler.cs,d4a38cb3e3085518), which is the threadpools task scheduler. – Yuval Itzchakov Aug 12 '15 at 07:13
  • @Gusdor, Yuval is correct. From MSDN: "Queues the specified work to run on the ThreadPool and returns a task or Task handle for that work." – shay__ Aug 12 '15 at 07:14
  • Thanks Yuval, I updated the question with more details about SyncContacts implementation. – nimatra Aug 12 '15 at 07:43
  • 1
    The only reason `return await` is not in the last line is because we're returning, so there's no need to await, right? – Sinjai Aug 11 '17 at 19:31
  • 1
    @Sinjai It is because I wanted to avoid the extra state machine allocation. Since smeone is already going to await higher up he call stack, and there are no other async operations in the method, we can do that. – Yuval Itzchakov Aug 12 '17 at 06:29
0

What your line Task.Run(() => this.SyncContacts()); really does is creating a new task starting it and returning it to the caller (which is not used for any further purposes in your case). That's the reason why it will do its work in the background and the UI will keep working. If you need to (a)wait for the task to complete, you could use await Task.Run(() => this.SyncContacts());. If you just want to ensure that SyncContacts has finished when you return your SyncData method, you could using the returning task and awaiting it at the end of your SyncData method. As it has been suggested in the comments: If you're not interested in whether the task has finished or not you just can return it.

However, Microsoft recommend to don't mix blocking code and async code and that async methods end with Async (https://msdn.microsoft.com/en-us/magazine/jj991977.aspx). Therefore, you should consider renaming your methods and don't mark methods with async, when you don't use the await keyword.

Daniel Fink
  • 173
  • 1
  • 8
  • That's incorrect. `await` has nothing to do with whether something runs in the background or not. It has to do with how you handle waiting for the task to complete and how you handle its response. MS doesn't recommend to use `await` in all cases either because it introduces overhead. If a method that returns a Task doesn't need to wait for the task's completion, there's no reason to use `async/await`, just return the Task as is – Panagiotis Kanavos Aug 12 '15 at 07:21
  • 1
    I didn't say that await would run tasks in the background. I said that Task.Run() will do it. With the second thing, you're right, I should better have said that MS recommend not mixing blocking code and async code https://msdn.microsoft.com/en-us/magazine/jj991977.aspx. Anyways, returning a Task instead of void should definitely be done. – Daniel Fink Aug 12 '15 at 07:27
0

Just to clarify why the UI freezes - the work done in the tight foreach loop is likely CPU-bound and will block the original caller's thread until the loop completes.

So, irrespective of whether the Task returned from SyncContacts is awaited or not, the CPU bound work prior to calling AddContactsAsync will still occur synchronously on, and block, the caller's thread.

private Task SyncContacts()
{
    foreach(var contact in this.AllContacts)
    {
       // ** CPU intensive work here.
    }

    // Will return immediately with a Task which will complete asynchronously
    return CloudInstance.AddContactsAsync(contactsToUpload);
}

(Re : No why async / return await on SyncContacts- see Yuval's point - making the method async and awaiting the result would have been wasteful in this instance)

For a WPF project, it should be OK to use Task.Run to do the CPU bound work off the calling thread (but not so for MVC or WebAPI Asp.Net projects).

Also, assuming the contactsToUpload mapping work is thread-safe, and that your app has full usage of the user's resources, you could also consider parallelizing the mapping to reduce overall execution time:

var contactsToUpload = this.AllContacts
    .AsParallel()
    .Select(contact => MapToUploadContact(contact)); 
    // or simpler, .Select(MapToUploadContact);
StuartLC
  • 104,537
  • 17
  • 209
  • 285