Panic when calling deferred reload through new typed API

[sylbeth]

So, my build plugin is made with godot-rust, and it seems like GDExtensionManager's reload extension is incompatible when reloading the library it's being called from. This does make a lot of sense intuitively as the gdextension is still performing work inside that closure, but I thought it would be sensible to report it anyway. Here is the error message, but I don't know how to access anything else:

panicked at C:\Users\Sylbeth\scoop\persist\rustup\.cargo\git\checkouts\gdext-067f4b88e7bd088f\517053d\godot-core\src\private.rs:367:49:

thread panicked while processing panic. aborting.

The code for the build plugin's reloading I have here:

//! Build plugin, to manage the build script.

use std::sync::atomic::{AtomicBool, Ordering};

use godot::{
    classes::{
        EditorPlugin, GDExtensionManager, IEditorPlugin, editor_plugin::CustomControlContainer,
    },
    global::{godot_print, godot_str},
    obj::{Base, WithBaseField, WithDeferredCall},
    prelude::{GodotClass, godot_api},
};

use buttons::{BuildButtons, construct_build_buttons};

pub mod buttons;
pub mod commands;

static MUST_RELOAD: AtomicBool = AtomicBool::new(false);

/// A plugin that adds buttons for the ease of this gdext compilation.
#[derive(GodotClass)]
#[class(tool, init, base=EditorPlugin)]
pub struct BuildPlugin {
    /// Buttons loaded by the plugin.
    buttons: Option<BuildButtons>,

    /// Base class of the node.
    base: Base<EditorPlugin>,
}

#[godot_api]
impl IEditorPlugin for BuildPlugin {
    fn enter_tree(&mut self) {
        let buttons = construct_build_buttons();
        for button in buttons.iter() {
            self.base_mut()
                .add_control_to_container(CustomControlContainer::TOOLBAR, button);
        }
        self.buttons = Some(buttons);
    }

    fn ready(&mut self) {
        godot_print!("BuildPlugin: Ready!");
    }

    fn process(&mut self, _delta: f32) {
        if MUST_RELOAD.swap(false, Ordering::Acquire) {
            GDExtensionManager::singleton().apply_deferred(|gdextension_manager| {
                godot_print!("BuildPlugin: Reloading GDExtension.");
                gdextension_manager
                    .reload_extension(&godot_str!("res://{}", env!("GDEXTENSION-FILE-PATH")));
            });
        }
    }

    fn exit_tree(&mut self) {
        if let Some(buttons) = std::mem::replace(&mut self.buttons, None) {
            for button in buttons {
                self.base_mut()
                    .remove_control_from_container(CustomControlContainer::TOOLBAR, &button);
                button.free();
            }
        }
    }
}

[sylbeth]

And yes, changing the process function to this does solve it:

fn process(&mut self, _delta: f32) {
    if MUST_RELOAD.swap(false, Ordering::Acquire) {
        godot_print!("BuildPlugin: Reloading GDExtension.");
        GDExtensionManager::singleton().call_deferred(
            "reload_extension",
            &[godot_str!("res://{}", env!("GDEXTENSION-FILE-PATH")).to_variant()],
        );
    }
}

[sylbeth]

I am using feature experimental threads, since I thought I needed to spawn a thread for it, but not really, I was just using output before. But I am indeed using it for printing on the console.

[Yarwin]

GDExtensionManager's reload extension is incompatible when reloading the library it's being called from.

During hot-reload all the custom callables (which means any callable but ones made from object methods, including typed signals, linked callables, from_local_fn etc) from reloaded extension become invalid and accessing them might cause instant UB, see: #1221.

[sylbeth]

Yeah, thought something like that may be the case, I don't know if it's even possible to do it. But then, can reload extension for your own library even be used? It's always going to be inside a custom function unless it's call deferred in the variant way.

[TitanNano]

But then, can reload extension for your own library even be used

My gut feling ia that we should remove this API and only expose it as a deferred call. I.e. reload_extension_deferred 🤔

[sylbeth]

I don't know if a non deferred call can work on reloading another extension, though. But indeed, deferred call is always going to be safer

[Bromeon]

Not really sure this is a bug; rug-pulling the currently running extension cannot work well.

If Godot (the runtime) doesn't support this, what is godot-rust supposed to do when such a method is called? The standing-to-reason thing would be to validate whether the extension being reloaded is the current one. That's possible, but it doesn't solve the problem. Imagine this call stack:

> [Other.gdextension]  f()
> [This.gdextension]   g()
> [This.gdextension]   reload_extension(Other)

It's still a rugpull with 1 indirection and would cause the same problem.

---

My gut feling ia that we should remove this API and only expose it as a deferred call. I.e. reload_extension_deferred 🤔

Either that (but then for all three load/unload/reload methods), or add a warning about the reloading. There may be valid use cases for these, I asked in RocketChat.

[sylbeth]

The standing-to-reason thing would be to validate whether the extension being reloaded is the current one.

How can that be done, exactly?

It's still a rugpull with 1 indirection and would cause the same problem.

I don't get it, why would two unrelated and unlinked extensions not be able to reload each other in a deferred call?

add a warning about the reloading.

I would encourage this, at least, to say it should never be used in typed deferred calls to the same library.

There may be valid use cases for these

I am currently using it in the same extension to hot reload it when clicking a build button. The exact same extension, since I made a quick and dirty build plugin inside, and it works great, just have to keep in mind to use the variant call deferred.

[Bromeon]

The standing-to-reason thing would be to validate whether the extension being reloaded is the current one.

How can that be done, exactly?

I would probably compare the resource path -- but I don't think it's relevant, since that approach would mean outlawing reloading at all. The method can't detect if it's being called in deferred mode or not 🙂
_{(Maybe it can be detected with some stacktrace heuristics, but absolutely hacky.)}

---

I don't get it, why would two unrelated and unlinked extensions not be able to reload each other in a deferred call?

I meant a non-deferred call, which is the GDExtension class' design as exposed now. This is a fundamentally low-level API with certain dangers.

---

I think I misunderstood the problem slightly before -- I agree it's unintuitive that deferred calls work, but typed deferred ones don't. However, I don't think we can fix it due to the nature of callables breaking after reload.

There are a few things we can do:

• Document apply_deferred to not be used across reloads.
• Mark 3 GDExtension methods unsafe and offer safe defer_* variants for them.
• Possibly runtime checks?
  • Callables would need to be invalidated across reload, but how? Their state is not accessible after reload -> #1221
  • Anything else?