gdext icon indicating copy to clipboard operation
gdext copied to clipboard

Single- and multi-threading support for `Callable::from_fn`

Open MatrixDev opened this issue 2 years ago • 1 comments

Callable::from_fn requires Send + Sync from closures but Gd<T> is not. I saw in few bugs Callable::from_fn is mentioned as an alternative to using function name Base::callable / Callable::from_object_method (which are not type safe because function name can be misspelled) but it doesn't really work.

Basically this limitation limits usefulness of Callable::from_fn quite a bit.

Possibly related to #18 but usecase is different. Maybe Callable can be adjusted instead of Gd<T>.

Code

#[derive(GodotClass)]
#[class(init, base = Node3D)]
pub struct TestNode {
    base: Base<Node3D>,
    #[export]
    button: Option<Gd<Button>>,
}

#[godot_api]
impl TestNode {
    fn on_clicked(&mut self) {}
}

#[godot_api]
impl INode3D for TestNode {
    fn ready(&mut self) {
        if let Some(mut button) = &self.button {
            let mut this = self.to_gd();
            let callback = Callable::from_fn("callback", move |args| {
                this.bind_mut().on_clicked();
                Ok(Variant::nil())
            });
            button.connect("pressed".into(), callback);
        }
    }
}

Error

error[E0277]: `*mut TestNode` cannot be sent between threads safely
   --> src/nodes/screens/ship_edit.rs:101:56
    |
101 |               let callback = Callable::from_fn("sensor", move |args| {
    |                              -----------------           ^----------
    |                              |                           |
    |  ____________________________|___________________________within this `{closure@src/nodes/screens/ship_edit.rs:101:56: 101:67}`
    | |                            |
    | |                            required by a bound introduced by this call
102 | |                 this.bind_mut().on_clicked();
103 | |                 Ok(Variant::nil())
104 | |             });
    | |_____________^ `*mut TestNode` cannot be sent between threads safely
    |
    = help: within `{closure@src/nodes/screens/ship_edit.rs:101:56: 101:67}`, the trait `Send` is not implemented for `*mut TestNode`
note: required because it appears within the type `RawGd<TestNode>`
   --> /Users/matrixdev/.cargo/git/checkouts/gdext-76630c89719e160c/df302ea/godot-core/src/obj/raw.rs:30:12
    |
30  | pub struct RawGd<T: GodotClass> {
    |            ^^^^^
note: required because it appears within the type `Gd<TestNode>`
   --> /Users/matrixdev/.cargo/git/checkouts/gdext-76630c89719e160c/df302ea/godot-core/src/obj/gd.rs:87:12
    |
87  | pub struct Gd<T: GodotClass> {
    |            ^^
note: required because it's used within this closure
   --> src/nodes/screens/ship_edit.rs:101:56
    |
101 |             let callback = Callable::from_fn("sensor", move |args| {
    |                                                        ^^^^^^^^^^^
note: required by a bound in `godot::prelude::Callable::from_fn`
   --> /Users/matrixdev/.cargo/git/checkouts/gdext-76630c89719e160c/df302ea/godot-core/src/builtin/callable.rs:95:22
    |
93  |     pub fn from_fn<F, S>(name: S, rust_function: F) -> Self
    |            ------- required by a bound in this associated function
94  |     where
95  |         F: 'static + Send + Sync + FnMut(&[&Variant]) -> Result<Variant, ()>,
    |                      ^^^^ required by this bound in `Callable::from_fn`

MatrixDev avatar Jan 31 '24 14:01 MatrixDev

Callable::from_fn requires Send + Sync from closures but Gd<T> is not.

This is mostly a direct consequence of #18 not yet being implemented.

Mid-term, I see the option to have two functions:

  1. Callable::from_unsync_fn()

    • This would record the thread ID on creation and only allow execution on the same thread.
    • Even if called from GDScript, it could simply skip the user code and print an error if invoked from a different thread.
    • It would not require Send/Sync.
  2. Callable::from_sync_fn()

    • Requires Send/Sync and can be called from any thread.
    • To be actually useful, it needs threading support of objects.

Bromeon avatar Jan 31 '24 14:01 Bromeon