DiagnosticsCollector provokes memory leaks

[Maesla]

I was having a strange behavior with some instances not destroyed and some memory leaks. Then I realized that this behavior was only occurring when VContainerSettings.DiagnosticsEnabled = true.

I have done a Test trying to isolate and simplify my case and indeed, aparrently the Diagnostics is creating some memory leak, not allowing the gc to destroy some instances

using NUnit.Framework;
using System;
using System.Collections;
using UnityEngine;
using UnityEngine.TestTools;
using VContainer;
using VContainer.Diagnostics;

public class MemoryLeakTests
{
    private class DestroyCount
    {
        public int Count { get; private set; }

        public DestroyCount()
        {
            Count = 0;
        }

        public void IncreaseCount(string msg)
        {
            UnityEngine.Debug.Log(msg);
            Count++;
        }
    }

    private interface IFoo{}

    private class  FooA : IFoo
    {
        private readonly DestroyCount destroyCount;

        public FooA(DestroyCount destroyCount)
        {
            this.destroyCount = destroyCount;
        }

        ~FooA()
        {
            destroyCount.IncreaseCount("FooA destroyed");
        }
    }

    private class FooB : IFoo
    {
        private readonly DestroyCount destroyCount;

        public FooB(DestroyCount destroyCount)
        {
            this.destroyCount = destroyCount;
        }

        ~FooB()
        {
            destroyCount.IncreaseCount("FooB destroyed");
        }
    }

    private class FooFactory
    {
        public IFoo foo { get; set; }
    }

    static bool[] diagnosticsValues = new bool[] { true, false};

    [UnityTest]
    public IEnumerator MemoryLeakWithDiagnosisTest([ValueSource(nameof(diagnosticsValues))] bool useDiagnostics)
    {
        DestroyCount destroyCount = new DestroyCount();

        var builder = new ContainerBuilder();
        DiagnosticsCollector diagnosticsCollector = useDiagnostics ? DiagnositcsContext.GetCollector("test") : null;
        builder.Diagnostics = diagnosticsCollector;

        builder.Register<FooFactory>(Lifetime.Singleton);
        builder.Register<IFoo>(container => container.Resolve<FooFactory>().foo, Lifetime.Transient);

        var resolver = builder.Build();

        {
            FooFactory fooFactory = resolver.Resolve<FooFactory>();
            fooFactory.foo = new FooA(destroyCount);
        }
        {
            IFoo fooA = resolver.Resolve<IFoo>();
            Assert.That(fooA, Is.InstanceOf<FooA>());
        }
        {
            FooFactory fooFactory = resolver.Resolve<FooFactory>();
            fooFactory.foo = new FooB(destroyCount);
        }
        { 
            IFoo fooB = resolver.Resolve<IFoo>();
            Assert.That(fooB, Is.InstanceOf<FooB>());
        }

        //uncomment this line to pass both tests
        //diagnosticsCollector?.Clear(); 

        GC.Collect();

        yield return new WaitForSeconds(20f);
        Assert.That(destroyCount.Count, Is.EqualTo(1));
    }

    [UnityTest]
    public IEnumerator MemoryLeakGroundTruth()
    {
        DestroyCount destroyCount = new DestroyCount();

        FooFactory  fooFactory = new FooFactory();

        fooFactory.foo = CreateFooA(destroyCount);
        fooFactory.foo = CreateFooB(destroyCount);

        GC.Collect();

        yield return new WaitForSeconds(10f);
        Assert.That(destroyCount.Count, Is.EqualTo(1));
    }

    private IFoo CreateFooA(DestroyCount destroyCount)
    {
        return new FooA(destroyCount);
    }

    private IFoo CreateFooB(DestroyCount destroyCount)
    {
        return new FooB(destroyCount);
    }
}

[hadashiA]

In above test case, you have not Dispose the resolver ( var resolver = builder.Build();), so all instances may be retained, not just DiagnosticsCollector.

If you call GC.Collect(), is there any guarantee that all finalisers will be executed in the line immediately following it?

The only way to be sure is to Dispose. If LifetimeScope is used, it will be DIsposed if you Destroy. Are there any other problematic cases, other than this test case, on which Dispose is executed?

[Maesla]

It is not that DiagnosticsCollector, it's that the DiagnosticsCollector is preventing the gc to collect instances that are not referenced anymore, but probably the diagnostics is keeping some reference to them.

Maybe it wasn't clear in the test. In MemoryLeakWithDiagnosisTest, new FooA is created outside VContainer (fooFactory.foo = new FooA(destroyCount);),

but it is resolved using a factory .... builder.Register(container => container.Resolve().foo, Lifetime.Transient); ...... IFoo fooA = resolver.Resolve();

Then, this object is unreferenced everywhere, because other new object has been created fooFactory.foo = new FooB(destroyCount);

So, if it is unreferenced everywhere, if you call the gc, this object should be collected and cleaned at some point. You can see exactly that behaviour in the test MemoryLeakGroundTruth, that have this logic but not using VContainer.

My point is that the gc is not able to clean this unreferenced object because the diagnosticsCollector is keeping track of it, although is not been using anymore.

And you can see that this is true because the same test, with diagnosticsCollector = null, the object is correctly collected. You can check also that clearing the diagnosticsCollector diagnosticsCollector?.Clear() allows the gc to correctly collect all the unreferenced objects

This was only a toy example to show this issue, but I had it in a real logic, where a type was resolved using a reference in a factory. It had a problem because I was overwritting the object that need to be used in the resolve, and the old instances weren't never collected by the GC. But this happened only with the Diagnistic On. If I wasn't using the diagnostic, the gc collector worked as expected.

[hadashiA]

Can't you Dispose?

It seems difficult to assume that everything will be GC'd if Dispose is not called, not just DiagnosticsCollector. ( For example, EntryPoints such as IStartable are grabbed by PlayerLoopRunner.

[darkon76]

Because The DiagnosticsCollector keeps a reference of anything that has been resolved, it is problematic with long lived LifetimeScopes that have objects registered as Transient.

When you can dispose the Transient object the DiagnosticsCollector's reference count will never decrease.

The diagnosticCollector windows will tell the user that there are more references that in reality there are, confusing the user.

The expected behavior would be if the user dispose a Transient object the DiagnosticsCollector's reference count decrease.

[hadashiA]

Because The DiagnosticsCollector keeps a reference of anything that has been resolved, it is problematic with long lived LifetimeScopes that have objects registered as Transient.

Dispose should clear the reference, but why not Dispose?

The diagnosticCollector windows will tell the user that there are more references that in reality there are, confusing the user. The expected behavior would be if the user dispose a Transient object the DiagnosticsCollector's reference count decrease.

Ah yes, it might be confusing because it is 'Ref Count'. However, it is probably impossible to get a reference count, since it also handles Plain C# classes. This number is simply the number of instances created. This would still be useful enough. Changing the labels might be something to consider.

In any case, the answer to the memory leak, which is the topic of this issue, is to Dispose. If there is still some issue I am not aware of, Reopen is welcome. Thanks a lot.

Translated with www.DeepL.com/Translator (free version)