Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Why does ReSharper tell me "implicitly captured closure"?

Tags:

c#

linq

resharper

I have the following code:

public double CalculateDailyProjectPullForceMax(DateTime date, string start = null, string end = null)
{
    Log("Calculating Daily Pull Force Max...");

    var pullForceList = start == null
                             ? _pullForce.Where((t, i) => _date[i] == date).ToList() // implicitly captured closure: end, start
                             : _pullForce.Where(
                                 (t, i) => _date[i] == date && DateTime.Compare(_time[i], DateTime.Parse(start)) > 0 && 
                                           DateTime.Compare(_time[i], DateTime.Parse(end)) < 0).ToList();

    _pullForceDailyMax = Math.Round(pullForceList.Max(), 2, MidpointRounding.AwayFromZero);

    return _pullForceDailyMax;
}

Now, I've added a comment on the line that ReSharper is suggesting a change. What does it mean, or why would it need to be changed? implicitly captured closure: end, start

like image 755
PiousVenom Avatar asked Nov 29 '12 19:11

PiousVenom


5 Answers

The warning tells you that the variables end and start stay alive as any of the lambdas inside this method stay alive.

Take a look at the short example

protected override void OnLoad(EventArgs e)
{
    base.OnLoad(e);

    int i = 0;
    Random g = new Random();
    this.button1.Click += (sender, args) => this.label1.Text = i++.ToString();
    this.button2.Click += (sender, args) => this.label1.Text = (g.Next() + i).ToString();
}

I get an "Implicitly captured closure: g" warning at the first lambda. It is telling me that g cannot be garbage collected as long as the first lambda is in use.

The compiler generates a class for both lambda expressions and puts all variables in that class which are used in the lambda expressions.

So in my example g and i are held in the same class for execution of my delegates. If g is a heavy object with a lot of resources left behind, the garbage collector couldn't reclaim it, because the reference in this class is still alive as long as any of the lambda expressions is in use. So this is a potential memory leak, and that is the reason for the R# warning.

@splintor As in C# the anonymous methods are always stored in one class per method there are two ways to avoid this:

  1. Use an instance method instead of an anonymous one.

  2. Split the creation of the lambda expressions into two methods.

like image 94
quadroid Avatar answered Oct 04 '22 19:10

quadroid


Agreed with Peter Mortensen.

The C# compiler generates only one type that encapsulates all variables for all lambda expressions in a method.

For example, given the source code:

public class ValueStore
{
    public Object GetValue()
    {
        return 1;
    }

    public void SetValue(Object obj)
    {
    }
}

public class ImplicitCaptureClosure
{
    public void Captured()
    {
        var x = new object();

        ValueStore store = new ValueStore();
        Action action = () => store.SetValue(x);
        Func<Object> f = () => store.GetValue();    //Implicitly capture closure: x
    }
}

The compiler generates a type looks like :

[CompilerGenerated]
private sealed class c__DisplayClass2
{
  public object x;
  public ValueStore store;

  public c__DisplayClass2()
  {
    base.ctor();
  }

  //Represents the first lambda expression: () => store.SetValue(x)
  public void Capturedb__0()
  {
    this.store.SetValue(this.x);
  }

  //Represents the second lambda expression: () => store.GetValue()
  public object Capturedb__1()
  {
    return this.store.GetValue();
  }
}

And the Capture method is compiled as:

public void Captured()
{
  ImplicitCaptureClosure.c__DisplayClass2 cDisplayClass2 = new ImplicitCaptureClosure.c__DisplayClass2();
  cDisplayClass2.x = new object();
  cDisplayClass2.store = new ValueStore();
  Action action = new Action((object) cDisplayClass2, __methodptr(Capturedb__0));
  Func<object> func = new Func<object>((object) cDisplayClass2, __methodptr(Capturedb__1));
}

Though the second lambda does not use x, it cannot be garbage collected as x is compiled as a property of the generated class used in the lambda.

like image 28
Smartkid Avatar answered Oct 04 '22 20:10

Smartkid


The warning is valid and displayed in methods that have more than one lambda, and they capture different values.

When a method that contains lambdas is invoked, a compiler-generated object is instantiated with:

  • instance methods representing the lambdas
  • fields representing all values captured by any of those lambdas

As an example:

class DecompileMe
{
    DecompileMe(Action<Action> callable1, Action<Action> callable2)
    {
        var p1 = 1;
        var p2 = "hello";

        callable1(() => p1++);    // WARNING: Implicitly captured closure: p2

        callable2(() => { p2.ToString(); p1++; });
    }
}

Examine the generated code for this class (tidied up a little):

class DecompileMe
{
    DecompileMe(Action<Action> callable1, Action<Action> callable2)
    {
        var helper = new LambdaHelper();

        helper.p1 = 1;
        helper.p2 = "hello";

        callable1(helper.Lambda1);
        callable2(helper.Lambda2);
    }

    [CompilerGenerated]
    private sealed class LambdaHelper
    {
        public int p1;
        public string p2;

        public void Lambda1() { ++p1; }

        public void Lambda2() { p2.ToString(); ++p1; }
    }
}

Note the instance of LambdaHelper created stores both p1 and p2.

Imagine that:

  • callable1 keeps a long-lived reference to its argument, helper.Lambda1
  • callable2 does not keep a reference to its argument, helper.Lambda2

In this situation, the reference to helper.Lambda1 also indirectly references the string in p2, and this means that the garbage collector will not be able to deallocate it. At worst it is a memory/resource leak. Alternatively it may keep object(s) alive longer than otherwise needed, which can have an impact on GC if they get promoted from gen0 to gen1.

like image 31
Drew Noakes Avatar answered Oct 04 '22 20:10

Drew Noakes


For Linq to Sql queries, you may get this warning. The lambda's scope may outlive the method due to the fact that the query is often actualized after the method is out of scope. Depending on your situation, you may want to actualize the results (i.e. via .ToList()) within the method to allow for GC on the method's instance vars captured in the L2S lambda.

like image 45
Jason Dufair Avatar answered Oct 04 '22 20:10

Jason Dufair


You could always figure out with a reasons of R# suggestions just by clicking on the hints like shown below:

enter image description here

This hint will direct you here.


This inspection draws your attention to the fact that more closure values are being captured than is obviously visibly, which has an impact on the lifetime of these values.

Consider the following code:

using System; 
public class Class1 {
    private Action _someAction;

    public void Method() {
        var obj1 = new object();
        var obj2 = new object();

        _someAction += () => {
            Console.WriteLine(obj1);
            Console.WriteLine(obj2);
        };

        // "Implicitly captured closure: obj2"
        _someAction += () => {
            Console.WriteLine(obj1);
        };
    }
}

In the first closure, we see that both obj1 and obj2 are being explicitly captured; we can see this just by looking at the code. For the second closure, we can see that obj1 is being explicitly captured, but ReSharper is warning us that obj2 is being implicitly captured.

This is due to an implementation detail in the C# compiler. During compilation, closures are rewritten into classes with fields that hold the captured values, and methods that represent the closure itself. The C# compiler will only create one such private class per method, and if more than one closure is defined in a method, then this class will contain multiple methods, one for each closure, and it will also include all captured values from all closures.

If we look at the code that the compiler generates, it looks a little like this (some names have been cleaned up to ease reading):

public class Class1 {
    [CompilerGenerated]
    private sealed class <>c__DisplayClass1_0
    {
        public object obj1;
        public object obj2;

        internal void <Method>b__0()
        {
            Console.WriteLine(obj1);
            Console.WriteLine(obj2);
        }

        internal void <Method>b__1()
        {
            Console.WriteLine(obj1);
        }
    }

    private Action _someAction;

    public void Method()
    {
        // Create the display class - just one class for both closures
        var dc = new Class1.<>c__DisplayClass1_0();

        // Capture the closure values as fields on the display class
        dc.obj1 = new object();
        dc.obj2 = new object();

        // Add the display class methods as closure values
        _someAction += new Action(dc.<Method>b__0);
        _someAction += new Action(dc.<Method>b__1);
    }
}

When the method runs, it creates the display class, which captures all values, for all closures. So even if a value isn't used in one of the closures, it will still be captured. This is the "implicit" capture that ReSharper is highlighting.

The implication of this inspection is that the implicitly captured closure value will not be garbage collected until the closure itself is garbage collected. The lifetime of this value is now tied to the lifetime of a closure that does not explicitly use the value. If the closure is long lived, this might have a negative effect on your code, especially if the captured value is very large.

Note that while this is an implementation detail of the compiler, it is consistent across versions and implementations such as Microsoft (pre and post Roslyn) or Mono's compiler. The implementation must work as described in order to correctly handle multiple closures capturing a value type. For example, if multiple closures capture an int, then they must capture the same instance, which can only happen with a single shared private nested class. The side effect of this is that the lifetime of all captured values is now the maximum lifetime of any closure that captures any of the values.

like image 20
anatol Avatar answered Oct 04 '22 18:10

anatol