Skip to content

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

Draft
TitanNano wants to merge 4 commits intogodot-rust:masterfrom
TitanNano:jovan/thread_unique
Draft

Unique<T> for thread-safe usage of engine types#1524
TitanNano wants to merge 4 commits intogodot-rust:masterfrom
TitanNano:jovan/thread_unique

Conversation

@TitanNano
Copy link
Copy Markdown
Contributor

@TitanNano TitanNano commented Mar 10, 2026

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.

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.

@TitanNano TitanNano self-assigned this Mar 10, 2026
@TitanNano TitanNano added feature Adds functionality to the library breaking-change Requires SemVer bump c: threads Related to multithreading in Godot labels Mar 10, 2026
@TitanNano TitanNano force-pushed the jovan/thread_unique branch 2 times, most recently from 1e43f6c to 28c83fc Compare March 10, 2026 22:37
@GodotRust
Copy link
Copy Markdown

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

@Bromeon Bromeon added this to the 0.6 milestone Mar 10, 2026
@TitanNano TitanNano force-pushed the jovan/thread_unique branch 2 times, most recently from 7a094fa to 967028a Compare March 15, 2026 10:46
Copy link
Copy Markdown
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 TitanNano force-pushed the jovan/thread_unique branch from 967028a to 42b7a13 Compare April 5, 2026 20:06
All reference based Godot types can be created inside a Unique struct. The only way to interact with the godot class is with Send + Sync types.

Unique::map and Unique::apply only allow Send and Sync closures and and trying to sneak in Gd<T>s via thread_locals will result in a runtime panic.
@TitanNano TitanNano force-pushed the jovan/thread_unique branch from 42b7a13 to d37d968 Compare April 5, 2026 20:16
When experimental-threads is on we can only pass &Gd<T> to the engine on the main-thread. Other threads will panic.
Some tests to check if it works as intended. More real world use-cases would help.
@TitanNano TitanNano force-pushed the jovan/thread_unique branch from d37d968 to d0a4657 Compare April 5, 2026 20:19
@TitanNano
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Member

Bromeon commented Apr 5, 2026

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change Requires SemVer bump c: threads Related to multithreading in Godot feature Adds functionality to the library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants