raulsntos / godot-dotnet

MIT License
86 stars 13 forks source link

Prevent Unexpected GC of Valid GodotObject #22

Open scgm0 opened 2 months ago

scgm0 commented 2 months ago

GC may accidentally collect a GodotObject instance since DisposablesTracker only holds a WeakReference to it.

While the DisposablesTracker has applied a similar mechanism in the GodotSharp, a direct reference to the GodotObject instance is retained in CustomGCHandle to keep the GodotObject alive.

godot-dotnet currently does not implement such CustomGCHandle, so I added a direct reference to the GodotObject instance in the DisposablesTracker instead. Still, the RefCounted instance continues to hold a weak reference, just like what GodotSharp does.

takethebait commented 2 months ago

Just tested these changes and unfortunately they do not fix #20 for me. I still get the same unref errors:

image

scgm0 commented 2 months ago

Just tested these changes and unfortunately they do not fix #20 for me. I still get the same unref errors:

image

It seems they are two different issues, I just don't know why they always come up at the same time in my case

scgm0 commented 2 months ago

@raulsntos I found that Callable also has unexpected GC problems, which makes the signal linking method of += completely unavailable. Even if you use Connect to manually link the signal, you need to keep a separate reference for the instance created by Callable.From. Only in this way can we ensure that the signal link method can always be executed normally. Should we not use weak references for Callable?

raulsntos commented 2 months ago

I've noticed the same issues and I made the following changes locally. I was considering pushing these changes but I was on the fence about it.

diff --git a/src/Godot.Bindings/Bridge/Callables/CustomCallable.cs b/src/Godot.Bindings/Bridge/Callables/CustomCallable.cs
index 6032ff5..09f2809 100644
--- a/src/Godot.Bindings/Bridge/Callables/CustomCallable.cs
+++ b/src/Godot.Bindings/Bridge/Callables/CustomCallable.cs
@@ -25,7 +25,7 @@ public abstract class CustomCallable

     internal unsafe NativeGodotCallable ConstructCallable()
     {
-        var gcHandle = GCHandle.Alloc(this, GCHandleType.Weak);
+        var gcHandle = GCHandle.Alloc(this, GCHandleType.Normal);

         var info = new GDExtensionCallableCustomInfo2()
         {
diff --git a/src/Godot.Bindings/Bridge/ClassDB/ClassDBRegistrationContext.cs b/src/Godot.Bindings/Bridge/ClassDB/ClassDBRegistrationContext.cs
index 1ee4309..e374532 100644
--- a/src/Godot.Bindings/Bridge/ClassDB/ClassDBRegistrationContext.cs
+++ b/src/Godot.Bindings/Bridge/ClassDB/ClassDBRegistrationContext.cs
@@ -13,7 +13,7 @@ public partial class ClassDBRegistrationContext

     internal ClassDBRegistrationContext(StringName className)
     {
-        GCHandle = GCHandle.Alloc(this, GCHandleType.Weak);
+        GCHandle = GCHandle.Alloc(this, GCHandleType.Normal);
         ClassName = className;
     }
 }
diff --git a/src/Godot.Bindings/Core/GodotObject.cs b/src/Godot.Bindings/Core/GodotObject.cs
index d4393f1..9b480f2 100644
--- a/src/Godot.Bindings/Core/GodotObject.cs
+++ b/src/Godot.Bindings/Core/GodotObject.cs
@@ -26,7 +26,7 @@ partial class GodotObject : IDisposable
     /// <param name="nativePtr">The pointer to the native object in the engine's side.</param>
     internal protected GodotObject(nint nativePtr)
     {
-        _gcHandle = GCHandle.Alloc(this, GCHandleType.Weak);
+        _gcHandle = GCHandle.Alloc(this, GCHandleType.Normal);

         NativePtr = nativePtr;
scgm0 commented 2 months ago

I've noticed the same issues and I made the following changes locally. I was considering pushing these changes but I was on the fence about it.

diff --git a/src/Godot.Bindings/Bridge/Callables/CustomCallable.cs b/src/Godot.Bindings/Bridge/Callables/CustomCallable.cs
index 6032ff5..09f2809 100644
--- a/src/Godot.Bindings/Bridge/Callables/CustomCallable.cs
+++ b/src/Godot.Bindings/Bridge/Callables/CustomCallable.cs
@@ -25,7 +25,7 @@ public abstract class CustomCallable

     internal unsafe NativeGodotCallable ConstructCallable()
     {
-        var gcHandle = GCHandle.Alloc(this, GCHandleType.Weak);
+        var gcHandle = GCHandle.Alloc(this, GCHandleType.Normal);

         var info = new GDExtensionCallableCustomInfo2()
         {
diff --git a/src/Godot.Bindings/Bridge/ClassDB/ClassDBRegistrationContext.cs b/src/Godot.Bindings/Bridge/ClassDB/ClassDBRegistrationContext.cs
index 1ee4309..e374532 100644
--- a/src/Godot.Bindings/Bridge/ClassDB/ClassDBRegistrationContext.cs
+++ b/src/Godot.Bindings/Bridge/ClassDB/ClassDBRegistrationContext.cs
@@ -13,7 +13,7 @@ public partial class ClassDBRegistrationContext

     internal ClassDBRegistrationContext(StringName className)
     {
-        GCHandle = GCHandle.Alloc(this, GCHandleType.Weak);
+        GCHandle = GCHandle.Alloc(this, GCHandleType.Normal);
         ClassName = className;
     }
 }
diff --git a/src/Godot.Bindings/Core/GodotObject.cs b/src/Godot.Bindings/Core/GodotObject.cs
index d4393f1..9b480f2 100644
--- a/src/Godot.Bindings/Core/GodotObject.cs
+++ b/src/Godot.Bindings/Core/GodotObject.cs
@@ -26,7 +26,7 @@ partial class GodotObject : IDisposable
     /// <param name="nativePtr">The pointer to the native object in the engine's side.</param>
     internal protected GodotObject(nint nativePtr)
     {
-        _gcHandle = GCHandle.Alloc(this, GCHandleType.Weak);
+        _gcHandle = GCHandle.Alloc(this, GCHandleType.Normal);

         NativePtr = nativePtr;

Looks good, I'll try it