How to get this recursive async/await issue right?

Question

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:

• Is the approach just not suitable for an async world?
• How to get this done right?
• I originally did not want to use a callback method but an IEnumerable<Node>. However this seems to be impossible with async methods.
• Is this question maybe more adequate for codereview.stackexchange.com?

Answer 1

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.