What is the correct way to mark async method as not implemented/not supported or invalid operation. For the simplicity I would use only NotImplementedException in examples, but the question applies to the NotSupportedException and InvalidOperationException as well.
In a sync way one would simple throw the exception:
public override void X() {
throw new NotImplementedException();
}
What would be the equivalent of this code in async world?
/* 1 */ public override Task XAsync() {
throw new NotImplementedException();
}
Or
/* 2 */ public override Task XAsync() {
return Task.FromException(new NotImplementedException());
}
What are the complications of these approaches? Is there any better method?
To avoid the "Nah, you don't need a method to be async here"/"It's not async" I would say that the method implements some interface or abstract class.
Some methods which I'm not considering:
/* 3 */ public async override Task XAsync() { // here is an CS1998 warning
throw new NotImplementedException();
}
The compiler would just generate the useless state machine, which is semantically equivalent to 2
/* 4 */ public async override Task XAsync() {
await Task.Yield();
throw new NotImplementedException();
}
This is the same as 3, but with added await on Task.Yeild();
When calling a method that returns a Task, some parts of it are executed synchronously (even if the implementing method is defined as async and has await calls in it.. until the first away, everything is synchronous by default).
So the outcome is the same for all options: throw immediately or return a Task that is already completed with an exception (only behaves the same if you await the call immediately) or mark a method async (which would expect you to have await calls but let's add it for completeness).
I'd go for throwing immediately because returning a Task may indicate that you "have started work" and the caller isn't required to await a Task so if the caller doesn't really care about when your Task completes (it doesn't even have a return value), the fact that the method isn't implemented won't show up.
async keyword. - Martin Ullrich
I'm going to go out on a limb and say "it doesn't matter."
Boneheaded exceptions can be thrown directly (throw) or placed on the returned task (Task.FromException). Since they're boneheaded exceptions, they shouldn't ever be caught anyway, so it doesn't matter where they are thrown.
In your comment, you wrote:
we are trying to make an async version of NHibernate:)
That puts you in an unfortunate position: well-known libraries written by skilled programmers should be well-written to protect against accidental misuse (including misuse through copy/paste) by less-skilled programmers.
There are enough people who expect code such as await Task.WhenAll(a(), b(), c()) to work even if one of the asynchronous operations fails, that I'd say your first option shouldn't even be an option. If b() throws an exception synchronously, then a()'s returned task gets ignored, and c() doesn't get called.
I agree with Stephen Cleary's answer saying that NotImplementedException is, as he puts it, a boneheaded exception where it doesn't matter, as it should never end up in production code anyway. However, you write:
the question applies to the
NotSupportedExceptionandInvalidOperationExceptionas well.
These are not necessarily boneheaded exceptions. These could end up in production code.
Your second option avoids that problem.
Your second option does have an additional problem: it doesn't throw an exception. Since no exception is actually thrown, you're hindering debugging: when something's going wrong, it's very useful to have the option in your debugger of breaking at the point where the exception is thrown rather than where it's caught.
I suggested also considering
public async override Task XAsync() {
throw new NotImplementedException();
}
which you discarded because of the overhead associated with creating a state machine. I think this is not a valid reason to discard it. This is code where performance is not relevant, this is code that only handles error cases. I suggested it because in general, I'm in favour of using async/await over manipulating tasks directly, because it's so much easier to catch silly mistakes, because the time saved in development time is high enough to be worth it.
I understand why you don't want to go with this option, but I personally still would. It avoids the drawback of your first option. It avoids the drawback of your second option. Its own drawback, the slightly slower performance, is in my experience less likely to become a problem than the other two.
Out of the other two, hopefully the details of the drawbacks help you make a well-informed decision.
async Task XAsync() { throw ...; }is another option. With theasynckeyword. - user743382