Unique<T> for thread-safe usage of engine types#1524
Unique<T> for thread-safe usage of engine types#1524TitanNano wants to merge 4 commits intogodot-rust:masterfrom
Conversation
1e43f6c to
28c83fc
Compare
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1524 |
7a094fa to
967028a
Compare
There was a problem hiding this comment.
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?
967028a to
42b7a13
Compare
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.
42b7a13 to
d37d968
Compare
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.
d37d968 to
d0a4657
Compare
Yeah, docs weren't being rebuilt due to conflicts with
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
Breaking change, yes; restricting, I'm not sure. All user-defined types should be The blanked impl for That's why I currently have the
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. |
That's a reason why the trait should be sealed though, not private 🙂 It can have a private
It's possible to enforce such absence, but it will limit
Is 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.
You cannot return, but it's still possible to transport values outside through thread-safe means (e.g. |
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 byUnique<T>across threads.Values that are wrapped by
Uniquecan only be accessed via theUnique::applyorUnique::apply_gdfunctions. These functions accept aSend + Syncclosure to prevent any non-thread-safe values from getting passed into the wrapped types.Usage Example
Breaking Changes
This will very likely break code that relies on the
experimental-threadsfeature. The extent of the breakage needs to be assessed, and ideally a migration path can be offered for all safe use cases.To Dos: