[Analyzer/fixer suggestion]: Exceptions thrown inside async methods should be wrapped by Task.FromException

[carlossanlop]

Sparked from this question: #70574 (comment)

Exceptions thrown inside methods that return Task or ValueTask should be wrapped by Task.FromException, except if they are ArgumentNullException or ArgumentException.

If the method is marked as async, we could also await the Task.FromException.

Suggested category: Reliability
Suggested level: Warning (Same as CA2007)
Suggested milestone: 8.0

Examples

Method that returns Task

Before

// Task
private Task MyMethodAsync(string arg, CancellationToken cancellationToken)
{
    ArgumentException.ThrowIfNullOrEmpty(arg);
    if (/* some condition */)
    {
        throw new InvalidOperationException("Some message");
    }
    return SomethingAsync(cancellationToken);
}

// async Task
private async Task MyMethodAsync(string arg, CancellationToken cancellationToken)
{
    ArgumentException.ThrowIfNullOrEmpty(arg);
    if (/* some condition */)
    {
        throw new InvalidOperationException("Some message");
    }
    await SomethingAsync(cancellationToken).ConfigureAwait(false);
}

After

// Task
private Task MyMethodAsync(string arg, CancellationToken cancellationToken)
{
    ArgumentException.ThrowIfNullOrEmpty(arg); // Do not change this
    if (/* some condition */)
    {
        return Task.FromException(new InvalidOperationException("Some message")); // wrap this
    }
    return SomethingAsync(cancellationToken);
}

// async Task
private Task MyMethodAsync(string arg, CancellationToken cancellationToken)
{
    ArgumentException.ThrowIfNullOrEmpty(arg); // Do not change this
    if (/* some condition */)
    {
        // wrap this, await it, but don't add ConfigureAwait(false), that should be added by CA2007 separately
        await Task.FromException(new InvalidOperationException("Some message"));
    }
    await SomethingAsync(cancellationToken).ConfigureAwait(false);
}

Method that returns ValueTask

Before

// ValueTask
private ValueTask MyMethodAsync(string arg, CancellationToken cancellationToken)
{
    ArgumentException.ThrowIfNullOrEmpty(arg);
    if (/* some condition */)
    {
        throw new InvalidOperationException("Some message");
    }
    return SomethingAsync(cancellationToken);
}

// async ValueTask
private async ValueTask MyMethodAsync(string arg, CancellationToken cancellationToken)
{
    ArgumentException.ThrowIfNullOrEmpty(arg);
    if (/* some condition */)
    {
        throw new InvalidOperationException("Some message");
    }
    await SomethingAsync(cancellationToken).ConfigureAwait(false);
}

After

private ValueTask MyMethodAsync(string arg, CancellationToken cancellationToken)
{
    ArgumentException.ThrowIfNullOrEmpty(arg); // Do not change this
    if (/* some condition */)
    {
        return ValueTask.FromException(new InvalidOperationException("Some message")); // wrap this
    }
    return SomethingAsync(cancellationToken);
}

// async ValueTask
private async ValueTask MyMethodAsync(string arg, CancellationToken cancellationToken)
{
    ArgumentException.ThrowIfNullOrEmpty(arg);
    if (/* some condition */)
    {
        // wrap this, await it, but don't add ConfigureAwait(false), that should be added by CA2007 separately
        await ValueTask.FromException(new InvalidOperationException("Some message"));
    }
    await SomethingAsync(cancellationToken).ConfigureAwait(false);
}

cc @buyaa-n

[ghost]

Tagging subscribers to this area: @dotnet/area-system-threading-tasks
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Sparked from this question: #70574 (comment)

Exceptions thrown inside methods that return Task or ValueTask should be wrapped by Task.FromException, except if they are ArgumentNullException or ArgumentException.

If the method is marked as async, we could also await the Task.FromException.

Suggested category: Reliability
Suggested level: Warning (Same as CA2007)
Suggested milestone: 8.0

Examples

Method that returns Task

Before

// Task
private Task MyMethodAsync(string arg, CancellationToken cancellationToken)
{
    ArgumentException.ThrowIfNullOrEmpty(arg);
    if (/* some condition */)
    {
        throw new InvalidOperationException("Some message");
    }
    return SomethingAsync(cancellationToken);
}

// async Task
private async Task MyMethodAsync(string arg, CancellationToken cancellationToken)
{
    ArgumentException.ThrowIfNullOrEmpty(arg);
    if (/* some condition */)
    {
        throw new InvalidOperationException("Some message");
    }
    await SomethingAsync(cancellationToken).ConfigureAwait(false);
}

After

// Task
private Task MyMethodAsync(string arg, CancellationToken cancellationToken)
{
    ArgumentException.ThrowIfNullOrEmpty(arg); // Do not change this
    if (/* some condition */)
    {
        return Task.FromException(new InvalidOperationException("Some message")); // wrap this
    }
    return SomethingAsync(cancellationToken);
}

// async Task
private Task MyMethodAsync(string arg, CancellationToken cancellationToken)
{
    ArgumentException.ThrowIfNullOrEmpty(arg); // Do not change this
    if (/* some condition */)
    {
        // wrap this, await it, but don't add ConfigureAwait(false), that should be added by CA2007 separately
        await Task.FromException(new InvalidOperationException("Some message"));
    }
    await SomethingAsync(cancellationToken).ConfigureAwait(false);
}

Method that returns ValueTask

Before

// ValueTask
private ValueTask MyMethodAsync(string arg, CancellationToken cancellationToken)
{
    ArgumentException.ThrowIfNullOrEmpty(arg);
    if (/* some condition */)
    {
        throw new InvalidOperationException("Some message");
    }
    return SomethingAsync(cancellationToken);
}

// async ValueTask
private async ValueTask MyMethodAsync(string arg, CancellationToken cancellationToken)
{
    ArgumentException.ThrowIfNullOrEmpty(arg);
    if (/* some condition */)
    {
        throw new InvalidOperationException("Some message");
    }
    await SomethingAsync(cancellationToken).ConfigureAwait(false);
}

After

private ValueTask MyMethodAsync(string arg, CancellationToken cancellationToken)
{
    ArgumentException.ThrowIfNullOrEmpty(arg); // Do not change this
    if (/* some condition */)
    {
        return ValueTask.FromException(new InvalidOperationException("Some message")); // wrap this
    }
    return SomethingAsync(cancellationToken);
}

// async ValueTask
private async ValueTask MyMethodAsync(string arg, CancellationToken cancellationToken)
{
    ArgumentException.ThrowIfNullOrEmpty(arg);
    if (/* some condition */)
    {
        // wrap this, await it, but don't add ConfigureAwait(false), that should be added by CA2007 separately
        await ValueTask.FromException(new InvalidOperationException("Some message"));
    }
    await SomethingAsync(cancellationToken).ConfigureAwait(false);
}

cc @buyaa-n

Author:	carlossanlop
Assignees:	-
Labels:	api-suggestion, area-System.Threading.Tasks, code-analyzer, code-fixer
Milestone:	8.0.0

[bartonjs]

If the method is built with the async keyword, then any throws are already wrapped by the task(-like) that was returned (which is wrong for argument validation, and why argument validation should be in a non-async-keyword wrapper call).

await Task.FromException(...) is unnecessary (and probably an anti-pattern)

[carlreinke]

argument validation should be in a non-async-keyword wrapper call

Seems like there could be an analyzer for this. Or does that already exist?

[stephentoub]

I've marked this as ready for review, but I suspect we're going to find it's a bit noisy. If this is approved, whoever tackles it will likely need to do a bit of work to tune the rule to get it to the point where it can be on by default in runtime.

[stephentoub]

(It's also very likely to have a lot of false negatives, due to exceptions getting thrown indirectly via method calls.)

[terrajobst]

Video

• Seems reasonable. We should have a list of exceptions for which we don't warn (exceptions derived from ArgumentException for instance).
• Should not apply to methods marked async

Category: Reliability
Severity: Warning

[rjgotten]

If the method is built with the async keyword, then any throws are already wrapped by the task(-like) that was returned (which is wrong for argument validation, and why argument validation should be in a non-async-keyword wrapper call).

One could just as easily argue that it's bad and may catch users off guard, and require them to handle both asynchronous and synchronous exceptions, instead of just asynchronous.

Which in fact are two bullet points that come up in other async guidance communicated for ASP.NET Core applications by e.g. David Fowl, in the context of preferring async-await over directly returning Task instances: https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#prefer-asyncawait-over-directly-returning-task

Setting up wrapping methods that synchronously throw on argument exceptions also fall afoul of exactly this guidance, because the way it's implemented is a task-returning method which is not async-await, but delegates to one that is.

I.e. there's no cut and dry single point of view that applies here.