Windowscommunitytoolkit: WeakEventListener is not weak

[dotMorten]

Describe the bug

The current implementation of the weak event listener isn't actually weak. That's because the DetachAction actually captures the object.
For example incc is captured here:
https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/f09b6d4d5dd26b256992d1a19b98c20a5b357181/Microsoft.Toolkit.Uwp.UI.Controls/RotatorTile/RotatorTile.cs#L445

To fix it however, you'd have to change the signature of the DetachAction to not just send the listener but also the instance, which would be a breaking (however necessary) change.

Ie change:
public Action<WeakEventListener<TInstance, TSource, TEventArgs>> OnDetachAction { get; set; }
to
public Action<TInstance, WeakEventListener<TInstance, TSource, TEventArgs>> OnDetachAction { get; set; }

And change the Detach function to:

        public void Detach()
        {
            TInstance target = (TInstance)weakInstance.Target;
            OnDetachAction?.Invoke(target, this);
            OnDetachAction = null;
        }

This would break all uses of the WeakEventListener. The class however is marked non-browsable (not sure if that matters wrt breaking the API).

I'm also noticing that there are two identical listeners in the repo, both with the same issue:
https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/master/Microsoft.Toolkit.Uwp.UI.Controls.DataGrid/Utilities/WeakEventListener.cs
https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/master/Microsoft.Toolkit.Uwp/Helpers/WeakEventListener.cs

[michael-hawker]

Thanks @dotMorten for catching this. I believe the DataGrid team wanted to not pull in the whole sub-project for this one item, so they copied and marked internal.

Adding @RBrid and @harinikmsft, let's sync on this next week and figure out the best path forward for a fix? The 6.0 release would be the time to make a breaking change.

[michael-hawker]

@dotMorten we probably don't want to expose the weakInstance variable as a property though due to encapsulation, eh? As then we wouldn't need to change the calling pattern.

However, I'd rather break this now and fix it for 6.0 and call it out as a breaking change.

Thoughts?

[RBrid]

@dotMorten and @michael-hawker, I have not seen any evidence of a leak yet. I got suspicious when I saw this old DataGrid comment:

// A local variable must be used in the lambda expression or the CollectionView will leak
ICollectionView collectionView = this.CollectionView;

That seemed to indicate that whoever worked on that code checked for mem leaks.

I modified the existing Test_WeakEventListener_Events() unit test to include more information:

namespace UnitTests.Helpers
{
    [TestClass]
    public class Test_WeakEventListener
    {
        public class SampleClass
        {
            public event EventHandler<EventArgs> Raisevent;

            public SampleClass(string name)
            {
                System.Diagnostics.Debug.WriteLine("SampleClass created for " + name);
                Name = name;
            }

            ~SampleClass()
            {
                System.Diagnostics.Debug.WriteLine("SampleClass destroyed for " + Name);
            }

            public string Name { get; set; }

            public void DoSomething()
            {
                OnRaiseEvent();
            }

            protected virtual void OnRaiseEvent()
            {
                Raisevent?.Invoke(this, EventArgs.Empty);
            }
        }

        [TestCategory("Helpers")]
        [TestMethod]
        public void Test_WeakEventListener_Events()
        {
            for (int i = 0; i < 10; i++)
            {
                bool isOnEventTriggered = false;
                //bool isOnDetachTriggered = false;

                SampleClass sample = new SampleClass(i.ToString());

                WeakEventListener<SampleClass, object, EventArgs> weak = new WeakEventListener<SampleClass, object, EventArgs>(sample);
                weak.OnEventAction = (instance, source, eventArgs) =>
                {
                    isOnEventTriggered = true;
                    System.Diagnostics.Debug.WriteLine("OnEventAction for " + instance.Name);
                };

                weak.OnDetachAction = (listener) =>
                {
                    //isOnDetachTriggered = true;
                    System.Diagnostics.Debug.WriteLine("OnDetachAction for " + sample.Name);
                };

                sample.Raisevent += weak.OnEvent;

                sample.DoSomething();
                Assert.IsTrue(isOnEventTriggered);

                //weak.Detach();
                //Assert.IsTrue(isOnDetachTriggered);

                System.Diagnostics.Debug.WriteLine("Releasing SampleClass " + i.ToString());
                sample = null;

                System.Diagnostics.Debug.WriteLine("Garbage collection starting...");
                GC.Collect();
                GC.WaitForPendingFinalizers();
                GC.Collect();
                System.Diagnostics.Debug.WriteLine("Garbage collection done");
            }
        }
    }
}

When you run this modified test, you do see the "SampleClass destroyed for ,,," outputs, whether the commented out code is commented out or not. Am I missing something? How do you repro the leak with that test? Thanks.

[michael-hawker]

Thanks @RBrid for the info.

I think this is really to prevent an anti-pattern case implemented in the toolkit, like the RotatorTile example above (and all other cases I've seen across the toolkit including DataGrid). If we instead pass the instance in, the local copy reference doesn't have to exist and it's easier to show the proper technique un-registering off the weak reference vs. potentially storing a hard reference.

I think the problem is we don't have a good sample of how to use this pattern, as all the docs do is point to the Unit Test which doesn't actually register to any other method like all our patterns in the toolkit like in AdvancedCollectionView and in DataGridRows.

@RBrid I think we'd need to add a Unit Test to show the same pattern we use everywhere of:

    INotifyPropertyChanged inpc = rowGroupInfo.CollectionViewGroup as INotifyPropertyChanged;
    var weakPropertyChangedListener = new WeakEventListener<DataGrid, object, PropertyChangedEventArgs>(this) {
        OnEventAction = (instance, source, eventArgs) => instance.CollectionViewGroup_PropertyChanged(source, eventArgs),
        OnDetachAction = (weakEventListener) => inpc.PropertyChanged -= weakEventListener.OnEvent
    }
    inpc.PropertyChanged += weakPropertyChangedListener.OnEvent;

Then it'd be easier to test for leaks on the inpc type variable in the case above in cases where it's not just a local variable which may get optimized differently?

Ideally, I believe the suggested new pattern for using this should be:

_weakListener?.Detach(); // If we had a prior object, let's detach from our previous instance

// This is all encapsulated now to aid in preventing capturing variables outside of scope
_weakListener = new WeakEventListener<SomeType, object, SomeEventArgs>(this) {
    OnEventAction = (instance, source, eventArgs) => instance.SomeMethodForward(source, eventArgs),
    OnDetachAction = (instance, listener) => instance.SomeEvent -= listener.OnEvent
}

// Do final registration to weak event listener
originalSource.SomeEvent += _weakListener.OnEvent;

Right @dotMorten?

[dotMorten]

I don't really get how that test tests for leaks. The problem with it that both the listener and the class goes out of scope at the same time. Generally the "leak" is when a short-lived UI object is being held on to by a long-lived model's event.

[dotMorten]

Here's a more accurate test that reproduces the problem. Notice how the two event methods captures the object with a hard reference. This means SampleClass doesn't get collected until ModelObject also goes out of scope. This is a major problem as VMs and Models tend to live a lot longer than UI objects:

    public class ModelObject : INotifyPropertyChanged
    {
        public event PropertyChangedEventHandler PropertyChanged;
        private string _name;

        public string Name
        {
            get { return _name; }
            set { _name = value; PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Name))); }
        }

    }
    public class SampleClass
    {
        public SampleClass(ModelObject model, string name)
        {
            this.Name = name;
            System.Diagnostics.Debug.WriteLine("SampleClass created for " + Name);
            var weak = new WeakEventListener<ModelObject, object, PropertyChangedEventArgs>(model);
            weak.OnEventAction = (instance, source, eventArgs) =>
            {
                System.Diagnostics.Debug.WriteLine("OnEventAction for " + Name); //Creates capture -> Leak
                //System.Diagnostics.Debug.WriteLine("OnEventAction"); //Doesn't leak
            };

            weak.OnDetachAction = (listener) =>
            {
                System.Diagnostics.Debug.WriteLine("OnDetachAction for " + Name);  //Creates capture -> Leak
                //System.Diagnostics.Debug.WriteLine("OnDetachAction for "); //Doesn't leak
            };
            model.PropertyChanged += weak.OnEvent;
        }

        ~SampleClass()
        {
            System.Diagnostics.Debug.WriteLine("SampleClass destroyed+ " + Name);
        }

        public string Name { get; }
    }
    [TestClass]
    public class UnitTest1
    {
        [TestMethod]
        public void TestMethod1()
        {
            ModelObject model = new ModelObject();
            for (int i = 0; i < 10; i++)
            {
                SampleClass sample = new SampleClass(model, i.ToString());
                model.Name = i.ToString();

                System.Diagnostics.Debug.WriteLine("Releasing SampleClass " + i.ToString());
                sample = null;

                System.Diagnostics.Debug.WriteLine("Garbage collection starting...");
                GC.Collect();
                GC.WaitForPendingFinalizers();
                GC.Collect();
                System.Diagnostics.Debug.WriteLine("Garbage collection done");
            }
        }
    }

[dotMorten]

Poking around some more, there is a way to use it without leaking, and the instance is usually null during Detach() (since it's called when the reference is lost), so I think I've been brainfarting quite a bit here.

[michael-hawker]

@dotMorten @RBrid I think this was a great discussion to ensure we have this pattern correct. As I mentioned, I think the problem is we haven't had the best guidance on how to not accidentally capture a reference here. I would think the unregister event in Detach would be used for the cases Detach is manually called, eh? As otherwise as Morten called out the reference has already been collected.

I believe the canonical sample would be something like:

    var inpc = rowGroupInfo.CollectionViewGroup as INotifyPropertyChanged;
    var weakPropertyChangedListener = new WeakEventListener<DataGrid, object, PropertyChangedEventArgs>(this) {
        OnEventAction = (instance, source, eventArgs) => instance.CollectionViewGroup_PropertyChanged(source, eventArgs),
        OnDetachAction = (weakEventListener) => inpc.PropertyChanged -= weakEventListener.OnEvent // Use Local References Only
    }
    inpc.PropertyChanged += weakPropertyChangedListener.OnEvent;

And we just need to call out to use local references only within OnEvent/Detach?

If this looks good, I'll update our docs with this guidance and we can close this matter without any code changes. 🙂

[dotMorten]

And we just need to call out to use local references only within OnEvent/Detach?

But it does seem ok to have a hard referenced to the object who's event you're listening to, as that's expected to live longer. ie in your DetachAction above you are making a hard reference to inpc. It's just that you have to avoid making a hard reference to 'this'

[michael-hawker]

Thanks for the clarification @dotMorten, I'll update our doc and add you to the PR.

I think checking all our usages in the toolkit, we never create a this reference, outside of calling a method which wouldn't carry a reference anyway, eh?

[michael-hawker]

Added a doc issue to track over there, so will close this one again now that we have guidance discussed here to copy over. Thanks!