C# .Dispose() should free the object from memory

[Beider]

Tested versions

-Reproducible in: Godot 4.2.1 Mono

System information

Godot v4.2.1.stable.mono - Windows 10.0.22621 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 2080 Ti (NVIDIA; 31.0.15.3770) - Intel(R) Core(TM) i9-9900 CPU @ 3.10GHz (16 Threads)

Issue description

On some godot objects such as TreeItem calling .Dispose() on them does not free them from memory but makes you unable to call any further methods on the item. So you can not even call .Free() to free the item from memory thus creating a memory leak.

In my use case I am filtering a tree by removing certain tree items. I wanted to free these from memory so I used the standard C# way of doing so by calling .Dispose() but this caused memory leaks. You can fix this by calling .Free() instead, however since TreeItem has the .Dispose() method I would assume this would free it from memory as that is normally the case in C#.

I think the best solution here is to make .Dispose() work as you would expect in C# and have it internally call Free() to free up the object. Either that or do not offer .Dispose() as an option at all, in C# you would expect that if an object is disposable then calling .Dispose() will completely dispose of the object from memory.

Unless I am missing something here, with the current implementation, is there any circumstance in which I would want to call .Dispose() on a TreeItem myself? To me it seems like there would never be such a use case.

Steps to reproduce

Here is some C# code that will cause leaks. Removing the dispose will allow you to free the children as expected.

Tree tree = FindChild("Tree") as Tree;
tree.Clear();

tree.Columns = 1;
tree.SetColumnTitle(0, "Name");

var root = tree.CreateItem(null);
root.SetText(0, "Root");
for (int i = 0; i < 10; i++)
{
    var child = tree.CreateItem(root);
    child.SetText(0, $"Child {i}");

    root.RemoveChild(child);
    child.Dispose(); // You now have a memory leak
    child.Free(); // Does not work after dispose
}

Minimal reproduction project (MRP)

DisposeIssue.zip

[bs-mwoerner]

I think there's a slight misunderstanding here on what disposing an object means in the C# world.

In C#, objects will be freed automatically by the runtime when they are no longer referenced. For performance reasons, this does not happen every time an object is unreferenced but is done in batches during a "garbage collection". Now, some objects may have external effects just by their mere existence: If you open a FileStream object, for example, the file it references will be locked while the FileStream exists. Often, you don't want to keep the lock until the stream is eventually removed from memory, so FileStream implements a Close() method that you can call to tell it to release the underlying file handle. The FileStream itself will live on, but further reads or writes will obviously give you an error now.

The disposable pattern is a generalization of this concept: Some managed objects hold on to unmanaged resources and you want a way to have them "dispose" of these resources at a specific point in the program without having to wait for the managed object to be freed at some unspecific point in the future. Consequently, FileStream implements the IDisposable interface and the more general Dispose() method as an alternative to the stream-specific Close() method (both methods do the same).

When writing C# scripts for Godot, many of the objects you use will be wrappers around the underlying Godot objects. These underlying Godot objects are unmanaged resources from C#'s point of view and thus the GodotObject class at the root of the class hierarchy for all these C# wrapper objects implements IDisposable. Calling Dispose() on one such objects severs the connection between the C# object and the underlying Godot object. If the Godot object is not referenced anywhere else, this will free the Godot object. The C# wrapper object, however, will live on in its disposed state and give you an error if you try to continue using it.

In your example, you're calling Dispose() to disconnect the C# object from the Godot TreeItem. This may actually free the TreeItem here, since you've previously removed it from the tree and while your child wrapper object still exists, it internally no longer references the TreeItem after the call to Dispose(). You then call child.Free(), which fails because there is no longer a Godot object whose free() method this could call.

Now, you might argue that it should be permissible to call Free() after Dispose(), just like you can call Close() on a FileStream() after Dispose(), because while all other methods stop working, it is generally not considered an error to try to dispose an already disposed object. In this case, however, I think Godot's Free() and Dispose() aren't actually the same thing:

• Dispose() means "I (the C# script) don't need this object anymore, so please free it at this point if no one else is using it".
• Free() means "Please free this object at this point, whether it's still in use or not"

So I think it's actually correct to treat Free() like any other operation and disallow calling it on disposed objects. There won't be a memory leak either way, because the underlying Godot object ceased to exist after the first call to Dispose().

[Beider]

I did not include this in my original post and probably should have. But your assumption that Dispose() frees the object is wrong. This is the output from godot with --verbose after runnig my example project and clicking the button a few times. Calling Dispose() does not free the object, this is the problem.

Leaked instance: TreeItem:1028593127785675
Leaked instance: TextParagraph:-9222345092994431747
Leaked instance: TextParagraph:-9222342893971176194
Leaked instance: TextParagraph:-9222343993482803968
Leaked instance: TreeItem:1026394104530179
Leaked instance: TreeItem:1027493616157956
Leaked instance: TreeItem:1005503383602442
Leaked instance: TextParagraph:-9222365983715359477
Leaked instance: TreeItem:1006602895230220
Leaked instance: TextParagraph:-9222364884203731699
Leaked instance: TreeItem:1007702406857998
Leaked instance: TextParagraph:-9222363784692103921
Leaked instance: TreeItem:1008801918485776
Leaked instance: TextParagraph:-9222362685180476143
Leaked instance: TreeItem:1009901430113554
Leaked instance: TextParagraph:-9222361585668848365
Leaked instance: TreeItem:1011000941741332
Leaked instance: TextParagraph:-9222360486157220587
Leaked instance: TreeItem:1012100453369110
Leaked instance: TextParagraph:-9222359386645592809
Leaked instance: TreeItem:1013199964996888
Leaked instance: TextParagraph:-9222358287133965031
Leaked instance: TreeItem:1014299476624666
Leaked instance: TextParagraph:-9222357187622337253
Leaked instance: TreeItem:1015398988252444
Leaked instance: TextParagraph:-9222356088110709475
Leaked instance: TreeItem:1029692639413534
Leaked instance: TextParagraph:-9222341794459548385
Leaked instance: TreeItem:1030792151041312
Leaked instance: TextParagraph:-9222340694947920607
Leaked instance: TreeItem:1031891662669090
Leaked instance: TextParagraph:-9222339595436292829
Hint: Leaked instances typically happen when nodes are removed from the scene tree (with `remove_child()`) but not freed (with `free()` or `queue_free()`).
ERROR: 40 RID allocations of type 'PN18TextServerAdvanced22ShapedTextDataAdvancedE' were leaked at exit.
WARNING: ObjectDB instances leaked at exit (run with --verbose for details).
     at: cleanup (core/object/object.cpp:2208)

If I use Free() instead of Dispose() this does not happen and the memory does not leak. My suggestion is therefore that Dispose() for TreeItems (and probably other objects that are affected) should free up the memory if you call Dispose(). If not immediately at least make sure it gets collected eventually. As it currently is calling Dispose() causes a memory leak since you are unable to free the object yourself afterwards and the underlying C++ implementation never frees it either.

[bs-mwoerner]

Ah, you're right. I assumed TreeItem was reference counted. It's actually not, so whoever "owns" it is responsible for releasing its memory. For these kinds of object, simply disposing the C# wrapper won't free the underlying Godot object. You need to call Free() explicitly, just like you would in GD Script, and just like the documentation (which applies to both C# and GD Script) says:

To create a TreeItem, use Tree.create_item or create_child. To remove a TreeItem, use Object.free.

To be able to call Object.free(), however, you need your wrapper object, so if you want to call Dispose(), you need to do that after you free the underlying Godot object.

I can see how it may be unclear how one is supposed to handle this from the C# side, especially since in my FileStream example, Close() and Dispose() were the same thing. Here, however, Dispose() only disposes the wrapper object and does not call any methods of the underlying object, not even free(). It may be worthwhile to discuss whether it could do that for convenience, but consider that Godot uses a mixed model, where some objects are reference counted and should not be freed while they are still referenced. Also, I think one goal is to keep the differences between GD Script and C# at a minimum, so it may be contraproductive to introduce C# mechanics that call free() implicitly in some circumstances, especially since tracking when and where something is freed is complicated enough as it is.

[Beider]

Also, I think one goal is to keep the differences between GD Script and C# at a minimum

I find this a really bad argument though, as of right now the case is that the standard way of disposing of items in C# leads to a memory leak. One thing is not offering Dispose() at all, but to have some objects leak if you call Dispose() is really bad in my opinion. This means that if you want to override dispose in a subclass of TreeItem or if you want to collect many different items and dispose them all this is not possible without reading the documentation for each type of item.

Why not have all items that are not reference counted just call Free() immediately if they are disposed. If the developer calls Dispose() they got no way to access the item anymore anyway so might as well free it.

At the very least then an error message should be added to say that you should not use Dispose() on objects that are not reference counted, but at that point you might as well just free it because you have to check anyway.

I think when professional C# developers coming from other places (such as Unity) try out godot they would expect Dispose() to work like you would assume in C#. To tell them "It's in the documentation which objects it doesn't work on" will just feel lazy.

[bs-mwoerner]

I think there really is no good argument not to just Free() non-reference counted objects when you call Dispose()

I tried to hint at a few reasons above, but here's a more elaborate argument: By convention, an object implementing IDisposable is disposed when it is finalized. If disposing a C# wrapper object would cause the underlying Godot object to be freed, any Godot object for which there is a wrapper object would be freed once the wrapper is finalized. Now consider a slight variant of your example above:

for (int i = 0; i < 10; i++)
{
    var child = tree.CreateItem(root);
    child.SetText(0, $"Child {i}");
}

This creates 10 TreeItem objects along with C# wrapper objects and uses the wrappers to set the text on the items. The 10 items appear in the tree and the method completes. Then, at some unspecific time in the future, the .NET garbage collector runs and determines that all those wrapper objects are no longer referenced. It finalizes them, which disposes them, which, following your suggestion, would cause the underlying Godot TreeItems to be freed. As a consequence, all items created (or even just accessed) in C# would eventually disappear from the tree at seemingly random times.

I think when professional C# developers coming from other places (such as Unity) try out godot they would expect Dispose() to work like you would assume in C#.

The assumption that Dispose() always destroys the underlying object is not correct, though. Consider this:

for (int i = 0; i < 10; i++)
{
    var file = File.Create($"file{i}.txt");
    file.Write(Encoding.UTF8.GetBytes($"Child {i}"));
    file.Dispose();
}

At the very least then an error message should be added to say that you should not use Dispose() on objects that are not reference counted, but at that point you might as well just free it because you have to check anyway.

Having a warning message when explicitly calling Dispose() on a GodotObject that is not a RefCounted might actually be a good idea if that's possible with the current analyzers. I'd expect that in the majority of these cases, calling Dispose() directly isn't all that useful and might indeed be an indication of the developer trying to achieve something else.

[Beider]

As far as I am aware calling Dispose on a FileStream or any other stream will close the stream and then it would be garbage collected later on when the GC gets around to it. It does not delete the underlying file, but claiming that because file stream does not delete the underlying file it is fine for dispose to cause memory leaks is a stretch.

I'd expect that in the majority of these cases, calling Dispose() directly isn't all that useful and might indeed be an indication of the developer trying to achieve something else.

If a developer is calling Dispose() I think it is pretty clear what they are trying to do. They are trying to ensure the object is cleaned up properly and eventually the memory is freed up. I am really confused by this statement, even reading the IDisposable interface page the entire interface is described in one simple line.

Provides a mechanism for releasing unmanaged resources.

The TreeItem that lives in the CPP code is an unmanaged resource from the C# developers point of view. Expecting unmanaged resources to be cleaned up when you call Dispose on an object that implements IDisposable is not weird. It's pretty much expecting the object to implement the interface properly.

I get that this is obviously because of the way C# was implemented into godot, but I stand behind that having a disposable object leak memory if you call dispose on it is an anti-pattern. In that case it should probably not be disposable at all.

Or in the worst case implement Dispose(bool) like FileStream and make it free the underlying resource if you pass in true and add a warning if you dispose without passing in the flag that the underlying resource will not be released.

[bs-mwoerner]

Or in the worst case implement Dispose(bool) like FileStream and make it free the underlying resource if you pass in true.

Careful, that method actually does the opposite of what you think: Passing true here unreferences managed dependencies in addition to unmanaged resources (which it always releases). This method is not part of the public interface but rather an internal (protected) helper method often implemented by classes that also want to release managed resources when someone calls Dispose() explicitly. While this is not strictly necessary, releasing those references may make the corresponding objects available for garbage collection earlier. Since it's not safe to access managed resources during finalization, disposing explicitly and disposing during finalization must be handled differently and the internal Dispose(bool) method helps to avoid code duplication in this case. Calling Dispose(true) is the code path for explicit disposing, so that's equivalent to calling Dispose().

I am really confused by this statement

The idea of the disposable pattern is that in a garbage collection environment where objects are cleaned up asynchronously, you sometimes want at least part of that cleanup to happen early and synchronously. This is often the case when the mere existence of the objects blocks other tasks (such as a file that can't be used by other applications before a stream holding on to it is closed). Calling Dispose() on a FileStream basically says "you can clean up the FileStream object whenever you feel like it, but could you please close the internal file handle right now? I promise I won't use this FileStream again". In a correct implementation of the pattern, calling Dispose() may only ever change when something happens but not what happens, i.e. the FileStream will always close the handle when it is destroyed, calling Dispose() merely makes this part of the cleanup happen sooner.

With regard to the C# wrappers in Godot, this means that calling Dispose() merely makes happen now what would otherwise happen some time after the object goes out of scope. If Dispose() were to free the underlying object, then the object would also have to be freed without the call to Dispose() when the wrapper is cleaned up. However, just like the FileStream only owns the file handle but not the file and thus the file should remain if the FileStream is destroyed, the C# wrapper only owns a handle to the underlying Godot object and the Godot object should remain if the wrapper is destroyed:

var root = tree.CreateItem(null);
{
    var child = tree.CreateItem(root);
}
// Would we really be okay with the root node having no children at this point?

[Beider]

the C# wrapper only owns a handle to the underlying Godot object and the Godot object should remain if the wrapper is destroyed:

I was not aware that you were working with this separation, so ok if you look at it this way then the behavior of dispose is fine. But in cases where you have this sort of division you would expect proper garbage collection from the underlying implementation. If the underlying implementation does not garbage collect like with TreeItem (and other not ref counted objects) you would expect Dispose() to release the unmanaged resources attached to it.

From my point of view, as the current implementation is right now there is absolutely no circumstance under which you would want to call Dispose on any godot item that does not inherit from RefCounted. So then why should they even implement IDisposable.

Using TreeItem as an example, if you have a reference to a TreeItem that is still in a Tree then the Tree will delete the underlying object at some point and your reference will point to an invalid godot object. Thus you can just throw it away and the GC in C# will collect the C# wrapper at some point.

If you remove the TreeItem from the tree you have to call Free on it else you will leak memory, once you called Free there is no need to call Dispose since if you just drop the reference to the wrapper the GC will take care of it.

So in the end you have an object that implements IDisposable which never needs to be Disposed and if you make the wrong assumption and don't realise the wrapper / godot object separation exists it works as a memory leak trap for C# devs that are used to Disposing of objects to clean them up.

So given this separation wouldn't it then make logical sense to separate whatever logic currently is in Dispose to elsewhere, then have the finalizer/destructor call this logic from there. Then Dispose could be implemented to do one of two things.

1. If the object is RefCounted simply call the same logic as before and get rid of the wrapper
2. If the object is not RefCounted call Free as well to get rid of the underlying object before you call the same logic as before.

At least from my point of view this would make Dispose() behave the way you expect and it would no longer be a trap for memory leaks for non-ref counted objects. Also if someone calls dispose on an object like a TreeItem that is still in use by a Tree then that is just their fault.

Since the GC just calls the finalizer this would also solve the problem you have been describing where the GC would inadvertently free the underlying godot object.

[bs-mwoerner]

At least from my point of view this would make Dispose() behave the way you expect and it would no longer be a trap for memory leaks for non-ref counted objects.

Yes, although I think we can agree that a Dispose() method that frees unmanaged resources that are not freed upon normal finalization does not fulfill the requirements of the dispose pattern as outlined by Microsoft ("[The block that frees unmanaged resources] executes regardless of the value of the disposing parameter."). To avoid confusion and retain conformance with the pattern, we would need to give this "DisposePlus" method a different name such as Destroy() or Delete(). Or Free().

[Beider]

"[The block that frees unmanaged resources] executes regardless of the value of the disposing parameter."

Sure I can agree on that. But then surely you can agree that right before that it also says ("A block that frees unmanaged resources."), so you don't follow the pattern anyway since you don't free up non refcounted resources on dispose. The non ref counted objects in godot I would say are unmanaged. At the very least as soon as the TreeItem is removed from the tree it is an unmanaged resource, as proven by the fact that it leaks, if you wanted to follow the pattern you should free it.

So in the end with the current way C# is implemented in godot you probably have to choose in which way you want to break the pattern. Personally I think from the developer perspective doing in a way where an intentional call to Dispose frees up unmanaged resources makes more sense. If you dispose something that is still in use then you know it is your fault. However if I dispose something I know is not in use and it leaks memory that is the opposite of what I would expect and it certainly does not follow the dispose pattern.

[C-Higgins]

I think @bs-mwoerner makes compelling arguments; however, I didn't see a rebuttal to "it should not be disposable at all." If the developer never has good reason to call dispose, why does it implement IDisposable? Would anything be lost by removing that confusing implementation?