DataObjects-NET / dataobjects-net

https://dataobjects.net
MIT License
60 stars 23 forks source link

Fix ColumnInfoCollection & NodeCollection<AssociationInfo> leaks; refactor IndexInfo class #297

Closed SergeiPavlov closed 10 months ago

SergeiPavlov commented 1 year ago

Following leaks are fixed: image

alex-kulakov commented 11 months ago

Inspired by this, I started investigation and I was wondering how many subscriptions happen and how many of them finally got unsubscribed. I injected calculations of subscribed and unsubscribed items like so

    protected void TrySubscribe(TNode item)
    {
      if (item is IChangeNotifier notifier) {
        Interlocked.Increment(ref NodeCollectionTracker.SubscribedItemsCounts);
        notifier.Changing += itemChangingHandler;
        notifier.Changed += itemChangedHandler;
      }
    }

    protected void TryUnsubscribe(TNode item)
    {
      if (item is IChangeNotifier notifier) {
        Interlocked.Increment(ref NodeCollectionTracker.UnsubscribedItemsCounts);
        notifier.Changing -= itemChangingHandler;
        notifier.Changed -= itemChangedHandler;
      }
    }

and check the values after domain is built and disposed like so

    [Test]
    public void MainTest()
    {
      Orm.Model.NodeCollectionTracker.Reset(); // sets values to 0
      BuildAndDisposeDomain(); //builds big domain with 200 entities

      TestHelper.CollectGarbage(true);

      var subscribedItemsCounts = Orm.Model.NodeCollectionTracker.SubscribedItemsCounts;
      var unsubscribedItemsCounts = Orm.Model.NodeCollectionTracker.UnsubscribedItemsCounts;
      Console.WriteLine($"Subscribed items count {subscribedItemsCounts}");
      Console.WriteLine($"Unsubscribed items count {unsubscribedItemsCounts}");
    }

The output for Debug configuration and without your IndexInfo.Dispose() was

Subscribed items count 77790
Unsubscribed items count 12029

then with dispose

Subscribed items count 77790
Unsubscribed items count 12029

Then I added a finalizer to NodeCollection looking like so

    ~NodeCollection()
    {
      foreach (var item in this)
        TryUnsubscribe(item);
    }

and the values became equalized even without IndexInfo.Dispose()

Subscribed items count 77790
Unsubscribed items count 77790

This got me thinking, shouldn't we create finalizer which will work for all NodeCollection<T> instances?

SergeiPavlov commented 11 months ago

This got me thinking, shouldn't we create finalizer which will work for all NodeCollection<T> instances?

Objects with finalizer have significant overhead. And this will not help anyway to avoid event handler leak problem. Finalizer just will not be invoked because there are links to leaked objects until Domain itself is alive.

alex-kulakov commented 11 months ago

Ok, then. I just think of not having this problem in other classes which use NodeCollection, some of them may have such problems as well, now or in future.

alex-kulakov commented 11 months ago

I approved your changes. There are some conflicts that prevent me from merging right now, though.

SergeiPavlov commented 11 months ago

resolved