Omit parameters in wrapped inner async methods?

Question

Wrapping async methods in a synchronous part for parameter checks, as required by SonarQube code rule S4457, can quickly become hard to read. With C# local methods, the parameters can be omitted for the local method.

Does this omission of parameters have any disadvantages or issues with the compiler and local methods?

public Task<MyResult> GetMyResultAsync(string myId, string someOption, CancellationToken cancellationToken)
{
    // check arguments synchronously, before entering async context:
    myId = myId ?? throw new ArgumentNullException(nameof(myId));
    someOption = someOption ?? throw new ArgumentNullException(nameof(someOption));
    return InnerAsync();

    // don't replicate the arguments, use from outer method.
    async Task<MyResult> InnerAsync()
    {
        using var httpClient = _httpClientFactory.CreateClient(CfgName);
        var response = await httpClient.GetAsync(
                CreateUrl(myId, someOption), cancellationToken)
            .ConfigureAwait(false);
        return ParseResponse<MyResult>(response);
    }
}

as compared to:

public Task<MyResult> GetMyResultAsync(string myId, string someOption, CancellationToken cancellationToken)
{
    // check arguments synchronously, before entering async context:
    myId = myId ?? throw new ArgumentNullException(nameof(myId));
    someOption = someOption ?? throw new ArgumentNullException(nameof(someOption));
    return InnerAsync(myId, someOption, cancellationToken);

    // new arguments for inner async method:
    async Task<MyResult> InnerAsync(
       string myIdInner, string someOptionInner, 
       CancellationToken cancellationTokenInner)
    {
        using var httpClient = _httpClientFactory.CreateClient(CfgName);
        var response = await httpClient.GetAsync(
                CreateUrl(myIdInner, someOptionInner), 
                cancellationTokenInner)
            .ConfigureAwait(false);
        return ParseResponse<MyResult>(response);
    }
}

Background: The rule requires parameter check, throwing Argument(Null/OutOfRange)Exception, to happen synchronously, before entering the async context, so that these exceptions don't get lost in case of fire-and-forget; they will be thrown before an async task is even created. The methods are still named "-Async", since they were before the rule was applied.

I know that async methods are basically compiled into state machines, with local variables as properties, so I wouldn't expect any disadvantage from using the outer variables. Correct?

[See here](https://sharplab.io/#v2:EYLgtghglgdgNAFxBAzmAPgAQEwEYCwAUJgAwAEmuArANxGYDMF2ZAwmQN5Fk8VOYA2MgFkAFJXIpccCrknYZEsigYBKTt15bMAdjIBJGDACmAJzGq6hLVs02KADgpDDJ86NV2bXa/ZuYATmcAOgARYwAbCABPUVxLLz9ZANEpMgBqZRZMlUteRN4AXy9i3x4vRmcyADlxOWVpWXlFetyvHyTdAyMzWqkZFAVlNSskgp5KIUwnQW63WqV+pqyWyTVxjTKkiiDBMMiYuIStztwUtJzs4bzyk6KSokKgA=). The version which captures args does slightly more work, but it's pretty insignificant in the context of `async` code — canton7, Jan 10 '22 at 10:27
Are you writing a library or this is part of application code? If you are not writing a library you could ignore this rule IMHO. — Theodor Zoulias, Jan 10 '22 at 10:30
It is part of a company library, containing various REST clients. I really don't like the rule, since many applications have been written in a way to expect async methods being really asynchronous, including parameter checks. Meaning: fire and forget should never throw; if *AggregateException* is thrown (*.Wait(), .Result*), an *ArgumentException* should be wrapped in it, too. — Erik Hart, Jan 10 '22 at 10:43
@ErikHart: IMO, parameter checks are a kind of [boneheaded exception](https://ericlippert.com/2008/09/10/vexing-exceptions/), which should never be caught. So it doesn't matter if they're wrapped in an `AggregateException`, and it doesn't matter if they're thrown synchronously. Since the calling code shouldn't ever catch them, it should have no expectations around how they're thrown. IMO. :) — Stephen Cleary, Jan 10 '22 at 14:08

Theodor Zoulias · Answer 1 · 2022-01-10T12:07:13.940

0

If you hover your mouse over the InnerAsync local function in the Visual Studio, you'll see the variables that are captured:

This means that the C# compiler will have to create an instance of an invisible autogenerated struct that holds these variables as fields. Be aware that the fields of the autogenerated struct are mutable. Other than that, there are no problems with capturing variables in order to simplify your code.

If you don't want to use this language feature for some reason, make sure to mark your local functions as static, so that you don't create closures unwillingly by mistake.

edited Jan 10 '22 at 12:07

answered Jan 10 '22 at 11:29

Theodor Zoulias

34,835
7
69
104

1

Note that it generates an inner struct (unless it needs to be a class for other reasons), so no GC pressure – canton7 Jan 10 '22 at 11:43
1

@canton7 [you are right](https://sharplab.io/#v2:EYLgtghglgdgPgAQAwAIEEYCsBuAsAKAIQGY0AmFAYQIG8CUG1SEA2FAWQApYAXFAEwCmAGwgBPAJT1GdfI3loA7CgCSMGIIBOXCXjkKG0g61XqtOowtkHjy1gDoAIiPGchoyXpsoAvpb/4PkA==), it creates a `struct` passed by `ref`. – Theodor Zoulias Jan 10 '22 at 11:53
2

It needs to be a class [for async methods](https://sharplab.io/#v2:EYLgtghglgdgPgAQAwAIEEYCsBuAsAKAIQGY0AmFAYQIG8CUG1SEA2FAWQApYAXFAEwCmAGwgBPAJT1GdfI3loA7CgCSMGIIBOXCXjkKG0gwgAcaNmo3bOU/QdkHjATnMA6ACIjxnIaMl7HFABfI2CCIKA==), or if it's [used as a delegate](https://sharplab.io/#v2:EYLgtghglgdgPgAQAwAIEEYCsBuAsAKAIQGY0AmFAYQIG8CUG1SEA2FAWQApYAXFAEwCmAGwgBPAJT1GdfI3koAbhABOAlAF4UASRgxBK9njkKGCAOw69BrhOOmG006yv7DnKSdOyHCi2hYAOgAREXFOIVFJewcAXycUePxYoA==) etc. But it uses a struct if it can. – canton7 Jan 10 '22 at 11:56
1

Good hint to watch captures in the Visual Studio mouseover text. I noticed now, playing around with dotPeek decompiler, that capturing an outer variable in the local function creates an *additional* display class, with the variable as field, and then inside the state machine class for the async method. The variable/field is then referenced via "this" reference in the state machine. I did not notice any structs in a local example, only sealed class - maybe different depending on .NET version. – Erik Hart Jan 10 '22 at 14:10
@TheodorZoulias When I changed your `InnerM()` method to async-await on the website, a sealed display class appeared. However, the async state machine is a struct there, while it is a class in my local disassembly. Apart from class/struct, I get the same locally. – Erik Hart Jan 10 '22 at 14:30
1

@ErikHart make sure to compile your project in Release mode, to see the real picture. – Theodor Zoulias Jan 10 '22 at 14:39
1

OK, it was still Debug, is now struct in Release mode. – Erik Hart Jan 10 '22 at 14:47
Considering the mess which the separate sync-async method parts create, I now tend to prefer the version with captured parameters. While there's some overhead on the compiler side, and a minor performance penalty, it makes the code easier to read with seemingly the same variables, rather than similar, duplicated ones. Not sure if it might generate problems with debugging later... – Erik Hart Jan 10 '22 at 14:56
@ErikHart if you mark the local function as `static`, you will be able to use the same names for its parameters, because there will be no interference with the enclosing method. – Theodor Zoulias Jan 10 '22 at 15:33
@TheodorZoulias Static might be a good idea, if really no instance data is needed. But then it is totally static. In my case, the async methods use data from the main class (I changed to *_httpClientFactory* with underscore, as an example for a class field or property). – Erik Hart Jan 10 '22 at 15:55
@ErikHart in that case you could include a parameter that corresponds to `this` in the static local function. I would expect to see this level of strictness from the source code of the .NET itself, or from third-party libraries that hold performance as their selling point, but for a library intended for internal corporate usage it might be overkill. – Theodor Zoulias Jan 10 '22 at 16:11

Omit parameters in wrapped inner async methods?

1 Answers1