Bug in WeakAction in case of Closure Action

Question

In one of the projects I take part in there is a vast use of WeakAction. That's a class that allows to keep reference to an action instance without causing its target not be garbage collected. The way it works is simple, it takes an action on the constructor, and keeps a weak reference to the action's target and to the Method but discards the reference to the action itself. When the time comes to execute the action, it checks if the target is still alive, and if so, invokes the method on the target.

It all works well except for one case - when the action is instantiated in a closure. consider the following example:

public class A 
{
     WeakAction action = null;

     private void _register(string msg) 
     {
         action = new WeakAction(() => 
         {
             MessageBox.Show(msg);
         }
     }
}

Since the lambda expression is using the msg local variable, the C# compiler auto generates a nested class to hold all the closure variables. The target of the action is an instance of the nested class instead of the instance of A. The action passed to the WeakAction constructor is not referenced once the constructor is done and so the garbage collector can dispose of it instantly. Later, if the WeakAction is executed, it will not work because the target is not alive anymore, even though the original instance of A is alive.

Now I can't change the way the WeakAction is called, (since it's in wide use), but I can change its implementation. I was thinking about trying to find a way to get access to the instance of A and to force the instance of the nested class to remain alive while the instance of A is still alive, but I don't know how to do get it.

There are a lot of questions about what A has to do with anything, and suggestions to change the way A creates a weak action (which we can't do) so here is a clarification:

An instance of class A wants an instance of class B to notify it when something happens, so it provides a callback using an Action object. A is not aware that B uses weak actions, it simply provides an Action to serve as callback. The fact that B uses WeakAction is an implementation detail that is not exposed. B needs to store this action and use it when needed. But B may live much longer then A, and holding a strong reference to a normal Action (which by itself holds a strong reference of the instance of A that generated it) causes A to never be garbage collected. If A is a part of a list of items that are no longer alive, we expect A to be garbage collected, and because of the reference that B holds of the Action, which by itself points to A, we have a memory leak.

So instead of B holding an Action that A provided, B wraps it into a WeakAction and stores the weak action only. When the time comes to call it, B only does so if theWeakAction is still alive, which it should be as long as A is still alive.

A creates that action inside a method and does not keep a reference to it on his own - that's a given. Since the Action was constructed in the context of a specific instance of A, that instance is the target of A, and when A dies, all weak references to it become null so B knows not to call it and disposes of the WeakAction object.

But sometimes the method that generated the Action uses variables defined locally in that function. In which case the context in which the action runs include not just the instance of A, but also the state of the local variables inside of the method (that is called a "closure"). The C# compiler does that by creating a hidden nested class to hold these variables (lets call it A__closure) and the instance that becomes the target of the Action is an instance of A__closure, not of A. This is something that the user should not be aware of. Except that this instance of A__closure is only referenced by the Action object. And since we create a weak reference to the target, and do not hold a reference to the action, there is no reference to the A__closure instance, and the garbage collector may (and usually does) dispose of it instantly. So A lives, A__closure dies, and despite the fact that A is still expecting the callback to be invoked, B can not do it.

That's the bug.

My question was if somebody knows of a way that the WeakAction constructor, the only piece of code that actually holds the original Action object, temporarily, can in some magic way extract the original instance of A from the A__closure instance that it finds in the Target of the Action. If so, I could perhaps extend A__Closure life cycle to match that of A.

Answer 1

After some more research and after collecting all the useful bits of information from the answers that were posted here, I realized that there is not going to be an elegant and sealed solution to the problem. Since this is a real life problem we went with the pragmatic approach, trying to at least reduce it by handling as many scenarios as possible, so I wanted to post what we did.

Deeper investigation of the Action object that is passed to the constructor of the WeakEvent, and especially the Action.Target property, showed that there are effectively 2 different cases of closure objects.

The first case is when the Lambda uses local variables from the scope of the calling function, but does not use any information from the instance of the A class. In the following example, assume EventAggregator.Register is a method that takes an action and stores a WeakAction that wraps it.

public class A 
{
    public void Listen(int num) 
    {
        EventAggregator.Register<SomeEvent>(_createListenAction(num));
    }

    public Action _createListenAction(int num) 
    {
        return new Action(() => 
        {
            if (num > 10) MessageBox.Show("This is a large number");
        });
    }
}

The lambda created here uses the num variable, which is a local variable defined in the scope of the _createListenAction function. So the compiler has to wrap it with a closure class in order maintain the closure variables. However, since the lambda does not access any of class A members, there is no need to store a reference to A. The target of the action will therefore not include any reference to the A instance and there is absolutely no way for the WeakAction constructor to reach it.

The second case is illustrated in the following example:

public class A 
{
   int _num = 10;

    public void Listen() 
    {
        EventAggregator.Register<SomeEvent>(_createListenAction());
    }

    public Action _createListenAction() 
    {
        return new Action(() => 
        {
            if (_num > 10) MessageBox.Show("This is a large number");
        });
    }
}

Now _num is not provided as parameter to the function, it comes from the class A instance. Using reflection to learn about the structure of the Target object reveals that the last field the the compiler defines holds a reference to the A class instance. This case also applies when the lambda contains calls to member methods, as in the following example:

public class A 
{
    private void _privateMethod() 
    {
       // do something here
    }        

    public void Listen() 
    {
        EventAggregator.Register<SomeEvent>(_createListenAction());
    }

    public Action _createListenAction() 
    {
        return new Action(() => 
        {
             _privateMethod();
        });
    }
}

_privateMethod is a member function, so it is called in the context of the class A instance, so the closure must keep a reference to it in order to invoke the lambda in the right context.

So the first case is a Closure that only contains functions local variable, the second contains reference to the parent A instance. In both cases, there are no hard references to the Closure instance, so if the WeakAction constructor just leaves things the way they are, the WeakAction will "die" instantly despite the fact that the class A instance is still alive.

We are faced here with 3 different problems:

1. How to identify that the target of the action is a nested closure class instance, and not the original A instance?
2. How to obtain a reference to the original class A instance?
3. How to extend the life span of the closure instance so that it lives as long as the A instance live, but not beyond that?

The answer to the first question is that we rely on 3 characteristics of the closure instance: - It is private (to be more accurate, it is not "Visible". When using C# compiler, the reflected type has IsPrivate set to true but with VB it does not. In all cases, the IsVisible property is false). - It is nested. - As @DarkFalcon mentioned in his answer, It is decorated with the [CompilerGenerated] attribute.

private static bool _isClosure(Action a)
{
    var typ = a.Target.GetType();
    var isInvisible = !typ.IsVisible;
    var isCompilerGenerated = Attribute.IsDefined(typ, typeof(CompilerGeneratedAttribute));
    var isNested = typ.IsNested && typ.MemberType == MemberTypes.NestedType;


    return isNested && isCompilerGenerated && isInvisible;
}

While this is not a 100% sealed predicate (a malicious programmer may generate a nested private class and decorate it with the CompilerGenerated attribute), in real life scenarios this is accurate enough, and again, we are building a pragmatic solution, not an academic one.

So problem number 1 is solved. The weak action constructor identifies situations where the action target is a closure and responds to that.

Problem 3 is also easily solvable. As @usr wrote in his answer, once we get a hold of the A class instance, adding a ConditionalWeakTable with a single entry where the A class instance is the key and the closure instance is the target, solves the problem. The garbage collector knows not to collect the closure instance as long as the A class instance lives. So that's ok.

The only non - solvable problem is the second one, how to obtain a reference to the class A instance? As I said, there are 2 cases of closures. One where the compiler creates a member that holds this instance, and one where it doesn't. In the second case, there is simply no way to get it, so the only thing we can do is to create a hard reference to the closure instance in order to save it from being instantly garbage collected. This means that it may out live the class A instance (in fact it will live as long as the WeakAction instance lives, which may be forever). But this is not such a terrible case after all. The closure class in this case only contains a few local variables, and in 99.9% of the cases it is a very small structure. While this is still a memory leak, it is not a substantial one.

But just in order to allow users to avoid even that memory leak, we have now added an additional constructor to the WeakAction class, as follows:

public WeakAction(object target, Action action) {...}

And when this constructor is called, we add a ConditionalWeakTable entry where the target is the key and the actions target is the value. We also hold a weak reference to both the target and the actions target and if any of them die, we clear both. So that the actions target lives no less and no more than the provided target. This basically allows the user of the WeakAction to tell it to hold on to the closure instance as long as the target lives. So new users will be told to use it in order to avoid memory leaks. But in existing projects, where this new constructor is not used, this at least minimizes the memory leaks to closures that have no reference to the class A instance.

The case of closures that reference the parent are more problematic because they affect garbase collection. If we hold a hard reference to the closure, we cause a much more drastic memory leak becuase the class A instance will also never be cleared. But this case is also easier to treat. Since the compiler adds a last member that holds a reference to the class A instance, we simply use reflection to extract it and do exactly what we do when the user provides it in the constructor. We identify this case when the last member of the closure instance is of the same type as the declaring type of the closure nested class. (Again, its not 100% accurate, but for real life cases its close enough).

To summarize, the solution I presented here is not 100% sealed solution, simply because there does not seem to be a such solution. But since we have to provide SOME answer to this annoying bug, this solution at least reduces the problem substantially.

Answer 2

You want to extend the lifetime of the closure class instance to be exactly the same that the A instance has. The CLR has a special GC handle type for that: the Ephemeron, implemented as internal struct DependentHandle.

1. This struct is only exposed as part of the ConditionalWeakTable class. You could create one such table per WeakAction with exactly one item in it. The key would be an instance of A, the value would be the closure class instance.
2. Alternatively, you could pry open DependentHandle using private reflection.
3. Or, you could use one globally shared ConditionalWeakTable instance. It probably requires synchronization to use. Look at the docs.

Consider opening a connect issue to make DependentHandle public and link to this question to provide a use case.

Answer 3

a.Target provides access to the object which holds the lambda parameters. Performing a GetType on this will return the compiler-generated type. One option would be to check this type for the custom attribute System.Runtime.CompilerServices.CompilerGeneratedAttribute and keep a strong reference to the object in this case.

Now I can't change the way the WeakAction is called, (since it's in wide use) but I can change it's implementation. Note that this is the only way so far which can keep it alive without requiring modifications to how the WeakAction is constructed. It also doesn't attain the goal of keeping the lambda alive as long as the A object (it would keep it alive as long as the WeakAction instead). I do not believe that is going to be attainable without changing how the WeakAction is constructed as is done in the other answers. At a minimum, the WeakAction needs to obtain a reference to the A object, which you currently do not provide.