Unique<T> for thread-safe usage of engine types

[TitanNano]

The Unique<T> type constrains the way non-thread-safe engine types can be used outside the main thread. With these constraints applied, we can safely send types that are wrapped by Unique<T> across threads.

Values that are wrapped by Unique can only be accessed via the Unique::apply or Unique::apply_gd functions. These functions accept a Send + Sync closure to prevent any non-thread-safe values from getting passed into the wrapped types.

Usage Example

std::thread::spawn(|| {
    let node: Unique<Gd<Node3D>> = Unique::new_alloc();
    let child: Unique<Gd<Node3D>> = Unique::new_alloc();
    
    node.apply_gd(move |node| {
         node.add_child(child);
    });

    node // A unique node can be returned by the thread back to the main thread.
});

Breaking Changes

This will very likely break code that relies on the experimental-threads feature. The extent of the breakage needs to be assessed, and ideally a migration path can be offered for all safe use cases.

• ToGodot has a new associated type Threads that has to be added to all manual implementations of the trait. Default values for associated types are still not stable.

To Dos:

• test the implementation with an actual project
  • It's currently possible to pass array and dictionary types to the engine by reference. This breaks the uniqueness constraints when storing such types in a thread-local variable.
• verify_unique_recursive needs more thorough testing.
• check older API version compatibility.

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1524

[Bromeon]

Thanks a lot for the effort, very interesting approach!

Several of the symbols (UniqueType, Array::to_unique, ...) do not appear in generated docs, which makes the PR a bit hard to introspect. Please make sure the full public API is visible, including required bounds 🙂

Since engine methods currently take &AnyArray/&AnyDictionary rather than impl AsArg<...>, the implicit conversions from Unique to array/dict arguments won't work. I'm not sure about making everything generic, as it increases complexity and compile time -- for now there could be an escape hook (explicit conversion) maybe? Although that might open it up for abuse 🤔

Element: ThreadSafeArgContext can be quite a restriction and breaking change, no? Especially after we just opened up Element to custom types for v0.5. The trait ThreadSafeArgContext not having a blanket impl is a problem -- it means that we have yet another trait that has to be implemented manually (yes, #[derive] can do it, but I also anticipate moving to builders and proc-macro less APIs one day, and making them more boilerplaty isn't great 😉). There's some prior art: I made a blanket-impl with AsArg<Variant> for impl ToGodot<Pass=ByValue> -- it would mean removing many explicit impls though (and not sure if that works out for all).

I see that apply() takes exclusive refs &mut T, is there a distinction to allow shared-ref access &T, or should we treat both the same?

[TitanNano]

Several of the symbols (UniqueType, Array::to_unique, ...) do not appear in generated docs, which makes the PR a bit hard to introspect. Please make sure the full public API is visible, including required bounds 🙂

Yeah, docs weren't being rebuilt due to conflicts with master which prevented CI from running.

UniqueType in particular, though, is supposed to be private as it seals the T of Unique to a curated selection. This is not absolutely necessary, as Unique cannot be constructed for arbitrary types, so we could also remove it.

Since engine methods currently take &AnyArray/&AnyDictionary rather than impl AsArg<...>, the implicit conversions from Unique to array/dict arguments won't work. [...]

This is actually an unsolved problem. Explicit conversion creates a gap in the thread safety guarantees. It's possible to smuggle in non-thread-safe values via thread locals or singletons and pass them to an API of a Unique<T>, which breaks the thread-safety guarantees. E.g. this but with an Array or Dictionary.

Element: ThreadSafeArgContext can be quite a restriction and breaking change, no? Especially after we just opened up Element to custom types for v0.5. [...]

Breaking change, yes; restricting, I'm not sure. All user-defined types should be Send + Sync so there shouldn't be an issue with implementing ThreadSafeArg for them.

The blanked impl for ThreadSafeArgContext should be impl<T: Send + Sync> ThreadSafeArgContext for T but the compiler has no guarantee that godot-ffi will not make Opaque send and sync in the future. 😓

That's why I currently have the ThreadSafeArg marker trait, but it still requires manual implementation outside the derive macro.

I see that apply() takes exclusive refs &mut T, is there a distinction to allow shared-ref access &T, or should we treat both the same?

I don't see much use for the apply function besides mutating the inner type. You can't return anything from the closure, so immutable access appears to be pointless so far.

[Bromeon]

UniqueType in particular, though, is supposed to be private as it seals the T of Unique to a curated selection.

That's a reason why the trait should be sealed though, not private 🙂 It can have a private sealed::Sealed supertrait, we do this in a few places. But it would still be good to know its implementors, as it's used in a public bound.

---

This is actually an unsolved problem. Explicit conversion creates a gap in the thread safety guarantees.

AsArg<T> provides implicit conversions and currently no public methods of its own, so it's effectively only usable to forward something to an engine method accepting AsArg<T>. This lack of API seems to be something that the thread-safety relies on, but it's not really guaranteed as of now (it might be useful one day to allow users have their own impl AsArg parameters).

It's possible to enforce such absence, but it will limit AsArg in a way that it wasn't originally designed for, and it feels a bit like mixing unrelated concerns (implicit conversions vs. thread-safety). But I do see the appeal to it. Maybe we should still evaluate multiple options before committing to one, though.

---

The blanked impl for ThreadSafeArgContext should be impl<T: Send + Sync> ThreadSafeArgContext for T but the compiler has no guarantee that godot-ffi will not make Opaque send and sync in the future. 😓

That's why I currently have the ThreadSafeArg marker trait, but it still requires manual implementation outside the derive macro.

Is Opaque the only obstacle here, or are there other coherence issues?

What about other blanket impls, like the one I suggested? It's still semi-manual, but at least most users defining their own types can deal with one trait less that they might ultimately not care about.

---

I don't see much use for the apply function besides mutating the inner type. You can't return anything from the closure, so immutable access appears to be pointless so far.

You cannot return, but it's still possible to transport values outside through thread-safe means (e.g. Mutex, Arc, etc.). But I'm OK with only supporting &mut, if there's another need, we can always extend it.

[TitanNano]

[...] But it would still be good to know its implementors, as it's used in a public bound.

Fair point, I made it public and sealed now.

[...] But I do see the appeal to it. Maybe we should still evaluate multiple options before committing to one, though.

Piggybacking off AsArg currently solves two problems:

1. the Send + Sync closure of Unique::apply is not reliable and only serves as guardrails to nudge users into the correct way of using the type. To have true thread safety, we need to perform a runtime check because the valid usage of many engine types depends on which thread they are being used on. I think we could do this runtime check independently of AsArg, it's just convenient to do it during the conversion.
2. We need to support two sets of function signatures for all engine APIs. One unrestricted one for the main thread and one restricted one for other threads. Doing this via generic argument types or impl Trait seems like the most ergonomic and maintainable solution. If this gets decoupled from AsArg we still would require a generic argument trait to support both "normal" argument types and thread-safe argument types.

Is Opaque the only obstacle here, or are there other coherence issues?

What about other blanket impls, like the one I suggested? It's still semi-manual, but at least most users defining their own types can deal with one trait less that they might ultimately not care about.

It's not Opaque per se. We want to differentiate between Send and !Send types at runtime, and this is a real pain because user-defined !Send types in stable Rust are always implicit. The compiler does not trust that a type will remain !Send so we would need a negative trait bound to specify that the blanket implementation only applies to something like !ExplicitlyNonSend. Even when I move ThreadSafeArgContext into godot-ffi the error just moves up the chain, and the compiler complains about PhantomData<*const u8> possibly being Send in a future release.

Using T: AsArg<Variant> wouldn't be able to select only Send types. We want a blanket implementation for all thread-safe types and a specific one for the others.

We had the same issue when dealing with the signal arguments for the SignalFuture 😞. I keep thinking about this problem, but I haven't found a better solution so far.

[TitanNano]

I have now added an additional associated type to ToGodot which resolves our trait coherence issue. It is still a breaking change, as it requires this associated type to be added to any manual implementation of ToGodot.

Additionally, I have moved the runtime thread-safety validation into codegen so it is decoupled from AsArg. This is more involved than the previous approach but also covers much more of the API surface. Unfortunately it also breaks all non-main thread signal emission since all types are erased when calling Object::emit_signal. I think I will have to add an internal version of Object::emit_signal that skips the thread-safety validation so we still can correctly emit typed signals.

[Bromeon]

Thanks for keeping this up-to-date! There's probably still quite a few things to be looked at holistically, but I commented again on some parts to get a more detailed understanding on the approach 🙂

[Bromeon]

We can't have codegen-full in default config, it takes too long to compile. But I just added the --full flag for this 🙂

Also experimental-threads is rather discouraged -- discrepancies in feature flags between individual compiles (itest, clippy, test etc) will require re-compilations. (I think this might already be a problem today...)

[TitanNano]

Yes, I fully agree. This is all just so it's easier to test this PR locally. I will remove all changes here before moving things out of draft status.

[Bromeon]

This is quite restrictive, is this method still useful in practice?

• Gd<Node> etc isn't necessarily owning, pointing to nodes in the scene tree is harmless
• No arrays/dictionaries excludes a ton of use cases

At the same time it's also not airtight:

• You don't list Variant which can contain anything else
• Any fields that aren't #[var] are invisible
• Gd strong refs can be trivially worked around with InstanceId weak refs -- although this can be OK if from_instance_id is sound
• The recursion hits a stack overflow when you have cyclic references

I'm not saying it needs to be perfect -- for threading we definitely have to make compromises, and best-effort is better than nothing. But we should probably state a clear goal here, and then depending on that decide how far we want to go.

It's also worth noting that such a recursive check can be extremely expensive at runtime.

[TitanNano]

This is quite restrictive, is this method still useful in practice?

This is something I'm currently evaluating inside my own project. My theoretical use case here is that this should work well for engine built-in resources like materials, textures, animations, and so on.

You don't list Variant which can contain anything else

Variant types are being inspected during conversion.

Any fields that aren't #[var] are invisible

Yes, and that's why only engine ref-counted classes without a script are allowed. It's even more restrictive than you thought. 😁 I see that this detail is not yet covered by the description.

Gd strong refs can be trivially worked around with InstanceId weak refs -- although this can be OK if from_instance_id is sound

Yes, but I don't think engine classes store InstanceIDs; at least I haven't come across one yet. So it should be covered by the previous point. try_from_ref_counted is now also restricted to the main thread as well.

The recursion hits a stack overflow when you have cyclic references

Good point. I should probably guard against that. So far I'm just trusting that engine classes don't have cycling references.

[Bromeon]

Inherits<RefCounted> implies Inherits<Object>. In fact, the latter is always implied (but may sometimes be necessary for technical reasons).

[TitanNano]

Yes, looks like the bound is superfluous.

[Bromeon]

It seems like this or similar checks happen more than once. Probably makes sense to add a method on VariantType.

Note that I'm already going to add a is_pod/needs_ffi_destructor function for anything that doesn't/does need a FFI destruction (i.e. isn't Copy). This covers everything here except STRING/STRING_NAME.

Also strange that two strings are on the list, but NODE_PATH isn't.

[TitanNano]

Also strange that two strings are on the list, but NODE_PATH isn't.

We currently don't treat NodePath as thread-safe, so I omitted it here. I think it's also just a GString internally, but keep it the way it is for now.

[Bromeon]

Conceptually, do we also need to cover the other side -- not just arguments, but return values from Godot?

[TitanNano]

So far I'm thinking that we don't have to do that. There is no clue from the engine as to what the thread-safety properties of a return value are. So at the moment return values are just accepted, and we trust the engine that the return value is ok in the current context. Once you try to pass the return value back to the engine, thread safety checks will be applied. I think this keeps the restrictions somewhat balanced. Restricting the read and write access to shared references is out of scope of this PR, but once we get to that, you essentially can end up with a value that you can't do anything with. It would still be possible to run expensive checks to verify that the value is actually unique or read-only though. If we outright block return values, that wouldn't be possible.

[Bromeon]

Thanks for elaboration. I think this might deserve a short section -- even if it's just reflecting the status quo, and doesn't imply that we'll never have to do that in the future.

[Bromeon]

Can we reuse the existing Sealed trait -- if the purpose is only that users cannot implement it outside?

[TitanNano]

Correct me if I'm wrong, but we have a blanked impl of ThreadSafeSealed for any T that is also Send. This means ThreadSafeArgContext can only be implemented by our own types and by any type that is covered by the blanket impl. If we switch from ThreadSafeSealed to Sealed we would make a lot of user types Sealed which is not what we want.

[Bromeon]

Ah, good point. Maybe add a quick comment then:

Suggested change

	pub(crate) trait ThreadSafeSealed {}
	// We can't use private::Sealed due to blanket impl for all T: Send.
	pub(crate) trait ThreadSafeSealed {}

[ValorZard]

This is a drive by comment, but can you downcast a node stored in a unique?

like, can you cast a Unique<Node> to Unique<MyCustomClass>

[TitanNano]

This is a drive by comment, but can you downcast a node stored in a unique?

like, can you cast a Unique<Node> to Unique<MyCustomClass>

At the moment that is not supported. Do you see a use case for downcasting here? Unique<Gd<NodeType>> can only be obtained by creating a new node. My assumption so far was that users would simply create the node they need, modify it, and then pass it to the main thread.