Panic when using `to_init_gd` in `RefCounted` when opening Godot editor

[beicause]

#[derive(GodotClass)]
#[class(base=RefCounted)]
pub struct MyRefCounted {
    base: Base<RefCounted>,
    #[export]
    some_string: GString,
}

#[godot_api]
impl IRefCounted for MyRefCounted {
    fn init(base: Base<RefCounted>) -> Self {
        let sf = base.to_init_gd();
        Self {
            base,
            some_string: "".into(),
        }
    }
}

Godot editor trys to generate docs for the class:

However, ClassDB::class_get_default_property_value instantiates the object and immediately delete it, which causes drop_strong_ref to panic, see the source code in: https://github.com/godotengine/godot/blob/6339f31a0217038ce6ed5e16776e34654895edc6/core/object/class_db.cpp#L2220-L2240

[Bromeon]

Every day I dislike Godot's RefCounted quirks a bit more 😔

What exactly is the panic you're facing?
And do you have an idea what we could do against it? 🤔

[Yarwin]

Probably RawGd::drop. We can link callable with deferred refcount to object (thus invalidating it when object is being freed), but it would effectively cause the memory leak 🤔. We can also check for object validity (non-optionally, every time, for every single instance) and avoid a drop if it would result in panic.

We can also bring it up to upstream. Personally I think that _init in current form is way too cursed to rely on it (newly created instance is being used to way too many things – placeholder, docs…) and we should get callback similar to gdscript's _init.

[Bromeon]

We can also check for object validity (non-optionally, every time, for every single instance) and avoid a drop if it would result in panic.

Maybe this particular example is solvable by validity check -- I do wonder if there is any other condition we can check to detect "in editor"?

@beicause could you see if updating the logic here -- to not drop if Gd::is_instance_valid() returns false -- fixes your issue? Without creating a memory leak, that is -- i.e. call Gd::drop_weak().

gdext/godot-core/src/obj/base.rs

Lines 239 to 251 in 7118691

	/// Drops any extra strong references, possibly causing object destruction.
	fn drop_strong_ref(instance_id: InstanceId) {
	PENDING_STRONG_REFS.with(|refs| {
	let mut pending_refs = refs.borrow_mut();
	let strong_ref = pending_refs.remove(&instance_id);
	assert!(
	strong_ref.is_some(),
	"Base unexpectedly had its strong ref rug-pulled"
	);
	// Triggers RawGd::drop() -> dec-ref -> possibly object destruction.
	});
	}

---

More generally, we should decide if

1. we bring the RefCounted init/drop issues upstream
2. we keep working around each individual of them in godot-rust

But I feel like (2) might be a never-ending quest. I don't feel like playing cat-and-mouse forever; at some point we need to call it a day, document the limitations and live with it. That is, only fix critical issues. We've spent too much dev resources on this topic already, and it brings relatively little value for games overall. At this point I even wonder whether to_init_gd for refcounted objects was a good idea, with all the pitfalls it introduced.

I would also encourage you @beicause or anyone affected by this to describe the problem and a proposal upstream (and maybe lobby for it in RocketChat), and I appreciate the past reports on this 👍 But it seems like a holistic design would be appropriate rather than specialized fixes. I'll gladly support such efforts, but I don't want to do it alone.

[TitanNano]

I also think this should be brought upstream.

[beicause]

I can confirm this fixes the panic:

diff --git a/godot-core/src/obj/base.rs b/godot-core/src/obj/base.rs
index 3461d04a..f042b047 100644
--- a/godot-core/src/obj/base.rs
+++ b/godot-core/src/obj/base.rs
@@ -245,7 +245,9 @@ impl<T: GodotClass> Base<T> {
                 strong_ref.is_some(),
                 "Base unexpectedly had its strong ref rug-pulled"
             );
-
+            if !instance_id.lookup_validity() {
+                strong_ref.unwrap().drop_weak();
+            }
             // Triggers RawGd::drop() -> dec-ref -> possibly object destruction.
         });
     }

---

I'm not sure if it's an upstream issue or a problem with godot-rust. I think accessing base in init (and possible predelete) is OK as long as we don't touch the reference count, at least this is not a problem in godot-cpp.
However, accessing base in drop and the predelete in gdscript are indeed problems of upstream.

[Yarwin]

1. we bring the RefCounted init/drop issues upstream

I think this would be the good call – if we are running into these issues then Godot-dotnet will splash against them as well.

Exposing a way to create weakref wouldn't fully solve this issue – it would create a lot of hard to debug jank if user would want to create some kind of side effects (for example – while instantiating resource X do some extra amount of work and store reference to itself in some user singleton). Some Godot abstractions require doing extra work in init (for example CompositorEffect) as well, so supporting it is not optional

I'll try to prepare some examples with godot-cpp for the next GDExtension meeting.

[Bromeon]

@beicause Thanks a lot for testing!

I think accessing base in init (and possible predelete) is OK as long as we don't touch the reference count

Yes, but this is still quite a limitation (e.g. when you want to register each newly created object in some global inventory/registry).

Also, it's very hard to enforce: even if we outlaw clone (panic or only hand out &mut T instead of Gd<T>), there are methods like Resource::duplicate (?) or Object::call which can increment refs 🤔

[beicause]

Yes, but this is still quite a limitation (e.g. when you want to register each newly created object in some global inventory/registry).

I don't understand why is this a limitation.

there are methods like Resource::duplicate (?) or Object::call which can increment refs

From what I saw, only converting pointer to Ref or Varaint can increment refs in godot. But in godot-rust base() constructs a strong ref. Can't base() return a weak ref?

[Bromeon]

I don't understand why is this a limitation.

Cloning and other operations incrementing refcounts are part of the Gd public API. It's very unintuitive when these must not be touched in certain situations, or worse, if they can accidentally be touched and this isn't validated.

From what I saw, only converting pointer to Ref or Varaint can increment refs in godot. But in godot-rust base() constructs a strong ref. Can't base() return a weak ref?

Good point! I tried this in #1302, but there's a memory leak that needs investigation. Not sure if it's feasible.