WeakEventManager<TEventSource, TEventArgs> and PropertyChangedEventManager causes memory leak

Question

I've an application where I'm not able to remove event handlers because I don't know when the last reference will be freed.

My application contains a PropertyChanged event source that is put into a container class that also implements INotifyPropertyChanged. This hierarchy contains more than 6 levels. Each instance of a level could be placed into multiple other instances. That's the reason why I couldn't determine when to free those instances.

The instances on the lowest level will live for the whole application runtime. This causes that all other instances will not be freed and I got a memory leak.

To avoid this event driven memory leak I tried to use WeakEventManager(TEventSource, TEventArgs). This class is only available in .Net 4.5 and because of compatibility to existing hardware I’ve to use .Net 4.0.

In .Net 4.0 is a PropertyChangedEventManager available that should do the same for INotifyPropertyChanged.

My classes are freed correctly.

But there is still a memory leak.

I simplified my application to the following code that produces a memory leak:

// This code will force the memory leak
while (true)
{
    var eventSource = new StateChangedEventSource();
    var eventReceiver = new StateChangedEventReceiver();

    PropertyChangedEventManager.AddListener(eventSource, eventReceiver, string.Empty);
}

public class EventSource : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler PropertyChanged;
}

public class  EventReceiver : IWeakEventListener
{
    public bool ReceiveWeakEvent(Type managerType, object sender, EventArgs e)
    {
        return true;
    }
}

Yes I know there is no RemoveListener call. I couldn’t determine when an instance is never used and could be freed. If I knew that I could use normal event registration and deregistration. In that case I don’t have to use the PropertyChangedEventManager.

What is the problem of my sample code? Why does it produce a memory leak?

Edit 2014/02/17:

I tried the WeakEventManager(TEventSource, TEventArgs) and .Net 4.5 and the problem still exists.

var eventSource = new EventSource();

var i = 0;
while (true)
{
    var eventReceiver = new EventReceiver();

    // --> Use only one of the following three lines. Each of them will produce a memory leak.
    WeakEventManager<EventSource, PropertyChangedEventArgs>.AddHandler(eventSource, "PropertyChanged", eventReceiver.OnEvent);
    PropertyChangedEventManager.AddListener(eventSource, eventReceiver, string.Empty);
    WeakEventManager<EventSource, EventArgs>.AddHandler(eventSource, "SomeOtherEvent", eventReceiver.OnSomeOtherEvent);
    // <--

    ++i;
    if (i == 1 << 18)
    {
        Thread.Sleep(10);
        GC.Collect(2);
        Thread.Sleep(10);
        i = 0;
    }
}


public class EventSource : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler PropertyChanged;

    public event EventHandler<EventArgs> SomeOtherEvent;
}

public class EventReceiver : IWeakEventListener
{
    public void OnSomeOtherEvent(object sender, EventArgs args)
    {
    }

    public void OnEvent(object sender, PropertyChangedEventArgs args)
    {
    }

    public bool ReceiveWeakEvent(Type managerType, object sender, EventArgs e)
    {
        return true;
    }
}

This code compiled using .Net 4.5 do also run out of memory. I got the hint using the Thread.Sleep construct here.

Resnick

First find out what/where that leak really is. How sure are you that it's the events? – Thormora

Do you mean my original memory leak or the memory leak described above? I'm sure that my objects are never freed and I've a memory leak with the events. I used JetBrains Memory Profiler to detect my memory leak. I could reduce the memory consumption a lot by using the PropertyChangedEventManager but there's still a memory leak left that could be simply reproduced using the sample code of this question. – Resnick

I posted this issue in msdn forum too because I still got no answer in this forum. If you're interested in this issue you may have a look here. – Resnick

Answer 1

Based on the information at msdn and codeproject I realized that the WeakEventManager(TEventSource, TEventArgs) class will only work in WPF applications. I'm using WinForms what's the reason why it seems not to work.

I decided to create my own WeakEventManager that works without using the build in WeakEventManager provided by the .Net framework.

The implementation of my WeakEventManager uses a background thread to clean up all instances. Maybe there's a better solution but this solution will work correctly in my application.

public static class ThreadedWeakEventManager
{
    private static readonly TimeSpan CleanupInterval = TimeSpan.FromSeconds(1.0);

    private static readonly List<IInternalWeakEventManager> EventManagers = new List<IInternalWeakEventManager>();

    private static volatile bool _performCleanup = true;

    static ThreadedWeakEventManager()
    {
        new Thread(Cleanup) { IsBackground = true, Priority = ThreadPriority.Lowest }.Start();
    }

    public static void AddHandler<TEventArgs>(object eventSource, string eventName, EventHandler<TEventArgs> eventHandler)
        where TEventArgs : EventArgs
    {
        var weakEventManager = new InternalWeakEventManager<TEventArgs>(eventSource, eventName, eventHandler);

        lock (EventManagers)
        {
            EventManagers.Add(weakEventManager);
        }
    }

    public static void AddPropertyChangedHandler(INotifyPropertyChanged eventSource, EventHandler<PropertyChangedEventArgs> eventHandler)
    {
        AddHandler(eventSource, "PropertyChanged", eventHandler);
    }

    public static void AddCollectionChangedEventHandler(INotifyCollectionChanged eventSource, EventHandler<NotifyCollectionChangedEventArgs> eventHandler)
    {
        AddHandler(eventSource, "CollectionChanged", eventHandler);
    }

    public static void RemoveHandler<TEventArgs>(object eventSource, string eventName, EventHandler<TEventArgs> eventHandler)
        where TEventArgs : EventArgs
    {
        if (eventSource == null || string.IsNullOrWhiteSpace(eventName) || eventHandler == null)
        {
            return;
        }

        lock (EventManagers)
        {
            EventManagers.RemoveAll(item => object.ReferenceEquals(item.EventData.EventSource, eventSource) && item.EventName.Equals(eventName) && eventHandler.Method.Equals(item.EventData.EventHandlerMethodInfo));
        }
    }

    public static void RemovePropertyChangedHandler(INotifyPropertyChanged eventSource, EventHandler<PropertyChangedEventArgs> eventHandler)
    {
        RemoveHandler(eventSource, "PropertyChanged", eventHandler);
    }

    public static void RemoveCollectionChangedEventHandler(INotifyCollectionChanged eventSource, EventHandler<NotifyCollectionChangedEventArgs> eventHandler)
    {
        RemoveHandler(eventSource, "CollectionChanged", eventHandler);
    }

    public static void CancelCleanup()
    {
        _performCleanup = false;
    }

    private static void Cleanup()
    {
        while (_performCleanup)
        {
            Thread.Sleep(CleanupInterval);

            lock (EventManagers)
            {
                for (var i = EventManagers.Count - 1; i >= 0; --i)
                {
                    var item = EventManagers[i];
                    if (item.EventData.IsGarbageCollected)
                    {
                        item.UnwireEvent();
                        EventManagers.RemoveAt(i);
                    }
                }
            }
        }
    }

    private interface IInternalWeakEventManager
    {
        string EventName { get; }
        IWeakEventData EventData { get; }>
        void UnwireEvent();
        void OnEvent(object sender, EventArgs args);
    }

    private class InternalWeakEventManager<TEventArgs> : IInternalWeakEventManager
        where TEventArgs : EventArgs
    {
        private static readonly MethodInfo OnEventMethodInfo = typeof(InternalWeakEventManager<TEventArgs>).GetMethod("OnEvent");
        private EventInfo _eventInfo;
        private Delegate _onEvent;

        public InternalWeakEventManager(object eventSource, string eventName, EventHandler<TEventArgs> eventHandler)
        {
            this.EventData = new WeakEventData<TEventArgs>(eventSource, eventHandler);

            this.WireEvent(eventSource, eventName);
        }

        public string EventName
        {
            get { return this._eventInfo.Name; }
        }

        public IWeakEventData EventData { get; private set; }

        public void UnwireEvent()
        {
            var eventSource = this.EventData.EventSource;
            if (eventSource == null)
            {
                return;
            }

            this._eventInfo.RemoveEventHandler(eventSource, this._onEvent);
        }

        public void OnEvent(object sender, EventArgs args)
        {
            this.EventData.ForwardEvent(sender, args);
        }

        private void WireEvent(object eventSource, string eventName)
        {
            this._eventInfo = eventSource.GetType().GetEvents().FirstOrDefault(item => item.Name == eventName);
            if (this._eventInfo == null)
            {
                throw new InvalidOperationException(string.Format("The event source type {0} doesn't contain an event named {1}.", eventSource.GetType().FullName, eventName));
            }

            this._onEvent = Delegate.CreateDelegate(this._eventInfo.EventHandlerType, this, OnEventMethodInfo);
            this._eventInfo.AddEventHandler(eventSource, this._onEvent);
        }
    }

    private interface IWeakEventData
    {
        bool IsGarbageCollected { get; }
        object EventSource { get; }>
        MethodInfo EventHandlerMethodInfo { get; }
        void ForwardEvent(object sender, EventArgs args);
    }

    private class WeakEventData<TEventArgs> : IWeakEventData
        where TEventArgs : EventArgs
    {
        private readonly WeakReference _eventSource;
        private readonly WeakReference _eventTargetInstance;

        public WeakEventData(object eventSource, EventHandler<TEventArgs> eventHandler)
        {
            this._eventSource = new WeakReference(eventSource);
            this._eventTargetInstance = new WeakReference(eventHandler.Target);
            this.EventHandlerMethodInfo = eventHandler.Method;
        }

        public object EventSource
        {
            get { return this._eventSource.Target; }
        }

        public MethodInfo EventHandlerMethodInfo { get; private set; }

        public bool IsGarbageCollected
        {
            get
            {
                return !this._eventSource.IsAlive || !this._eventTargetInstance.IsAlive;
            }
        }

        public void ForwardEvent(object sender, EventArgs args)
        {
            var target = this._eventTargetInstance.Target;
            if (target != null)
            {
                this.EventHandlerMethodInfo.Invoke(target, new[] { sender, args });
            }
        }
    }
}

Resnick

I like your solution. Anyway, any better solution found @ 2018? – Beholden

@Beholden No Sorry. But I have to confess that I didn't spend more effort to that topic. – Resnick

Answer 2

I don't think the isue with WeakEventManager<,> is specific to non-WPF, as i can reproduce a memory leak in a WPF application as well.

The problem lies with the management of the event table. For each subscription, the WeakEventManager creates an entry in a table. This entry and table are (by necessity) strong-referenced.

The problem is, that by default, the WeakEventManager does not clean up the records. You have to call RemoveHandler. But beware. It's not thread safe. If you call it from another thread it may fail (doesn't throw an exception, you'll just experience that there's still a memory leak). When called from a finalizer, it doesn't work reliably either.

I also investigated into the source code and found that while it contains logic to do cleanup on AddHandler and when an event is received, it is disabled by default (see WeakEventManager.cs => WeakEventTable.CurrentWeakEventTable.IsCleanupEnabled). Also, you can't access the Cleanup method since the methods and properties necessary to do so are private or internal. So you can't even create subclass to access these methods / modify the behavior.

WeakEventManager<,> is broken

So basically (as far as i can understand) WeakEventManager<,> is broken by design (it keeps a StrongReference to the subscriber table-entry). Instead of fixing a MemoryLeak it will only reduce the MemoryLeak (the event source and listener can be garbage collected, but the entry for the event subscription is not => new memory leak). Of course the memory leak introduced by WeakEventManager<,> is small.

Brainwash

-1 Seems you don't understand how the WeakEventManager works then. The subscriber NEVER is subscribed to the event. Instead, a static class subscribes to the event (which has a fixed memory footprint). The static class then has a collection of WeakReferences to the subscribers, and acts as a dispatcher for the actual subscribers. – Armagnac

@Armagnac i changed the wording of my answer a little bit to make it clearer what i wanted to say. The implementation of WeakEventManager<,> is broken because it keeps a strong reference to a table entry. The table grows and is not reduced unless one calls RemoveHandler. As i write the event source and listener are correctly garbage collected and not part of the memory leak. – Brainwash

So you mean that although the target object IS Weakly referenced. There is a strong reference to the Weak Reference object, that is leaked. That, is, retarded. Luckily that is a very very small amount of memory. Worse, it could have been avoided with a ConditionalWeakTable. – Armagnac

@aron that's exactly what's happening. It is a small amount of memory. The "retarded" thing is that MS made the Cleanup related methods / config private / internal, so there's code to fix it, but you don't have a chance to use it! – Brainwash

Good thing we can fix it in .net core. Hazza for OpenSource. – Armagnac

@Armagnac WeakEventManager<,> does not exist in .net core (yet). As far as i can see it also wasn't published under MIT license (parts of the .net source have been), so there's currently no easy way to bring it to .net core. – Brainwash

Answer 3

Memory leak in WeakEventManager can occur if AddListener is called from different threads.

Just call AddListener from the main UI thread and inner "CleanUp" will work fine.

Circinus

Maybe this will work but in that case it is unusable. I posted this issue in MSDN and the result should be that it should work in WPF-Applications. I didn't tried it since this post - maybe it works now (having a UI-Thread). We don't have a UI-Thread in each application and therefore we need another solution: my own implementation works as expected for years. – Resnick