Toplevels create references to its subviews that arent being released upon disposal.

[a-usr]

Describe the bug
Toplevels create references to its subviews that arent being released upon disposal.

To Reproduce
Steps to reproduce the behavior:
-Attempt to run my updated version of the TestViewsDisposeCorrectly test, wich can be found here or in my pull request.
-Check Output

Expected behavior
Test completes without error

Screenshots

Desktop (please complete the following information):

• Android (Just kidding, Windows)

Smartphone (please complete the following information):
no

Additional context
...

[BDisp]

In debug mode optimization is off and IsAlive stay true. In release mode the optimization is on and IsAlive may be false and the unit test pass.

[a-usr]

In debug mode optimization is off and IsAlive stay true. In release mode the optimization is on and IsAlive may be false and the unit test pass.

However the View should have been GC'd considering it was disposed and all references I created where moved off the stack. And weak references don't prevent garbage collection of an object.

[BDisp]

Try run on release mode. On debug mode the compiler maintain them in the memory.

[a-usr]

I'll try that to be safe, but in my other project i do the exact same, and the objects are GC'd as long as there is absolutely no strong reference. even in Debug mode.

[BDisp]

I'll try that to be safe, but in my other project i do the exact same, and the objects are GC'd as long as there is absolutely no strong reference. even in Debug mode.

It's working now, see my PR a-usr#1. The culprit was some containers weren't being removed and the most important Application.Top wasn't being disposed as well.

[BDisp]

@a-usr did you already saw my PR to your branch?

[BDisp]

I saw that the ViewDisposalTest.cs is already in the develop branch and it's working well. Why do you want to change that?

[a-usr]

@a-usr did you already saw my PR to your branch?

yes, i did, but i wont merge it just yet because I first want to create a small demo to prove to you (and me) that stuff is being recycled even in debug mode, however i dont really have time for that atm, so ill do it maybe on monday.

[a-usr]

I saw that the ViewDisposalTest.cs is already in the develop branch and it's working well. Why do you want to change that?

You see, the way the test is supposed to work is by discovering all non-generic derived types of ˋviewˋ, add them to a ˋviewˋ which acts as a container, add the container to the current top level (which is a mistake, because there is no current toplevel, thus making ˋApplication.topˋ return ˋnullˋ) and then run the mainloop for 100 (i think) iterations, before removing the container from the toplevel and then trying to dispose it.

After that, the program makes the GC to collect all collectible objects (using a snippet from the Microsoft Tutorial to AssemblyLoadContext, if i find it I'll link it here) and then checks if the container was garbage collected.

If it wasn't something is holding a reference to the container which prevents it from being Garbage Collected.

In my PR i changed it so that the program creates a Toplevel and sets it as the mainloop top before the container is being added to it. Note that the actual logic from the test wasn't changed.

Now however, the test fails (or failed, your PR will probably have changed that) now that the container was actually added to an existing toplevel.

So, to sum up, althought not failing the test wasn't working properly, I noticed that when revisiting my code, which is why i fixed it, which resulted in the test failing. And that is a good thing because it prooves the test is doing what its suposed to do.

[BDisp]

I take back what I said that it is necessary to run the test in release mode. I've reverted the changes in the unit test. The cause of the test failing is that Application.Top isn't null and the Application class didn't execute the cleanup and releasing objects that were using the View class, which is done by the Application.Shutdown method. Since you are using the Application.Begin which really create an instance on the toplevel's stack, the Application.End isn't enough to clean all the objects and the Application.Shutdown must be called before invoke the GC .

[a-usr]

I take back what I said that it is necessary to run the test in release mode. I've reverted the changes in the unit test. The cause of the test failing is that Application.Top isn't null and the Application class didn't execute the cleanup and releasing objects that were using the View class, which is done by the Application.Shutdown method. Since you are using the Application.Begin which really create an instance on the toplevel's stack, the Application.End isn't enough to clean all the objects and the Application.Shutdown must be called before invoke the GC .

But why is it that Application.Run behaves differently than the combination of Application.Begin , Application.Iterate and Application.End?
I always imagined that Application.Run is only A wrapper for calling Application.Begin and then continuously calling Application.Iterate?

[a-usr]

Here is the demo I promised you:

using Terminal.Gui;

internal class Program
{
  static View? StaticReferencetoTestObj2;
  public View? InstanceReferenceToTestObj3;
  private static void Main(string[] args)
  {
    // create new instance of program class
    Program This = new Program();
    // start the test and retrieve the list of references
    var refs = This.Test();
    // check wether some new objects can be disposed
    TryCollectAll(refs);
    printCheckAlive(refs);
    // free Instance Reference, collect and print
    Console.WriteLine("Freeing instance reference to TestObj3");
    This.InstanceReferenceToTestObj3 = null;
    TryCollectAll(refs);
    printCheckAlive(refs);
    // free static reference, collect and print
    Console.WriteLine("Freeing static reference to TestObj2");
    StaticReferencetoTestObj2 = null;
    TryCollectAll(refs);
    printCheckAlive(refs);
    //end

  }
  private List<WeakReference> Test()
  {
    List<WeakReference> refs = new List<WeakReference>();
    //create new disposable objects and Weakreferences pointing to them.
    // you can use any disposable class type instead of Terminal.Gui's View
    View? TestObj1 = new();
    refs.Add(new WeakReference(TestObj1));
    View? TestObj2 = new();
    refs.Add(new WeakReference(TestObj2));
    View? TestObj3 = new();
    refs.Add(new WeakReference(TestObj3));
    View? TestObj4 = new();
    refs.Add(new WeakReference(TestObj4));
    View? TestObj5 = new();
    refs.Add(new WeakReference(TestObj5));
    // retrieve TestObj6 from a function instead of directly from a call to new();
    View? TestObj6 = createTestObj6();
    refs.Add(new WeakReference(TestObj6));
    // create additional references to object 2, 3 and 4
    StaticReferencetoTestObj2 = TestObj2;
    InstanceReferenceToTestObj3 = TestObj3;
    View? LocalReferenceToTestObj4 = TestObj4;

    // check wether the objects are alive
    Console.WriteLine("Initial Check");
    printCheckAlive(refs);
    // dispose objects
    // this is done implicitly to make extending of the test easier
    Console.WriteLine("Disposing all objects");
    foreach(WeakReference wr in refs)
    {
      ((IDisposable)wr.Target).Dispose();
    }

    // Try to collect all objects and check again
    TryCollectAll(refs);
    printCheckAlive(refs);
    // remove references from the TestObjI variables (Except TestObj5) and 
    // make sure all collectible objects are collected.
    Console.WriteLine("Setting TestObjI references to null (except for TestObj5)");
    TestObj1 = null;
    TestObj2 = null;
    TestObj3 = null;
    TestObj4 = null;
    //TestObj5 = null
    TestObj6 = null;
    TryCollectAll(refs);
    // check again
    printCheckAlive(refs);
    // check wether they are actually null
    Console.WriteLine("Are they null though?");
    Console.WriteLine("Is the TestObj1 variable null? " + (TestObj1 is null));
    Console.WriteLine("Is the TestObj2 variable null? " + (TestObj2 is null));
    Console.WriteLine("Is the TestObj3 variable null? " + (TestObj3 is null));
    Console.WriteLine("Is the TestObj4 variable null? " + (TestObj4 is null));
    Console.WriteLine("Is the TestObj5 variable null? " + (TestObj5 is null));
    Console.WriteLine("Is the TestObj6 variable null? " + (TestObj6 is null));
    Console.WriteLine();
    Console.Out.Flush();
    // return so that all leftover locals are freed and the GC evaluates wether their objects are eligible for collection
    Console.WriteLine("Returning from function and freeing locals");
    return refs;

  }
  static void printCheckAlive(List<WeakReference> refs)
  {
    for (int i = 0; i < refs.Count; i++)
    {
      Console.WriteLine("Was test object " + (i + 1) + " Garbage-Collected? " + !(refs[i].IsAlive));
    }
    Console.WriteLine();
  }

  static void TryCollectAll(List<WeakReference> refs)
  {
    foreach (WeakReference r in refs)
    {
      // Code snippet from
      // https://learn.microsoft.com/en-us/dotnet/standard/assembly/unloadability#use-a-custom-collectible-assemblyloadcontext
      // (at the very end of the section)
      for (int i = 0; r.IsAlive && (i < 10); i++)
      {
        GC.Collect();
        GC.WaitForPendingFinalizers();
      }
    }
  }
  static View createTestObj6()
  {
    return new View();
  }
}

Please notethat this is written in .NET Core. Thats why i can use just new() and nullable is enabled by default
For me this produces both in Debug and in Release mode the following output:

Initial Check
Was test object 1 Garbage-Collected? False
Was test object 2 Garbage-Collected? False
Was test object 3 Garbage-Collected? False
Was test object 4 Garbage-Collected? False
Was test object 5 Garbage-Collected? False
Was test object 6 Garbage-Collected? False

Disposing all objects
Was test object 1 Garbage-Collected? False
Was test object 2 Garbage-Collected? False
Was test object 3 Garbage-Collected? False
Was test object 4 Garbage-Collected? False
Was test object 5 Garbage-Collected? False
Was test object 6 Garbage-Collected? False

Setting TestObjI references to null (except for TestObj5)
Was test object 1 Garbage-Collected? False
Was test object 2 Garbage-Collected? False
Was test object 3 Garbage-Collected? False
Was test object 4 Garbage-Collected? False
Was test object 5 Garbage-Collected? False
Was test object 6 Garbage-Collected? False

Are they null though?
Is the TestObj1 variable null? True
Is the TestObj2 variable null? True
Is the TestObj3 variable null? True
Is the TestObj4 variable null? True
Is the TestObj5 variable null? False
Is the TestObj6 variable null? True

Returning from function and freeing locals
Was test object 1 Garbage-Collected? True
Was test object 2 Garbage-Collected? False
Was test object 3 Garbage-Collected? False
Was test object 4 Garbage-Collected? True
Was test object 5 Garbage-Collected? True
Was test object 6 Garbage-Collected? True

Freeing instance reference to TestObj3
Was test object 1 Garbage-Collected? True
Was test object 2 Garbage-Collected? False
Was test object 3 Garbage-Collected? True
Was test object 4 Garbage-Collected? True
Was test object 5 Garbage-Collected? True
Was test object 6 Garbage-Collected? True

Freeing static reference to TestObj2
Was test object 1 Garbage-Collected? True
Was test object 2 Garbage-Collected? True
Was test object 3 Garbage-Collected? True
Was test object 4 Garbage-Collected? True
Was test object 5 Garbage-Collected? True
Was test object 6 Garbage-Collected? True

Feel free to test this yourself :) !

[BDisp]

But why is it that Application.Run behaves differently than the combination of Application.Begin , Application.Iterate and Application.End?

No, the reason for this behavior is that Application.Top is only set to null on the Application.Shutdown which is wrong forcing call the Application.Shutdown method to set Application.Top to null when it must only be called to freeing all the objects that aren't yet disposed on exit.
I submitted the PR #2986 that fix this.

I always imagined that Application.Run is only A wrapper for calling Application.Begin and then continuously calling Application.Iterate?

Yes it run in a loop while running is true.

[BDisp]

Feel free to test this yourself :) !

Thanks. Why you commented //TestObj5 = null?

Edit:
Sorry, I already understanded that was because you instantiated from another object based on it and disposed after.

[BDisp]

I changed the unit test with and without Application.Shutdown method on the a-usr#1 which returns the same results on both after merge the #2986.

[BDisp]

Here is the demo I promised you:

I think you know that you great example works well because there isn't any issue with the View disposing. The only issue is with the derived class Toplevel that is on the static Application.Top field that wasn't setting to null after disposing. Right?

[a-usr]

Feel free to test this yourself :) !

Thanks. Why you commented //TestObj5 = null?

Edit: Sorry, I already understanded that was because you instantiated from another object based on it and disposed after.

Not really. At the time of writing this program I expected that the objects of TestObj 1, 4 and 6 to be recycled immedialty after their variables where set to null.

Now however i know that this isnt true.

I commented TestObj6 = null because i wanted to show that if you where to forget to set a local to null or somethng else, its object would live until the function ended.

However upon running the demo i noticed that setting the TestObj locals to null infact didn't release the objects. My best guess is that their references are kept on the stack frame untill overridden.

[a-usr]

Here is the demo I promised you:

I think you know that you great example works well because there isn't any issue with the View disposing. The only issue is with the derived class Toplevel that is on the static Application.Top field that wasn't setting to null after disposing. Right?

Yes, your right. However my intention for this demo was to show you when objects can be released, and to prove that this behaviour doesnt change regardless of wether your building in debug mode or in release mode.

And, since I myself think that this example is not too shabby, i also put it on a gist so that other people can have a look at it,

[BDisp]

However upon running the demo i noticed that setting the TestObj locals to null infact didn't release the objects. My best guess is that their references are kept on the stack frame untill overridden.

Since you are test View class on your demo you don't need to set them to null but calling only TestObj#.Dispose(). The only way for WeakReference work is create and dispose an instance that is returned from a method. If it's created and disposed on the same method that is checking it's state will always fail. For sure you don't need the for loop because they will be garbage collected after only one call to GC.Collect(); GC.WaitForPendingFinalizers();.

[a-usr]

For sure you don't need the for loop because they will be garbage collected after only one call to GC.Collect(); GC.WaitForPendingFinalizers();.

As the Microsoft Documentation says, that is not awlays the case. However this for loop only runs once anyways if one iteration is enough to dispose all objects at once.