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

[SergeiPavlov]

Following leaks are fixed:

[alex-kulakov]

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]

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]

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]

Members should be arranged by access level from most accessible to least accessible, from public to private. Constructors should follow the same rule, move private constructor to the place after all public constructors.

Also, let's make no exceptions and use comma at the end of the sentence in summary.

[alex-kulakov]

I see another violation of this rule above yours constructor. Since you shed light on this code region, it should also be fixed, I'll fix it after merge.

[SergeiPavlov]

Also, let's make no exceptions and use comma at the end of the sentence in summary.

dot, not comma?

[SergeiPavlov]

Members should be arranged by access level from most accessible to least accessible, from public to private.

But public properties follow private fields.
What is the order of initialization.
I think it is better to init in the order of declaration

[alex-kulakov]

But public properties follow private fields.
What is the order of initialization.
I think it is better to init in the order of declaration.

First, we group members by type - fields, properties, static methods, instance methods, constructors, finalizers. General rule within each group is to have members in order from most accessible to least accessible as I said.

In this case private constructor is less usable and has basically one job and probably will be the least readable ctor among all, so it should be at the end of constructors list.

This is preferable structure, but, of course, there are some exceptions, in particular for methods and constructors, in favor of reading we do allow members to be places against general rule, for example:

• methods that implement interface are likely to be in one group, sometimes they are in #regions;
• methods with the same name may also be grouped no matter what access modifier they have, but it is better to apply general rule within the methods, for instance public void Add(), then internal void Add() and then private void Add()/private void AddInternal().
• various helper methods, they mix and match instance and static methods but most of the time they are private and small; with proper name they don't require going into them to understand an algorithm which uses them. Since they are less important in general, they tend to be moved to the end of methods region.
• serialization/deserialization members (methods and ctors,); they are read extremely rarely so they are preferred to be after ctors.
• GetEnumerator methods are also very simple and not very important to be read, so they are preferred to be moved closer to the end of methods region.

These are exceptions I remembered but probably there are some others. If I saw something I just point to it, when I have time to properly explain why I try to, like now.

As you may already understood, the main idea of such structure is to bring up the members of their group (or whole class) which are more important when reading or scanning through the class and to push down the bottom of the class the members which are less likely to be read. We chose this structure and I try to maintain it because I see its effectiveness. After year of being junior in the project I became the only guy who supports it and had to read unseen code a lot and this structure, in my humble opinion, helped me to not waste some time skipping unimportant members like constructors, helpers, serialization members, etc.

[SergeiPavlov]

On the other side, when you participate in 10+ different projects, and every project has its own rules, it is hardly possible to remember compex rulesets, anything behind "fields, properties, constructors" order.

So I'm always trying to guess the order by example and forget them on next day

[alex-kulakov]

I understand, It's ok that you can't remember it. What more important is that you apply changes that requested.

[SergeiPavlov]

Best way to enforce code style rules - to develop a Roslyn Analyzer.

[alex-kulakov]

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

[SergeiPavlov]

resolved