CODEWITHSUNDEEP
c#recursionasync-await
Krumelur
Krumelur

Reputation: 33048

How to get this recursive async/await issue right?

I have a recursive method that visits every node in a tree hierarchy and triggers a callback for every node, like (code below is not tested and an example):

void Visit(Node node, Func<Node, int> callback, CancellationToken ct)
{
  if(ct.IsCancellationRequested)
  {
    return;
  }

  var processedNode = DoSomeProcessing(node);

  int result = callback(processedNode);

  // Do something important with the returned int.
  ...


  // Recursion.
  foreach(var childNode in node.Children)
  {
    Visit(childNode, callback);
  }
}

The method above is called from an async method that passes the callback on. As it is long running, I wrap the call into a Task.Run():

async Task ProcessAsync(Func<Node, int> callback)
{
  var rootNode = await someWebService.GetNodes();
  await Task.Run( () => Visit(rootNode, callback) );
}

Now here's the issue: some async code is awaiting ProcessAsync():

...
await ProcessAsync( node => {
  return DoSomethingWithTheNode();
});
...

This works. However, no we're refactoring and DoSomethingWithTheNode becomes async: DoSomethingWithTheNodeAsync. The result won't build because the delegate types don't match. I would have to return Task<int> instead of int:

...

    await ProcessAsync( async node => {
      return await DoSomethingWithTheNodeAsync();
    });
    ...

If I change the delegate's signature to Func<Node, Task<int>> I will have to make my Visit method async which is somehow weird - it is already running as a Task and I don't feel very good about using a recursive async method that gets an async callback passed in. Can't explain, but it looks wrong.

Question:

Upvotes: 3

Views: 3414

Answers (2)

noseratio
noseratio

Reputation: 61666

After re-factoring, your new async Visit might look like this:

async Task Visit(Node node, Func<Node, Task<int>> callback, CancellationToken ct)
{
  if(ct.IsCancellationRequested)
  {
    return;
  }

  var processedNode = DoSomeProcessing(node);

  int result = await callback(processedNode).ConfigureAwait(false);

  // Do something important with the returned int.
  ...


  // Recursion.
  foreach(var childNode in node.Children)
  {
    await Visit(childNode, callback, token);
  }
}

Then ProcessAsync would look like this:

async Task ProcessAsync(Func<Node, Task<int>> callback, token)
{
  var rootNode = await someWebService.GetNodes();
  await Visit(rootNode, callback, token);
}

And it can simply be called like this:

await ProcessAsync(DoSomethingWithTheNodeAsync, token);

Because you are introducing asynchrony into your callback, most likely you no longer need to offload ProcessAsync to a separate thread. Below I'll try to explain why.

Let's consider for a minute your DoSomethingWithTheNodeAsync looks like this:

async Task<int> DoSomethingWithTheNodeAsync(Node node)
{
    Debug.Print(node.ToString());
    await Task.Delay(10); // simulate an IO-bound operation
    return 42;
}

Inside Visit, the execution after await callback(processedNode).ConfigureAwait(false) will continue on a random pool thread (the thread which happened to serve the completion of the asynchronous Task.Delay operation). So, the UI thread will no longer be blocked.

The same is true for any other pure asynchronous API which you may be using inside DoSomethingWithTheNodeAsync (and which was the original reason for re-factoring, I believe).

Now, my only concern would be this: 

var processedNode = DoSomeProcessing(node);

Once you've called ProcessAsync(DoSomethingWithTheNodeAsync), the very first invocation of the above DoSomeProcessing will happen on the same thread as the original call. If that's a UI thread, DoSomeProcessing might block the UI for one time, for as as long as the processing goes inside it.

If that's a problem, then wherever you invoke ProcessAsync from the UI thread, wrap it with Task.Run, e.g:

void async button_click(object s, EventArgs e)
{
    await Task.Run(() => ProcessAsync(DoSomethingWithTheNodeAsync));
}

Note, we still don't use Task.Run anywhere inside ProcessAsync, so there will be no redundant thread switching during the recursive traversal of the tree.

Also note, you do not need to add another async/await to the lambda like below:

await Task.Run(async () => await ProcessAsync(DoSomethingWithTheNodeAsync));

That would add some redundant compiler-generated state machine code. Task.Run has an override to deal with a lambda returning Task<T> or Task, which unwraps the nested task with Task.Unwrap. More about this here.

Finally, if something inside DoSomeProcessing or DoSomethingWithTheNodeAsync updates the UI, it has to been done on the UI thread. With Monotouch, it can be done via SynchronizationContext.Post/Send of the UI thread's SynchronizationContext.

Upvotes: 2

usr
usr

Reputation: 171178

If you want to use async to unblock the calling thread, the entire call tree becomes infected up to the point where you need to release the thread. This is the nature of async, and it is a major reason why you should carefully think whether you want to bring it into a codebase or not.

In that sense, Visit should be async because something it calls is async. Don't fall into the trap of implementing the well-known async-over-sync anti-pattern (or sync-over-async).

Especially, this is likely to be wrong:

await Task.Run(() => Visit(rootNode, callback) );

This is async-over-sync. Make Visit async and treat Task.Run suspiciously.

Regarding your sub-question about IEnumerable<Node>: I'm unclear on what exactly you want to accomplish. Depending on that, you could use a Task<IEnumerable<Node>> or an IAsyncEnumerable<Node>. Depends on the use case.

... I will have to make my Visit method async which is somehow weird - it is already running as a Task and I don't feel very good about using a recursive async method that gets an async callback passed in.

You should feel bad about using Task.Run. That's the mistake, not making Visit async.

Upvotes: 0

Related Questions