Implementing a get_property_list(&mut self) method triggers the main thread check panic

[caphindsight]

I'm trying to implement an advanced resource that interacts with the editor, changing its property list at runtime.

After experimenting, I noticed that Godot calls _get_property_list from a different thread, resulting in my program panicking and crashing the engine.
Stacktrace:

[panic backtrace]                                                                                                                                                                                                                                                                                                                                                                              
0: std::backtrace::Backtrace::create                                                                                                                                                                                                                                                                                                                                                                  
at /rustc/ed61e7d7e242494fb7057f2657300d9e77bb4fcb/library\std\src\..\..\backtrace\src\backtrace/win64.rs:85:14                                                                                                                                                                                                                                                                   
1: std::backtrace::Backtrace::capture                                                                                                                                                                                                                                                                                                                                                                 
at /rustc/ed61e7d7e242494fb7057f2657300d9e77bb4fcb/library\std\src/backtrace.rs:296:9                                                                                                                                                                                                                                                                                             
2: godot_core::private::set_gdext_hook::{{closure}}                                                                                                                                                                                                                                                                                                                                                   
at /home/.../.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/godot-core-0.4.3/src/private.rs:320:44                                                                                                                                                                                                                                                                    
3: std::panicking::panic_with_hook                                                                                                                                                                                                                                                                                                                                                                    
at /rustc/ed61e7d7e242494fb7057f2657300d9e77bb4fcb/library\alloc\src/boxed.rs:1999:9                                                                                                                                                                                                                                                                                              
4: std::panicking::panic_handler::{{closure}}                                                                                                                                                                                                                                                                                                                                                         
at /rustc/ed61e7d7e242494fb7057f2657300d9e77bb4fcb/library\std\src/panicking.rs:700:13                                                                                                                                                                                                                                                                                            
5: std::sys::backtrace::__rust_end_short_backtrace                                                                                                                                                                                                                                                                                                                                                    
at /rustc/ed61e7d7e242494fb7057f2657300d9e77bb4fcb/library\std\src\sys/backtrace.rs:174:18                                                                                                                                                                                                                                                                                        
6: __rustc::rust_begin_unwind                                                                                                                                                                                                                                                                                                                                                                         
at /rustc/ed61e7d7e242494fb7057f2657300d9e77bb4fcb/library\std\src/panicking.rs:698:5                                                                                                                                                                                                                                                                                             
7: core::panicking::panic_fmt                                                                                                                                                                                                                                                                                                                                                                         
at /rustc/ed61e7d7e242494fb7057f2657300d9e77bb4fcb/library\core\src/panicking.rs:75:14                                                                                                                                                                                                                                                                                            
8: godot_ffi::binding::single_threaded::BindingStorage::ensure_main_thread                                                                                                                                                                                                                                                                                                                            
at /home/.../.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/godot-ffi-0.4.3/src/binding/single_threaded.rs:152:17                                                                                                                                                                                                                                                     
9: godot_ffi::binding::single_threaded::BindingStorage::get_binding_unchecked                                                                                                                                                                                                                                                                                                                         
at /home/.../.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/godot-ffi-0.4.3/src/binding/single_threaded.rs:124:9                                                                                                                                                                                                                                                     
10: godot_ffi::binding::get_binding                                                                                                                                                                                                                                                                                                                                                                    
at /home/.../.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/godot-ffi-0.4.3/src/binding/mod.rs:267:5                                                                                                                                                                                                                                                                 
11: godot_ffi::binding::get_interface                                                                                                                                                                                                                                                                                                                                                                  
at /home/.../.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/godot-ffi-0.4.3/src/binding/mod.rs:116:6                                                                                                                                                                                                                                                                 
12: <godot_core::builtin::string::string_name::StringName as core::convert::From<&str>>::from::{{closure}}                                                                                                                                                                                                                                                                                             
at /home/.../.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/godot-core-0.4.3/src/builtin/string/string_name.rs:362:17                                                                                                                                                                                                                                                
13: godot_core::builtin::string::string_name::StringName::new_with_string_uninit                                                                                                                                                                                                                                                                                                                       
at /home/.../.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/godot-ffi-0.4.3/src/godot_ffi.rs:199:13                                                                                                                                                                                                                                                                  
14: <godot_core::builtin::string::string_name::StringName as core::convert::From<&str>>::from                                                                                                                                                                                                                                                                                                          
at /home/.../.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/godot-core-0.4.3/src/builtin/string/string_name.rs:361:13                                                                                                                                                                                                                                                
15: <T as core::convert::Into<U>>::into                                                                                                                                                                                                                                                                                                                                                                
at /rustc/ed61e7d7e242494fb7057f2657300d9e77bb4fcb\library\core\src\convert\mod.rs:778:9                                                                                                                                                                                                                                                                                         
16: <clipmap::clipmap::ClipmapMesh as godot_core::gen::classes::primitive_mesh::re_export::IPrimitiveMesh>::get_property_list                                                                                                                                                                                                                                                                          
at /mnt/e/clipmap/src/clipmap.rs:26:47                                                                                                                                                                                                                                                                                                                                           
17: <clipmap::clipmap::ClipmapMesh as godot_core::obj::traits::cap::GodotGetPropertyList>::__godot_get_property_list                                                                                                                                                                                                                                                                                   
at /mnt/e/clipmap/src/clipmap.rs:19:1                                                                                                                                                                                                                                                                                                                                            
18: godot_core::registry::callbacks::get_property_list                                                                                                                                                                                                                                                                                                                                                 
at /home/.../.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/godot-core-0.4.3/src/registry/callbacks.rs:326:25                                                                                                                                                                                                                                                        
19: <unknown>                                                                                                                                                                                                                                                                                                                                                                               
20: <unknown>                                                                                                                                                                                                                                                                                                                                                                               
21: <unknown>                                                                                                                                                                                                                                                                                                                                                                               
22: <unknown>                                                                                                                                                                                                                                                                                                                                                                               
23: <unknown>                                                                                                                                                                                                                                                                                                                                                                               
24: <unknown>                                                                                                                                                                                                                                                                                                                                                                               
25: <unknown>                                                                                                                                                                                                                                                                                                                                                                               
26: <unknown>                                                                                                                                                                                                                                                                                                                                                                               
27: <unknown>                                                                                                                                                                                                                                                                                                                                                                               
28: <unknown>                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                         
ERROR:                                                                                                                                                                                                                                                                                                                                                                                       
[panic /home/.../.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/godot-core-0.4.3/src/storage/mod.rs:38]                                                                                                                                                                                                                                                                          
Gd<T>::bind_mut() failed, already bound; T = clipmap::clipmap::ClipmapMesh.                                                                                                                                                                                                                                                                                                                   
Make sure to use `self.base_mut()` instead of `self.to_gd()` when possible.                                                                                                                                                                                                                                                                                                                 
Details: cannot borrow while accessible mutable borrow exists.                                                                                                                                                                                                                                                                                                                             
at: godot_core::private::set_gdext_hook::{{closure}} (/home/.../.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/godot-
core-0.4.3/src/private.rs:343)                                                                                                                                                                                                                          
fatal runtime error: failed to initiate panic, error 5, aborting

Relevant part of the code:

#[godot_api]
impl IPrimitiveMesh for ClipmapMesh {
  fn get_property_list(&mut self) -> Vec<PropertyInfo> {
    let mut res = Vec::new();
    res.push(PropertyInfo{
      variant_type: VariantType::INT,
      class_id: ClassId::none(),
      property_name: "clipmap/number_of_lods".into(),
      hint_info: PropertyHintInfo::none(),
      usage: PropertyUsageFlags::EDITOR | PropertyUsageFlags::STORAGE,
    });
    // TODO: add more properties here
    res
  }
}

I am cross-compiling a dll from under a WSL terminal using

$ cargo build --target=x86_64-pc-windows-gnu

Adding the experimental-threads crate feature fixes this (by removing the check, I presume); but it feels like such basic functionality shouldn't rely on an experimental crate feature.

[Bromeon]

Adding the experimental-threads crate feature fixes this (by removing the check, I presume); but it feels like such basic functionality shouldn't rely on an experimental crate feature.

It doesn't fix the problem, it just hides potential UB. Which is why the feature is still experimental 🙂

I agree that we should support this, but how can we do it without introducing unsoundness?

#[derive(GodotClass)]
#[class(init, base=PrimitiveMesh)]
struct MyClass {
    #[init(node = "...")]
    tree: Gd<Node>, // points to scene tree
}

#[godot_api]
impl IPrimitiveMesh for MyClass {
  fn get_property_list(&mut self) -> Vec<PropertyInfo> {
    do_something(&self.tree); // instant UB
  }
}

This is undefined behavior, because Godot doesn't allow accessing the active scene tree outside the main thread.
UB occurring in safe Rust code makes the latter unsound.

You don't need the scene tree, any Rust type that isn't Sync would be unsound to access like this.

[caphindsight]

Maybe I'm missing something, but does this make get_property_list essentially broken?
I don't do anything special to repro this condition, just add a MeshInstance3D node to a scene and give it my mesh type as an override. I don't mess with threads myself. I presume Godot does it for me

[caphindsight]

Also, I'm sure you're right that accessing the tree from a separate thread is UB, but surely you could access an individual node, without reading / writing to the structure of the tree itself? I have been doing this with success for a long time, from C++ and GDScript. Of course the usual considerations apply, i.e. you shouldn't concurrently read and write to the same variable of the node, because that would lead to concurrent memory access.

Godot even supports running callbacks on a separate threads for a node..

I suspect the problem here is the function signature...
My implementation of _get_property_list is safe to call concurrently, but it's still forced to use &mut self because that's the signature generated for me.

[Bromeon]

Yes, unfortunately there are a few such limitations, see parent issue I just linked. The solution is everything but trivial, we will need to establish a model to safely access shared state in such situations. The fact that Godot doesn't document any of this information, let alone expose it through GDExtension, doesn't make it easier.

This is a problem in all language bindings, btw -- it's just that godot-rust is the only that cares about thread safety. In others, you can happily write gamedev code which works most of the time, but then you might have the rare, hard-to-reproduce race conditions ruining your day. For people who prefer that sort of experience, we offer experimental-threads.

---

Also, I'm sure you're right that accessing the tree from a separate thread is UB, but surely you could access an individual node, without reading / writing to the structure of the tree itself? I have been doing this with success for a long time, from C++ and GDScript.

That's a very slippery slope. You don't know the implementation of each node method -- when you change a property, this can trigger a signal to notify others in the tree, set a dirty flag somewhere, invalidate a scene-local cache, etc. Godot explicitly documents this to be unsafe, and I don't need to mention that "it works in GDScript" isn't sufficient to mean there's no UB 😉

But like I said, it's not just about nodes. If we didn't check the caller thread, this would allow you to access any shared Rust data from a different thread, without said data implementing Sync. This is unsound.

[caphindsight]

I don't disagree about the slippery slope; but in practice, one thread is simply not enough for some games. If you iron out these aspects making sure crashes don't happen, it's arguably a better player experience than occasional FPS spikes from a single threaded bottleneck