Meaning and problems of closure allocation

Question

I'm using JetBrains Rider for programming C#, and I guess this warning will also appear when using ReSharper:

I wrote this function, GetTicket:

public async Task<IEnumerable<Ticket>> GetTicket(int id)
{
    return await _memoryCache.GetOrCreateAsync(_cachingFunctionalty.BuildCachingName(id), entry =>
    {
        entry.SlidingExpiration = TimeSpan.FromSeconds(10);
        return GetTicket_uncached(id);
    });
}

and GetTicket_uncached, which it calls:

private async Task<IEnumerable<Ticket>> GetTicket_uncached(int id)
{
    RestClient client = new RestClient(ServiceAdress);
    Request req = new Request
    {
        Method = Method.GET,
        Resource = "api/tickets/get",
        Parameters = new {ident = id}
    };

    return await client.ExecuteRequestAsync<Ticket[]>(req);
}

So, the parameter id in method public async Task<IEnumerable<Ticket>> GetTicket(int id) is highlighted with following warning:

Closure allocation: 'id' parameter and 'this' reference

I found a few things while Googling but I still don't know what this means and what's the problem?

Hyohyoid

just create a var localId = id and use localid instead of id, what is simply means is using variable which can be modified outside the scope of current closure and thus can lead to unpredictable results, especially when you work with multi threading – Monarchal

@MrinalKamboj warning is probably a false positive. there is no need for that here. – Moureaux

@M.kazemAkhgary It is indeed, but it is the matter of good practice, since same mistake can be done later by modifying a variable outside the scope of closure – Monarchal

Resharper is a decent product but their analyzers have a high false positive rate, which leads people to worry, and to ask questions here about what's going on. Nothing is going on; the analyzer is not sophisticated enough to tell a bad use of this pattern from a good one. – Arceliaarceneaux

@EricLippert so there is no problematic code here? Yes, I just switched from Visual Studio Code to Rider, and there are thousands of warnings :D – Hyohyoid

@MatthiasBurger: id is a primitive being passed by value, so the analysis is pretty simple. In fact, I'd argue that it's simple enough that Resharper ought to do it. Consistent, sometimes spurious warnings will lead towards consistent, sometimes spurious code...which may be a tradeoff Jetbrains made intentionally. – Benzidine

The answer by @citizenmatt is actually the right one. This "xxx allocation" highlightings should only be considered as warnings if your application is performance critical. Otherwise, it is just an information. – Kellerman

Answer 1

It is expecting you to design as follows:

public async Task<IEnumerable<Ticket>> GetTicket(int id)
{

    return await _memoryCache.GetOrCreateAsync(_cachingFunctionalty.BuildCachingName(id), entry =>
    {
        var localId = id;
        entry.SlidingExpiration = TimeSpan.FromSeconds(10);
        return GetTicket_uncached(localId);
    });
}

Now you don't have a variable that can be modified outside the scope of closure, Re-sharper will give this warning, when ever it is theoretically possible to modify a variable outside the executing closure and thus leading to unpredictable results. Same will apply to other method. In act you shall follow the same for the all other objects, which can be modified outside the closure scope, creating a local version

Monarchal

Is this the same warning as "Access to Modified Closure"? – Freestanding

@StephenKennedy yes that's what the warning means – Monarchal

when I do this, the localId is highlighted with same warning :D – Hyohyoid

I don't have access to VS right now, mostly I have added that to the wrong location add the same inside the curly bracket, where you are supplying a Func delegate – Monarchal

Check the modified version – Monarchal

Answer 2

This message comes from the Heap Allocations Viewer plugin. When you're creating your lambda to pass as a Func to GetOrCreateAsync, you're capturing some values from the calling method (GetTicket) and using them later.

When compiling this code, the compiler will rewrite this lambda into a class that holds the values and also a method whose body is the same as the body of the lambda, although it will use the values captured in this new class, and not the original method call.

What the Heap Allocations Viewer plugin is saying is that there is a hidden allocation happening here at runtime - this new compiler-generated class is being allocated, the values assigned and the method called.

The plugin is telling you that id is being captured and allocated in this new class - this is obvious from the lambda because you see it in the code. But you're also capturing this, because GetTicket_uncached is an instance method and not a static method. You can't call an instance method without this, so both id and this are captured and allocated in the compiler generated class.

You can't get rid of the allocation of the id variable, but you could get rid of the this reference if you make GetTicket_uncached static (but this might require passing in ServiceAddress, in which case, the Heap Allocations Viewer will tell you that you're closure allocation is now id and ServiceAddress).

You can see some more detail in the ReSharper help page for the "Implicitly capture closure" warning. While it talks about a different scenario and warning message, the background details on allocating classes to capture variables is useful.

Sweptwing