[EventAggregator] Memory problem with EventAggregator and never published message

[softwaretirol]

Description

We haved an EventAggregator in production and see a huge memory amount being used over a longer runtime (> 1 Day). We have looked into that issue with a profile and see many, many DelegateReference (coming from PubSubEvent) laying arround and eating up ~200MB of data.

We looked up the sourcecode and as we can say, the method "PruneAndReturnStrategies" is responsible of clearing up the subscription list of the EventBase. This method is going to be called only if someone is publishing a message of type T.

Because we have the case that it is very rare that a message will be published, but several subscriber are subscribing to the event, we have lots of dead delegates within this subscription list.

Steps to Reproduce

public partial class MainWindow : Window
{

    EventAggregator eventAggregator = new EventAggregator();
    public MainWindow()
    {
        InitializeComponent();

        Task.Run(() =>
        {
            while (true)
            {
                new SampleConsumer(eventAggregator);
                Publish();
                //GC.Collect(3, GCCollectionMode.Forced, true, true);
                //GC.WaitForPendingFinalizers();
            }
        });
    }

    private void Publish()
    {
        eventAggregator.GetEvent<PubSubEvent<string>>().Publish("Hallo");
    }

    public class SampleConsumer
    {
        public SampleConsumer(EventAggregator eventAggregator)
        {
            eventAggregator.GetEvent<PubSubEvent<string>>().Subscribe(Received);
        }

        private void Received(string obj)
        {
        }
    }
}

Expected Behavior

• EventAggregator should check even on Subscribe or otherwise on a time based approach to clean up the subscriptions

Actual Behavior

• Subscription List is growing indefinitely

Basic Information

• Version with issue: master
• Last known good version: n/a

[brianlagunas]

If the list of subscribers isn't being reduced, it's because the subscriber parent is still in memory and not collected. We have tests that validate that subscribers that are collected and automatically unsubscribed. This type of behavior normally points to an issue with the application code and object remaining in memory. Unless you can provide a memory analyzer that shows the EA being the object that roots the objects.

[softwaretirol]

Thanks for your answer, but i do not have any additional code as I have shown above.
The instances are removed from memory correctly, what remains in memory is the management object within the PubSubEvent itself.

To prove it is a bug, just execute the following sample code:
https://gist.github.com/softwaretirol/65dd594ecd0d8e845ca33839a7aecfdc

The given output is constantly:
Instances: 1
Subscriptionlist: 420000
Instances: 1
Subscriptionlist: 430000
Instances: 1
Subscriptionlist: 440000
Instances: 1
Subscriptionlist: 450000

SubscriptionList is the amount of entries within the EventBase:

protected ICollection<IEventSubscription> Subscriptions
{
  get
  {
    return (ICollection<IEventSubscription>) this._subscriptions;
  }
}

So the created instances are all deleted, but the internal subscriptionlist, is growing.

[brianlagunas]

Have you tried having a proper class for the event public class MyEvent : PubSubEvent<string> and not using a generic in GetEvent<PubSubEvent<string>>?

[softwaretirol]

Adapted the sample code: https://gist.github.com/softwaretirol/9ba839671a4ceeb1c81fe0e7b1907dc8

Completly same behavior, but i was expecting that it wont change, because internally a Dictionary<Type, EventBase> is hold, and it is no difference in my view of using a derived type here.

[brianlagunas]

I'll try to look at this when I find time. however, our unit tests are showing the EA working properly, and you have not provided a running application with memory snapshots to support your issue. This means I will have to recreate everything and create my own snapshots which will take more time.

In the mean time, Prism is open source, so if you have time it would be great if you could look through the source and see what you find. Maybe even submit a fix if you find one :)

[TimBo93]

The EventBase class, which is inherited by PubSubEvent has a

private readonly List<IEventSubscription> _subscriptions = new List<IEventSubscription>();

This list contains EventSubscription instances, when using the PubSubEvent implementation of EventBase.

Instances of EventSubscription do not hold references to the subscriber.
That is enforced by the WeakReference in DelegateReference.

But the list that is containing the EventSubscription gets only pruned when calling
PruneAndReturnStrategies() which is called only when InternalPublish of EventBase gets invoked.

So @brianlagunas you are right, that the subscribers are not hold, but the EventSubscription do like @softwaretirol said. Maybe that can help you.

[FamularoA]

Hello @brianlagunas,

I have create a sample WPF app which demonstrate the unexpected behaviour.

Steps:
• Click Start, Weak Classes with subscription will be created
• Click Publish, you will see that the amount of subscription will be change (PruneAndReturnStrategies is called)

Without publishing the memory increase, you need only your Task Manager to verify my observation.

Regard,
Alexander

PrismMemoryLeak

[brianlagunas]

Thanks @FamularoA. In my experience the Task Manager is not a good indicator of memory leaks. I will use a proper memory analyzer.

[brianlagunas]

A big thank you to @FamularoA for the sample. I now fully understand the issue. I guess when this was first written, no one considered that there would be a scenario that would have so many constant registrations without ever publishing a message. Like @softwaretirol said, it's "very rare". I see a PR has already been submitted for this issue. Great job @softwaretirol. You're contribution is greatly appreciated. Good find!

[adamhewitt627]

While I agree with something like this needing to happen, I just updated from 7.0 and see a significant performance degradation in subscribing. I have:

1. A lot of objects registering for the same message in a tight loop
2. Prune is now called for each registration
3. Profiling suggests the primary slowdown is the GC, which makes some sense looking at this code.

[brianlagunas]

Can you measure the difference and provide the numbers? We may need to think of another approach. Maybe a method that must be manually called in the rare scenario where you have constant subscriptions and no messages being publshed.

[adamhewitt627]

Sure thing. I can work on a better benchmark test (i.e. BenchmarkDotNet) but since that runs a long time, here is a quick summary:

var ea = new EventAggregator();
var @event = ea.GetEvent<PubSubEvent>();
var watch = Stopwatch.StartNew();
for (int i = 0; i < 10000; i++)
    @event.Subscribe(OnMessage);

var elapsed = watch.Elapsed;

Count	7.0.0.396	7.1.0.431
1,000	00:00:00.0102406	00:00:01.5510316
10,000	00:00:00.1025806	00:02:20.7220970

EDIT: I discovered this issue in a UWP app, but those numbers are a .NETCore 2.1 Console App. Runtime on the long one also doesn't show a lot of GC activity, the GC was showing up in the UWP profiler.

[brianlagunas]

WOW! That is significant! This needs to be reverted

[brianlagunas]

@softwaretirol we need an alternate approach as the current solution has introduced a massive performance issue.

[softwaretirol]

Hey folks,

that sounds very bad, just see #1646 for a fix.

@brianlagunas what is your opinion to this solution?

I really do not like any time based solution, but in this case it might be an easy solution to catch both situations? The internal behavior changed a little, it will be checked at which time the last prune was run, and if it is 1 minute ago, it will do the prune.

[TimBo93]

The reason for the tremendous loss of performance lies in the complexity of O(n²) instead of O(n) by subscribing to an event in a loop. This is because each time adding a subscriber, the whole list will be inspected for dead subscribers.
Maybe a GC-like generation approach can help us, so that not all the entries are inspected each time and more likely alive subscribers are passed.
I also like @softwaretirol approach but that might not be stable in some situations where many items are inserted in short time (less 1 Minute).

[brianlagunas]

I really don't like the timed approach. Honestly, having massive registrations without a publish is more of an edge-case. So I would prefer to provide a way to handle this that would have to be opt-ed into. Maybe a public method that allows you to Flush the registrations manually.

[softwaretirol]

I really don't like the timed approach.

#MeToo

Pushed a better solution, for performance reasons I replaced the internal List to a LinkedList. At every publish/subscribe it will getting cleaned up if necessary (which is simply an unlink). For not creating a delegate every time the "GetExecutionStrategy" the IsAlive Property of the WeakReference is passed through.

Weird the publish itself is getting faster as before because the prune is faster with that solution.

[adamhewitt627]

That looks like an improvement to the performance, but still loops through a (potentially) large array on inserts. What about something like:

abstract class EventBase
{
    //Subscribe checks this and calls Prune()
    protected virtual AutoPrune { get; } = false;
}

//User's event class that they know doesn't have many Publishes
class Rare : PubSubEvent
{
    protected override AutoPrune => true;
}

[softwaretirol]

For publishing the iteration is needed at all, so it effects only the subscribing performance.
I have just coded a rough, small test against it.

var eventAggregator = new EventAggregator();
var watch = Stopwatch.StartNew();
for (int i = 0; i < 100000; i++)
{
   eventAggregator.GetEvent<PubSubEvent>().Subscribe(DoSomething);
}
for (int i = 0; i < 100000; i++)
{
   eventAggregator.GetEvent<PubSubEvent>().Publish();
}
Console.WriteLine(watch.Elapsed);
Console.ReadLine();

Needs 00:00:00.3489100 on my machine. Looks quite fine if you ask me.

[brianlagunas]

I'm actually leaning towards @adamhewitt627 suggestion. Very simple, and opt-in for specific events that have a large number of subscriptions.

[adamhewitt627]

It could probably use a better name, since it would still prune automatically on Publish. I also got to wondering if the new runtime events API could be of value here. (given multi-targeting, that is)

Correct me if I'm wrong, but #1646 is only faster because it's using WeakReference.IsAlive rather than building up execution strategies during each loop.

I'm picturing 3 eventual changes:

1. A bool as described to return to 7.0 behavior, while allowing for prune-on-subscribe.
2. Some form of a Flush API that a consumer can call on demand. (such as in response to low memory events, app lifecycle, etc)
3. Use runtime events API to largely obsolete number 2, but would only benefit .NETCore2.2+.

[softwaretirol]

Correct me if I'm wrong, but #1646 is only faster because it's using WeakReference.IsAlive rather than building up execution strategies during each loop.

Yes that is crucial! In my opinion i would stick to PR #1646 , it is fast and is covering both situations very well now. But the decision is not mine 😄 .

@adamhewitt627 What i would improve with your PR #1647 :

• The IsAlive thing should be done also in your case because it boosts everything
• I really do not like the async void thing - fire&forget seems not legit for me at this point

[rafsanulhasan]

Hi guys, as @softwaretirol explained about the fire & forget issue, you can take a look at https://github.com/brminnick/AsyncAwaitBestPractices. It might solve the issue.

[brianlagunas]

I have reverted the changes and have added a Prune method that can manually be called when having multiple registrations within short periods of time without a publish. The developer will be responsible for calling Prune in those rare scenarios to keep their application memory footprint down.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.