9

Similar to Implementing an interface that requires a Task return type in synchronous code although I'm curious if I should just ignore the compiler error my situation generates instead.

Let's say I have an interface like this:

public interface IAmAwesome {
    Task MakeAwesomeAsync();
}

In some implementations making awesome benefits from being done asynchronously using async and await. This is really what the interface is attempting to allow for.

In other cases, perhaps rare, only a simple synchronous method is needed to make awesome. So let's suppose that implementation looks like this:

public class SimplyAwesome : IAmAwesome {
    public async Task MakeAwesomeAsync() {
        // Some really awesome stuff here...
    }
}

This works, but the compiler is warning:

This method lacks 'await' operators and will run synchronously. Consider using the await operator to await non-blocking API calls, or 'await TaskEx.Run(...)' to do CPU-bound work on a background thread.

The compiler is actually suggesting this solution:

public class SimplyAwesome : IAmAwesome {
    public async Task MakeAwesomeAsync() {
        await Task.Run(() => {
            // Some really awesome stuff here, but on a BACKGROUND THREAD! WOW!
        });
    }
}

My question is - what should determine when I choose to ignore this compiler warning? In some cases the work is so simple that spawning a thread for it is undeniably counter-productive.

Community
  • 1
  • 1
Yuck
  • 49,664
  • 13
  • 105
  • 135
  • 2
    What you "Should" do is have `public interface IAmAwesome { Task MakeAwesomeAsync(); void MakeAwesome(); }` so both a asynchronous and synchronous method is exposed. But if that is practical for your real world situation is a different story. – Scott Chamberlain Jan 28 '15 at 16:59
  • I guess the question would be why are you making tasks that don't involve any asynchrony. Can you provide a concrete use case? – JLRishe Jan 28 '15 at 16:59
  • 2
    @JLRishe A common example would be doing something that can sometimes be cached. The "get from cache" version can be synchronous, the cache miss version would be asynchronous. – Servy Jan 28 '15 at 17:00
  • Without getting overly involved, in my case the example is getting pricing. Sometimes it's simple math (i.e. CPU-bound) in other cases it involves expensive API and database calls. – Yuck Jan 28 '15 at 17:02
  • I would measure the cost of the calculation. If it's rather fast, wrapping the result in a `Task` should be fine. – Yuval Itzchakov Jan 28 '15 at 17:06

3 Answers3

13

If you really do want to do the work synchronously, you know that your async method will always run synchronously, and that's desirable in the situation, then by all means, ignore the warning. If you understand what the warning is telling you and feel that the action it is describing is correct, then it's not a problem. There's a reason it's a warning and not an error after all.

Of course, the other option is to just not make the method async and to simply use Task.FromResult to return an already completed task instead. It would change the error handling semantics (unless you also catch all exceptions and wrap them into a task that you return) so at least be mindful of that. If you really want exceptions to be propagated through the resulting Task, it may be worth leaving the method async and just suppressing the warning.

Servy
  • 202,030
  • 26
  • 332
  • 449
  • If I ignore the warning, and a client unknowingly uses `await` to invoke the method, what will happen to any exceptions? – Yuck Jan 28 '15 at 17:13
  • @Yuck The `Task` that is returned will be `Faulted` and will contain the information about the exception. When that `Task` is awaited, that exception will be re-thrown. It would look no different than if the operation were actually asynchronous. – Servy Jan 28 '15 at 17:14
  • Why not keep the `async` but get rid of the compiler warning by doing `await Task.FromResult` or nowadays `await Task.CompletedTask`? Wouldn't this accomplish getting rid of the compiler warning *and* keeping the exception handling semantics that consumers expect? – rory.ap Aug 23 '18 at 16:12
  • @rory.ap I don't see how intentionally wrapping the result in a completed task, only to then unwrap it again, expresses the intentions of the code any better than just not doing that. The second option I mention is the only option of any of them that really expresses the clear intention to run the method synchronously and return an already completed task. – Servy Aug 23 '18 at 17:06
7

What should determine when I choose to ignore this compiler warning? In some cases the work is so simple that spawning a thread for it is undeniably counter-productive.

The compiler isn't saying "use Task.Run inside this method". It is merely telling you that you prepared him for an async method, adding the async modifier to your method declaration, but you aren't actually awaiting anything.

You could take three approaches:

A. You could ignore the compiler warning and everything will still execute. Note, that there will be a slight overhead of state-machine generation, but your method call will execute synchronously. If the operation is time consuming and might cause the method execution to make a blocking call, this might confuse the users consuming this method.

B. Separate the generation of the "Awesome" into a synchronous interface and an asynchronous interface:

public interface MakeAwesomeAsync
{
    Task MakeAwesomeAsync();
}

public interface MakeAwesome
{
    void MakeAwesome();
}

C. If the operation is not too time consuming, you can simply wrap it in a Task using Task.FromResult. I would definitely measure how long it takes to run the CPU bound operation before choosing this.

Servy
  • 202,030
  • 26
  • 332
  • 449
Yuval Itzchakov
  • 146,575
  • 32
  • 257
  • 321
  • If splitting the interface is not possible, would you agree with Servy's answer? – Yuck Jan 28 '15 at 17:01
  • If the operation isn't a heavily CPU bound operation, wrapping it in `Task.Result` should be fine. – Yuval Itzchakov Jan 28 '15 at 17:02
  • `This will probably confuse the consumers of your code.` That's only true if the operation is long running. If the operation can be completed extremely quickly then it's not a problem. For example, the `Stream` implementation for file IO executes most of its "Async" methods synchronously, because it just takes so little time that it's not worth the hassle of doing them asynchronously, but the network IO implementation need to do things asynchronously because they just take long enough. – Servy Jan 28 '15 at 17:08
  • @Servy I agree, i should make that explicit. It always depends on if it's noticeable or not. If it isn't, `Task.FromResult` should definitely suffice. – Yuval Itzchakov Jan 28 '15 at 17:09
  • @YuvalItzchakov Keep in mind that providing the same error handling semantics as an `async` method becomes a fair bit of boilerplate. Having an `async` method with no `await` can actually be simpler and easier, so long as one understands what's going on. – Servy Jan 28 '15 at 17:11
  • @Servy I'm not quite sure what you mean about the boilerplate. Could you elaborate? – Yuval Itzchakov Jan 28 '15 at 17:13
  • 1
    @YuvalItzchakov You'd need to catch all exceptions, and then return a faulted task using that exception. If you didn't, exceptions would propagate to the caller of the method, rather than being returned in the resulting `Task`. – Servy Jan 28 '15 at 17:14
  • @Servy I think that actually answers the comment I had on your answer. – Yuck Jan 28 '15 at 17:14
  • @Servy I'd be surprised if i called an `async` method wouldn't return a faulted task TBH. That would be weird on all parts. Perhaps its a matter of taste, but i'd rather have the boilerplate then an unexpected behavior. – Yuval Itzchakov Jan 28 '15 at 17:15
  • @YuvalItzchakov My point is that you can use an `async` method, instead of calling `FromResult` in a non-async method, and get the desirable (in my opinion) error handling semantics *without the boilerplate*. If you make the method not be `async` you have to add that boilerplate to every method to have consistent error handling semantics. It wouldn't be *terrible*, but then again neither is ignoring a warning when you know you're doing the right thing. – Servy Jan 28 '15 at 17:16
  • @Servy I see what you mean. Well, i guess you could. It would be weird on part of other programmers that wouldn't understand why that is. And perhaps a small bit of overhead of the state-machine. – Yuval Itzchakov Jan 28 '15 at 17:17
  • 1
    @YuvalItzchakov The state machine overhead here is actually giving you something; you're using it. But yes, you'd need to be working with other programmers who have an understanding of why you'd write a method like this. The consumers wouldn't need to care, but other developers working on it would. – Servy Jan 28 '15 at 17:19
  • @YuvalItzchakov -- In option A, you said "If the operation is time consuming and might cause the method execution to make a blocking call, this might confuse the users consuming this method." Is this not also an issue with option C? Whether or not a method is `async` is not known to consumers, so it option A and C would essentially be the same to consumers (assuming you did as Servy spoke of when no using `async` which is to roll your own error handling semantics that mimics that which `async` gives you automatically). – rory.ap Aug 23 '18 at 16:17
  • @rory.ap From the POV of the consumer, yes, they will be identical. From the implementation POV, they'd be different. – Yuval Itzchakov Aug 23 '18 at 16:38
  • @YuvalItzchakov Thank you. Can you also elaborate on option B? If your interface still has an async version, your implementation will still have to, well, implement that version. It will still be just as "confusing" to the consumer as before in that it doesn't actually run asynchronously -- which may be OK if it runs fast, but what if it's CPU bound and runs slowly? What then? – rory.ap Aug 24 '18 at 11:26
7

As everybody else already pointed out, you have 3 different options, so it's a matter of opinion:

  1. keep it async and ignore the warning
  2. have sync/async overloads
  3. remove async and return a completed task.

I would recommend returning an already completed task:

public class SimplyAwesome : IAmAwesome 
{
    public Task MakeAwesomeAsync() 
    {
        // run synchronously
        return TaskExtensions.CompletedTask;
    }
}

For several reasons:

  • Making a method async has a slight overhead of creating a state-machine and disabling some optimizations (like inlining) because of the try-catch block the compiler adds.
  • You need to deal with the warning and with any other team-member looking at this code wondering where the await is.
  • There's somewhat of a dissonance in marking a completely synchronous method async. It's like adding while(false){} in your code, it will act the same, but it doesn't convey the meaning of the method.

Servy pointed out that returning a task changes the semantics of exception handling. While that's true, I think it's a non-issue.

First of all, most async code calls a method and awaits the returned task at the same place (i.e. await MakeAwesomeAsync()) which means the exception would be thrown at the same place no matter whether the method was async or not.

Second of all, even the .Net framework's Task-returning methods throw exceptions synchronously. Take for example Task.Delay which throws an exception directly without storing it in the returned task, so there's no need to await the task to raise the exception:

try
{
    Task.Delay(-2);
}
catch (Exception e)
{
    Console.WriteLine(e);
}

Since .Net developers need to except to encounter exceptions synchronously in .Net it's reasonable they should also except that from your code.

Moerwald
  • 10,448
  • 9
  • 43
  • 83
i3arnon
  • 113,022
  • 33
  • 324
  • 344
  • 1
    I'm not sure why this answer doesn't have more upvotes. IMO, it far and away makes the most sense to not use the async keyword for a method which does not run asynchronously. A return type of Task in an API simply means that the method *could* run asynchronously. It's not a guarantee that it will. The implementor of the method gets to make that implementation detail decision without affecting the public API. – jam40jeff Mar 26 '15 at 02:35
  • 1
    Also, two thumbs up for beating .NET 4.6 to the punch with TaskExtensions.CompletedTask, which is more performant (and conveys the meaning better) than what I have always done (Task.FromResult(0)). – jam40jeff Mar 26 '15 at 02:38