Is it really necessary for a UnityContaner to hold the list of the ChildContainers created by it?

[csm101]

Please correct me if I am wrong, but I am having a really hard time in finding a good real-world reason for which ChildContainers need to be Disposed() otherwise they surely create memory leaks.

See this simple NUnit test (that fails unless I uncomment that ChildContainer.Dispose()) line:

using NUnit.Framework;
using System;
using Unity;

namespace UnityTests
{
  public class ChildContainerMemoryLeakTest
  {

    IUnityContainer MainContainer = new UnityContainer();

    public class MyClass
    {
      public static int InstanceCounter { get; set; } = 0;

      public MyClass()
      {
        InstanceCounter++;
      }

      ~MyClass()
      {
        InstanceCounter--;
      }
    }

   // this method creates and "forgets" a ChildContainer: outside this code it is totally impossible
   // for the main program to get again hold of this child container, so I would expect it to be ready to
   // be garbage collected... but this doesn't happen as it stays in memory keeping 
   // alive any instance has been registered in it
    void CreateAndForget()
    {
      var childContainer = MainContainer.CreateChildContainer();
      var newInstance = new MyClass();
      childContainer.RegisterInstance(newInstance);
      Assert.AreEqual(1, MyClass.InstanceCounter);
      // childContainer.Dispose(); // Why  am I forced to do this if I want to avoid memory leaks? 
                                   // childContainer's lifetime, for my program ends here! there is no way to use it elsewhere again.
    }

  [Test]
    public void Test()
    {
      CreateAndForget();
      GC.Collect();
      GC.WaitForPendingFinalizers();
      Assert.AreEqual(0, MyClass.InstanceCounter); // this fails
    }
  }
}

Now, I know that explicitly calling Dispose() does the trick... But being forced to call Dispose() feels like an artificial and unnecessary requirement that defies the purpose of having a garbage collector: from the point of view of the programmer there is no way to access again the ChildContainer I created in that metod, so I would expect it to be garbage-collected.

Looking at the sources of UnityContainer I noticed that the whole problem is caused by the line I am adding a comment to here (taken from https://github.com/unitycontainer/container/blob/master/src/UnityContainer.Implementation.cs)

        private UnityContainer(UnityContainer parent)
        {
            // WithLifetime
            LifetimeContainer = new LifetimeContainer(this);

            // Context and policies
            _context = new ContainerContext(this);

            // Parent
            _parent = parent ?? throw new ArgumentNullException(nameof(parent));
            _parent.LifetimeContainer.Add(this); // << THIS IS THE ROOT CAUSE OF THE MEMORY LEAK

this line artificially extends the lifetime of the child container beyond the life of any variable referencing it in the main program... for no practical reason I can understand.

Without this _parent.LifetimeContainer.Add(this) line child containers would be automatically disposed by the garbage collector whenever the mail program is not able to reference them anymore, and this seems to me the more logic and appropriate thing to do.

If removing this " _parent.LifetimeContainer.Add(this);" line would be a breaking change, could you consider the idea of adding a "IUnityContainer CloneContainer()" call that does the same thing that does CreateChildContainer without executing such line?

Thank you for your time

[ENikS]

This was an original convention for the Unity in 2008. The container does not implement finalizer, so the only option to release it is by explicitly calling Dispose or disposing parent container.

I am considering implementing finalizers for Unity 6

[csm101]

Well... but... As long as the child container is referenced by the parent container _(because of "parent.LifetimeContainer.Add(this);") it won't be garbage collected and finalized, regardless of implementing the finalizer or not... Right?

[ENikS]

When disposed, child container removes itself from parent’s list.

[csm101]

That's exactly the issue i am talking about. I MUST call dispose for no reason.

[ENikS]

I am not sure what i can do for you at this point.

This is the original design that i have nothing to do with. For next version i am considering implementing finalizers. What exactly are you proposing?

[csm101]

Let me elaborate: dispose will never be automatically called by the garbage collector because of that parent.lifetimecontainer.add(this) parent/child link... They will stay in memory (for no reason) even when they are totally unreachable by the main program.

What's the actual benefit of keeping that parent-> child link? If it is really useful why isn't it implemented via a weak<> reference?

[ENikS]

This behavior is by design because lifetime of objects created by child container depends on that child. Collecting a child container will automatically dispose all container controlled objects, which might be unexpected by the consumers. Disposing is always done by the consumer when the whole scope is no longer required.

[csm101]

Ok, this leads to the modification I was proposing: adding a CloneContainer() method that creates a container initialized exactly like that child container but whithout adding it to the parent's lifetimeContainer, so that its lifetime (and the lifetime of all objects created by it) depends only by weather or not the child container instance is actually reachable by the main program.... So my test case will pass. Honestly I expexted this behavior by childcontainers, and I ended up in having my application infested by memory leaks because child containers insisted in staying in memory even when they were declared inside objects that were alreay garbage collected. I had to rewrite the whole architecture ditching child containers in favor of factory classes, due to this...

[ENikS]

Why don’t you use ‘using’ qualifier? Simple and clean solution to scopes

[csm101]

Well, for the same reason you need a garbage collector. The "using" syntax is meant to clean up things before the garbage collector does (being assured that the garbage collector will eventually do it anyway), not to force you to call Dispose because if you don't, the object will never be garbage-collected.

[ENikS]

If you use 'using' you don't need to call Dispose(). Compiler will add necessary code to handle it properly.

[ENikS]

After carefully considering your proposal I have to decline it. The solution to this problem could be easily created by adding 'using' to do proper scoping and would not require any changes to current implementation.

[csm101]

the problem can be "easily solved with the using construct" ONLY if the child container is assigned to a local variable whose scope is limited to the lifetime of a method call. this is very hardly a real-world scenario. You are confusing my test case which just illustrates the problem with a real world application.

In a real world scenario the ChildContainer is a member variable of a bigger class that might be referenced by multiple instances of other objects... even HUNDREDS of other objects and there could be even circular references among them, in complex data structures (think about the complex case, not about the toy two-liner program).

under these circumstances you can't tell the programmer "you are not calling Dispose() at the right time": to call Dispose() at the right time you must write your program like you don't have any garbage collector and manually keep track of reference counts and dependency graphs.

If this is the premise, ChildContainers are extremely dangerous and hard to be used correctly. it is much more safer to ditch child containers and use only Factory classes... and this only because of that parent -> child link.

[ENikS]

The decision to create this behavior was made in 2008 and was based on state of programming at that time. I am sure a lot has changed but fact is, everyone who uses Unity expects this behavior.

To add this feature will require considerable effort but would only solve very small, easily avoidable problem. There are much more pressing issues that require fixing. The economics of the project are not in favor of the change.

[csm101]

Still, adding a new optional boolean parameter "bool linkToParentContainer=true" to the current CreateChildContainer() wouldn't break existing applications, so my proposal is not a breaking change... Can we discuss about this proposal and stop pretending that calling "Dispose()" is an easy solution?

P.S: I have 20+ years experience in programming in languages that do not have a garbage collector, even on multi-million lines codebases, and I have also contributed to the porting of the ZXing barcode scanning library to the non-garbage-collected language Delphi, porting for which I am exactly the programmer responsible of solving all the memory leaks that popped up during the porting process because delphi does not have a garbage collector, so please stop lecturing me about the "using" keyword: I perfectly know how to write code that calls "x.Free" (delphi) or "delete x;"( C, C++) at the right moment, and I know it is not always easy (unless you are working on a toy program). (For the first time in my life I am in the opposite side of the "I don't need a garbage collector because I am able to handle memory pointers by myself" discussion... it feels really awkward.)

furthermore: I can't give to other programmers in my team a library (i am talking about the library I initially wrote using childcontainers) that creates objects containing a ChildContainer and just hope that all the programmers in the team will always remember to call .Dispose on my class otherwise I get a memory leak. a team leader must solve problems, not introduce new ones. it MIGHT feel easy to remember to call ".Dispose()" or to use the "using" pattern (under the totally fictional hypothesis that the lifetime of the objects I am talking about is the lifetime of a local variable during a function call), but when one of the 10 programmers in the team forgets to do it, it may take WEEKS (and thousands of dollars) to track down the line of code responsible of the memory leak. I can't give my green light to a solution that creates memory leaks so easily: I DID initially try to follow the .Dispose requirement and the resulting code resulted very hard to maintain because the lifetime of an object in some cases is dictated by external factors you are not in control of.

Just to give you a practical example: an instance of my class was needed to be held by the "Holder" instance of the "holder pattern" required for handling ListView items in a Xamarin.Android application: The Listview adapter class creates Holder instances for each visible item and each instance contains an instance of my class which in turn contains a ChildContainer object. Well: android ListView adapters NEVER tell you when the holder instance is not needed any more: they just wait for the garbage collector to run, so in my application, under this scenario, I have absolutely no way to know when a holder instance is not needed anymore and call the Dispose() method.

But, now... I think I have explained enough why the requirement of explicitly calling "Dispose" is a problem, and I have suggested a solution that wouldn't break existing codebases. Can we discuss about this solution? I can offer to implement it by myself and issuing a pull request for it.

Thank you again.

[ENikS]

There are no plans to add more features to Unity 5.x. Version 6 of Unity will have finalizers, weak references and will dispose on collection.

Meanwhile, while version 6 is in development, you can implement your idea and create custom build, for you and your team. The build certificate is available at GitHub so your build will be as good as the official one.