Can I avoid duplicate code in similar methods for synchronous and async lambdas?

Question

I've got a helper method that takes a lambda to execute within a try/catch/finally block, e.g.

public void ProcessSpreadsheet(string filename, Action<object[,]> process) {
    try {
        // Open MS Excel workbook
        // Open sheet & extract data into valueArray
        // ... more boiler plate ...

        process(valueArray);
    }
    catch (FooException e) {
        LogFoo(e.Message);
        throw;
    }
    catch (BarException e) {
        LogBar(e.Message);
        throw;
    }
    finally {
        // Close workbook, release resources, etc..
    }
}

I would now like to create an async version that accepts an async lambda.

(Note, I've read and agree with Stephen Toub's posts on async over sync, but that doesn't apply here as the crux of the method, where asynchrony is or isn't beneficial, is supplied by the consumer via a lambda.)

I can copy and paste the above, mark it as async, change the return type to Task, add "Async" to it's name, change the type of the process paramter to Func<object[,],Task> and change

process(valueArray);

to

await process(valueArray);

and everything works fine. But I'd like to avoid repeating all the code in the try/catch/finally. Is there a clean way to achieve this without repeated code?

So far the best solution I've got is:

public async Task ProcessSpreadsheetAsync(string filename, Func<object[,],Task> process) {
    var asyncTaskComplete = new ManualResetEvent(false);
    ProcessSpreadsheet(filename, async valueArray => {
        await process(valueArray);
        asyncTaskComplete.Set();
    });
    await Task.Run(() => asyncTaskComplete.WaitOne());
}

but it's messy and the exceptions aren't handled. I'm wondering if there's another way?

Answer 1

I've read and agree with Stephen Toub's posts on async over sync, but that doesn't apply here as the crux of the method, where asynchrony is or isn't beneficial, is supplied by the consumer via a lambda.

Well, yes and no. I can see where you'd say the consuming code will determine whether it's asynchronous or not. But to avoid duplication, you still have to beware the sync-over-async and async-over-sync antipatterns.

In terms of solutions, the first one that comes to mind is to only accept asynchronous lambdas. This is conceptually similar to defining a Task-returning method in an interface - the implementation may be asynchronous, or it may also be synchronous.

In your case, it would look like:

public async Task ProcessSpreadsheetAsync(string filename, Func<object[,],Task> process) {
  try {
    // Open MS Excel workbook
    // Open sheet & extract data into valueArray
    // ... more boiler plate ...

    await process(valueArray);
  }
  catch (FooException e) {
    LogFoo(e.Message);
    throw;
  }
  catch (BarException e) {
    LogBar(e.Message);
    throw;
  }
  finally {
    // Close workbook, release resources, etc..
  }
}

I would leave it at that. Any synchronous processes would be able to do:

await ProcessSpreadsheetAsync(filename, data => { ...; return Task.FromResult(true); });

In this particular case, you can also get away with writing a wrapper like this:

public void ProcessSpreadsheet(string filename, Action<object[,]> process)
{
  ProcessSpreadsheetAsync(filename, async data => { process(data); }).GetAwaiter().GetResult();
}

However, this is only safe because ProcessSpreadsheetAsync only has one await, which is on its process. If ProcessSpreadsheetAsync is ever changed to have another await, then the wrapper can easily cause a deadlock.

Answer 2

To answer your actual question instead of telling you to do something else, just extract the try/catch blocks into functions. This is advised generally as part of separation of concerns. Littering logic with error handling is bad form.

Answer 3

I would drop the idea of accepting a lambda. This is accepting responsibility in your code that really doesn't belong. Instead, simply provide a single overload that looks like this:

public object[,] ProcessSpreadsheet(string filename) {
    try {
        // Open MS Excel workbook
        // Open sheet & extract data into valueArray
        // ... more boiler plate ...

        return valueArray;
    }
    catch (FooException e) {
        LogFoo(e.Message);
        throw;
    }
    catch (BarException e) {
        LogBar(e.Message);
        throw;
    }
    finally {
        // Close workbook, release resources, etc..
    }
}

If this is being consumed by synchronous code, it will simply do:

try {
    var results = ProcessSpreadsheet("...");
    DoSomethingElse(results);
} catch (FooException e) {
    // ...
} catch (CanHappenWhileDoingSomethingElseException e) {
    // ...
}

The consumer of your code has infinite flexibility as far as dealing with exceptions is concerned.

If someone is using async/await, all they have to do is:

try {
    var results = await Task.Run(() => ProcessSpreadsheet("..."));
    DoSomethingElse(results);
} catch (FooException e) {
    // ...
} catch (CanHappenWhileDoingSomethingElseException e) {
    // ...
}

All of their error handling works exactly the same way.