Say I have an interface IBackgroundTask:

public interface IBackgroundTask
{
    Task<TaskResult> Execute();
}

The application that uses this reads messages from a queue, and then starts consumer logic to execute a background task for the message.

The interfaces dictates that it should be implemented as a Task, however sometimes the consumer does not actually need to execute any asynchronous work. In such cases the signature we have to implement would better be TaskResult Execute();. But we can't change this interface implementation, and it serves as the base to implement both synchronous and asynchronous work.

For the async work, this works well, we can properly use the async keyword and await the async calls (examples: calls to API, disk IO):

public async Task<TaskResult> Execute()
{
    await PossiblyLongExternalAPICall();
    return new TaskResult();
}

But for a consumer that only does synchronous work (examples: a query to the database), I'd have to implement it like so:

public Task<TaskResult> Execute()
{
    ExecuteSyncCode();
    return Task.FromResult(new TaskResult()); // is this the right thing to do?
}

Is this considered the right approach for situations like this? I have already read up on this topic and it seems like Task.FromResult for synchronous code isn't really a bad practice, but I'm wondering if there are better alternatives for this scenario.

CodePudding user response：

Is this considered the right approach for situations like this?

I would say No, for rather subtle reasons.

Task.FromResult allows you to return a task from a non-async method, sure. And it works fine for the successful case. But it doesn't work as expected for the exceptional case.

Methods with an asynchronous signature are expected to place all the method results on the returned task. They can complete synchronously - there's nothing wrong with that - but any exception is considered a "result" in this sense and should be placed on the returned task.

So, you'd want to catch any exceptions from ExecuteSyncCode (or even new TaskResult()) and return Task.FromException if there was one. And then to be properly pedantic, you may also want to catch OperationCanceledException explicitly and return Task.FromCanceled. By the time you're done handling all the corner cases, your code ends up looking something like this.

And by that time, your "simple" synchronous implementation is full of a bunch of boilerplate that muddles up the code and confuses future maintainers.

For this reason, I tend to prefer just marking the method as async and not using await. This will cause a compiler warning (CS1998 This async method lacks 'await' operators and will run synchronously.), which is generally useful but which you can #pragma ignore for this specific case since you want a synchronous implementation. Using async essentially makes the compiler do all that boilerplate for you, while keeping the implementation synchronous.

If you end up with several synchronous implementations of asynchronous interfaces, I'd recommend defining a couple static helper methods like this. That way, the ugly #pragmas are confined to a single helper class.

CodePudding user response：

Yes, you are doing the right thing. Using the Task.FromResult method is the most economical way to create a completed Task<TResult>.

In case you anticipate that implementations of the IBackgroundTask.Execute method will return completed tasks most of the time, you could consider changing the interface so that the return type is ValueTask<TaskResult> instead of Task<TaskResult>. Creating a completed ValueTask<TResult> imposes no additional allocations, beyond what memory needs to be allocated for the TResult itself. On the contrary a completed Task<TResult> requires ~70 additional bytes per instance. This optimization makes sense if you also anticipate that the Execute method will be invoked in hot paths. Otherwise, if it's invoked once in a while, any performance gains will be absolutely tiny and negligible. The ValueTask<TResult> type is generally more cumbersome to use than the Task<TResult>, so proceed with caution.

Note: this answer assumes that your decision to provide a synchronous implementation to a method with asynchronous contract, is based on good reasons. I am not an advocate of breaking the asynchronous contract in general. If you don't have good reasons to break it, then please don't.