RSPEC-4457 (C#): Wrong reasoning about param check in async methods

Clean Code Report False-positive / False-negative...
, ,
1

Parameter validation in “async”/“await” methods should be wrapped

Because of the way async/await methods are rewritten by the compiler, any exceptions thrown during the parameters check will happen only when the task is observed. That could happen far away from the source of the buggy code or never happen for fire-and-forget tasks.

Rule description seems to be wrong in reasoning why this is an issue. Any code before first await in async method is still being executed synchronously, exception is thrown immediately as expected.

public async Task<int> Divide(int a, int b)
{
    if (b == 0)
    {
        throw new ArgumentException();
    }
    
    // ^ above code is still synchronous
    await Task.Delay(500);
    
    return a / b;
}
  • SonarQube Version 8.1 (build 31237)

Reading Bill Wagner’s blog post on this topic: Async, Exceptions and Library Design, I can see discrepancy in two C# rules. However I’m still not convinced about this approach - after all, “Exception Driven Development” is hopefully overcame (exceptions are exceptional - if wrong input is expected, sure, faulted task makes sense to me). IMHO splitting method will cause clutter in code and possibility of wrong usage (calling implementation method from within class without argument validation).

3

Hi Zdenek,

Can you please clarify in more detail what you mean by this?

When I look at the compiler-generated code from MSIL, there’s a state machine that wraps the exception into faulted task. So the Exception is throw synchronously (from throw syntax point of view), but it’s not thrown outside of the method. It’s instead immediately caught inside the state machine and returned as failed task.

Rule reasoning seems fine then.