Deal with cross-thread access to user-defined GodotClass instances about gdext

[Bromeon]

Re-labeling this as feature for general discussion about multithreading support.

[Bromeon]

To elaborate on my original plans a bit and keep the discussion public:

As mentioned in #212 (comment), some operations are currently unsound, for example:

• when storing a Gd<T> equivalent reference on GDScript side
• through the scene tree
• Gd::upcast()
• Gd::from_instance_id()
• Gd::from_variant()
• ...

So individual unsound access points like from_instance_id are mostly symptoms, the core problem is that Gd is not generally safe to construct on other threads.

The semantics here are actually more limiting than Rust's own:

in general, Gd<T> can only be safely constructed and used on the main thread.

This is very important, as Gd<T> is not an isolated instance (as it were in Rust). Through the engine, there can be lots of hidden links to other objects (scene tree, connected signals, base classes...). In other words, Gd<T> potentially has side effects transcending thread boundaries. This concept is not expressible using Rust standard traits; Rust generally assumes that if you have ownership of something, it's unique; and if it's shared across threads, it's Sync.

---

So, what can we do? My idea is still more or less the same from the original post.

1. When threads feature is off: we strictly forbid construction of Gd<T> in other threads.

   • Validated through thread ID and panics.

2. When threads feature is on: we need to differentiate which types are "thread-safe" according to Godot.

   • This not only concerns classes, but also builtins, especially the composite ones (Variant, Array, PackedArray, Dictionary, Callable).
   • Servers and Godot singletons should be constructible from any thread, and shareable across thread boundaries.
   • Resources can be loaded on one thread and sent to another (Send) and read concurrently from different threads (Sync), but not modified concurrently.

3. Once we have a model, we can turn it into the type system:

   • Traits for properties that can be statically verified (like "this type is safe to construct on non-main threads").
   • Runtime checks and panics for properties that cannot be verified at compile time.
   • Escape hatches via unsafe for cases that need to be supported but don't fit into the type system. Ideally, this surface should be small.

[lilizoey]

i still kinda feel like it'd be better to just have two separate Gds. One that is only meant to be used on the main thread and one that is able to be used concurrently. maybe more idk we can make it as granular as we like there.

mainly because i really think that making Gd thread safe will be a very invasive procedure and likely hurt the ergonomics a fair bit. several methods may need to be made unsafe, etc. and having it be a feature that changes the behavior of Gd would mean it's either all or nothing.

im addition, how do we document that? a separate type is easy to document, but documenting all the changes from this feature seems like it'd be very confusing and weird. i imagine there is a way to do it in the docs but i cant imagine it'd look nice.

in addition, for the other core classes we can probably just make them Send/Sync if they are. all the value-types are already, since they're just implemented in rust. and for Dictionary and Array we can probably make a thread safe version of each.

anyway let's say we split Gd into Gd and GdSync (temp names). we could just make Gd check that it's always on the main thread when created, and have it not implement Send or Sync ever. (or maybe we can allow it in very specific cases, like perhaps the singletons? that might also just be confusing though).

We could also potentially use object metadata to store what thread affinity an object should have, if we want to allow Gd to be used on more than just the main thread. But that would also mean we'd need to ensure the user doesn't override the metadata somehow. unless, can we set instance bindings on arbitrary objects? if so then we might be able to use that instead.

GdSync would then have a more restricted api. likely more unsafe methods, and probably some way to track whether it should be Send or Sync. i dont know if we can do that by just implementing Send/Sync directly on the classes. at least for Sync that would allow:

let foo = Gd::<Node>::from_instance_id(id);
let node: &Node = *foo;
thread::spawn(|| node.get_child())

maybe it is fine to allow this for classes that actually are Sync (Node probably isn't).

Also there's a distinction i just thought of:

• some classes can always be safely sent/shared between threads
• some classes may be safely sent/shared between threads as long as the object is actually that class and not a subclass

generally the first one of these will be true if the entire subtree of the class hierarchy is also send/sync. this would technically include user-classes but we can perform a check in user classes to make sure they're not created/shared where they shouldn't be.

the second one may be true in some cases, for instance it's true for Object i believe.

we may also have some cases where some individual methods are safe to call from other threads. i suspect this may be a bit too hard to model properly though, may be feasible through an unsafe escape hatch. If we do add a RawGd we can use that as the escape hatch, RawGd would be Send and Sync, but most methods on it are unsafe.

There are also a couple other possibilities for dedicated smart pointers we could make if we wanted to. One I've thought of would be analogous to the Unique typestate in gdnative. Essentially a GdBox, which would be probably the most permissive of all the types. However it can only be safely constructed fresh, and unsafely converted from another Gd type. But also can be safely consumed and turned into other gd types. It's also freed on drop. We might actually be able to safely convert RefCounted objects into this type, since we can check the refcount to see that we're the only one owning it. This would likely be very useful in dealing with Resource in that case.

[Bromeon]

Thanks a lot for the great feedback!

Yep, it might be better to have its dedicated type GdSync (or Gds, SyncGd, Agd or whatever -- I'll use Gds for brevity here) if the APIs differ too much.

Proliferation of public types with closely related meaning, such as Gd, GdSync, RawGd, GdBox, ... goes a bit against our declared Simplicity principle. As a user wanting to develop a game, I'm usually not interested in selecting out of 4+ smart pointer types and memorize all the subtle differences and use cases, if GDScript or C# just allows me to get stuff done. But probably we will see if there's a real need for those once things get more concrete 🙂

Additionally, there are a lot of things we cannot check at compile time either way, by the nature how Godot operates.

You mentioned a good one: if Gds<Base> can point to Gds<Derived>, we need to ensure that all derivates follow the same constraints. Which creates non-locality: adding a new class deriving from Base may break existing usages of Gd<Base> (at runtime). Or it must be guaranteed that polymorphism (up/downcasting) is only possible when derived classes implement the same traits.

Alternatively, we could outlaw polymorphism on Gds and enforce that Gds<T> always hold exactly T. This might be an acceptable trade-off, since synced pointers are usually short-lived (e.g. load resource in background thread, do a parallel computation with a result, etc).