WeakEventManager holds reference to subscriber

Question

I have been using WeakEventManager to avoid memory leaks, and i started to overuse them. I created extension methods for example for the INotifyPropertyChanged like:

public static void AddWeakPropertyChanged(this INotifyPropertyChanged item, Action handler)
{
    PropertyChangedEventManager.AddHandler(item, (s, e) => handler(e.PropertyName), string.Empty);
}

Now i quickly realized, that this doesn't work. In fact, you can't really use anonymous methods for weak event handling. (If i understand correctly, then the compiler creates a 'closure class' for it (to hold referenced values), which has the handler, but since your closure class is not referenced anywhere, the GC will clear it away, and the event handler will not be called)

Question #1: Is that correct? I mean is it correct, then when using an anonymous method (or lambda) for a weak event handler, the handler is called only if the GC did not run in the meanwhile (e.g. it is undeterministic)?

Well, i thaught so, so i did some unit tests to make sure i get it right. It seemed allright until i hit the following unit test:

        class DidRun
        {
            public bool Value { get; set; }
        }
        class TestEventPublisher
        {
            public event EventHandler<EventArgs> MyEvent;
            public void RaiseMyEvent()
            {
                if (MyEvent != null)
                    MyEvent(this, EventArgs.Empty);

            }
        }
        class TestClosure
        {
            public DidRun didRun { get; set; }
            public EventHandler<EventArgs> Handler { get; private set; }
            public TestClosure()
            {
                this.Handler = new EventHandler<EventArgs>((s, e) => didRun.Value = true);
            }
        }
        [TestMethod]
        public void TestWeakReference()
        {
            var raiser = new TestEventPublisher();
            var didrun = new DidRun();
            var closure = new TestClosure { didRun = didrun };
            WeakEventManager<TestEventPublisher, EventArgs>.AddHandler(raiser, "MyEvent", closure.Handler);
            closure = null;

            GC.Collect();
            GC.Collect();
            raiser.RaiseMyEvent();
            Assert.AreEqual(false, didrun.Value);
        }

Question #2: Can anyone explain my why does this test fail?

Expectation: Here i do not have any closures (i took them out, to make sure what is happening), i simply have an object (closure), that subscribes to an event with the WeakEventManager, and then i drop the reference to it (closure = null;).

I was expecting the 2 GC.Collect() calls, to clean up my old closure class, so the WeakEventManager would drop the subscriber, and not run the handler, but the test fails. Any ideas?

EDIT: Sorry, the generic arguments were not visible, now they are

Answer 1

You are correct that the GC will collect the closure that was created around your lambda if there is no reference to it.

In your unit test you null out the local instance of TestClosure but you've passed a hard reference of the handler into WeakEventManager, not an instance of TestClosure. So the handler lives on...

I believe these examples demonstrate your trouble with the closure:

class DidRun
{
    public bool Value { get; set; }
}

class TestEventPublisher
{
    public event EventHandler<EventArgs> MyEvent;
    public void RaiseMyEvent()
    {
        if (MyEvent != null)
            MyEvent(this, EventArgs.Empty);
    }
}

class TestClosure
{
    static public EventHandler<EventArgs> Register(TestEventPublisher raiser, DidRun didrun)
    {
        EventHandler<EventArgs> handler = (s, e) => didrun.Value = true;
        WeakEventManager<TestEventPublisher, EventArgs>.AddHandler(raiser, "MyEvent", handler);
        return handler;
    }
}

[TestMethod]
public void Test1()
{
    var raiser = new TestEventPublisher();
    var didrun = new DidRun();

    TestClosure.Register(raiser, didrun);

    // The reference to the closure 'handler' is not being held,
    //  it may or may not be GC'd (indeterminate result)

    raiser.RaiseMyEvent();
    Assert.IsTrue(didrun.Value);
}

[TestMethod]
public void Test2()
{
    var raiser = new TestEventPublisher();
    var didrun = new DidRun();

    // The reference to the closure 'handler' is not being held, it's GC'd
    TestClosure.Register(raiser, didrun);

    GC.Collect();
    GC.Collect();

    raiser.RaiseMyEvent();
    Assert.IsFalse(didrun.Value);
}

[TestMethod]
public void Test3()
{
    var raiser = new TestEventPublisher();
    var didrun = new DidRun();

    // Keep local copy of handler to prevent it from being GC'd
    var handler = TestClosure.Register(raiser, didrun);

    GC.Collect();
    GC.Collect();

    raiser.RaiseMyEvent();
    Assert.IsTrue(didrun.Value);
}

As for your original issue, you can try to save the handler (closure) to prevent it from being GC'd. A ConditionalWeakTable should work for this:

// ConditionalWeakTable will hold the 'value' as long as the 'key' is not marked for GC
static private ConditionalWeakTable<INotifyPropertyChanged, EventHandler<PropertyChangedEventArgs>> _eventMapping =
  new ConditionalWeakTable<INotifyPropertyChanged, EventHandler<PropertyChangedEventArgs>>();

public static void AddWeakPropertyChanged(this INotifyPropertyChanged item, Action<string> handlerAction)
{
    EventHandler<PropertyChangedEventArgs> handler;

    // Remove any existing handler for this item in case it's registered more than once
    if (_eventMapping.TryGetValue(item, out handler))
    {   
        _eventMapping.Remove(item);
        PropertyChangedEventManager.RemoveHandler(item, handler, string.Empty);
    }   

    handler = (s, e) => handlerAction(e.PropertyName);

    // Save handler (closure) to prevent GC
    _eventMapping.Add(item, handler);

    PropertyChangedEventManager.AddHandler(item, handler, string.Empty);
}

class DidRun
{
    static public string Value { get; private set; }
    public void SetValue(string value) { Value = value; }
}

[TestMethod]
public void Test4()
{
    var property = new ObservableObject<string>();

    var didrun = new DidRun();
    property.AddWeakPropertyChanged(
        (x) => 
        {
            didrun.SetValue("Property Name = " + x);
        });

    GC.Collect();
    GC.Collect();

    property.Value = "Hello World";

    Assert.IsTrue(DidRun.Value != null);
}