Mark GString as Send

[radiantgurl]

Since String is actually thread-safe (even though not listed, the cow implementation of String uses an atomic usize)

https://github.com/godotengine/godot/blob/master/core/templates/safe_refcount.h
https://github.com/godotengine/godot/blob/c81fd6c51233a727da528cf7f74137d56b5d6efe/core/templates/cowdata.h#L79
and finally,
https://github.com/godotengine/godot/blob/c81fd6c51233a727da528cf7f74137d56b5d6efe/core/string/ustring.h#L267

With a simple read and write test, i've verified and made sure that no clang data race warnings are emitted when using this. (engine built with ThreadSanitizer)

[Yarwin]

I checked and it seems right!

How should we test/guarantee it in itest 🤔? CC: @Bromeon

[Bromeon]

Thanks! Related Discord thread.

@radiantgurl Can you post the test code, so this is reproducible?

---

@Yarwin First, we should double-check with Godot devs if this is really thread-safe -- such tests are probabilistic after all. I find it a bit curious that such a central type as Godot's String isn't mentioned on Thread-safe APIs, when others like Array are.

---

As for #[itest] setup, we could write a function that clones an instance to multiple threads (e.g. 4), and then run each in a busy loop to execute random operations (e.g. 10k times), like:

• Clone + Drop
• Conversions to variant
• Conversions to StringName
• Some GString API calls

This could even be made generic, so that we can reuse it for other types (closures for specific operations).

To verify that this approach works, we should test it for some known-thread-unsafe type, like Gd<Node>, and see if it runs into UB/segfaults/race conditions. I'm not sure if thread-sanitizer is needed, maybe we can trigger problems without it.

[radiantgurl]

Can you post the test code?

use std::hint::black_box;

use godot::classes::Engine;
use godot::prelude::*;

struct Lib;

#[gdextension]
unsafe impl ExtensionLibrary for Lib {
    fn on_level_init(level: InitLevel) {
        if level == InitLevel::Scene {
            let obj = GString::from("testinggggggggagaggagg");
            let obj_ref1: &'static GString = unsafe {&*(&raw const obj)};
            let obj_ref2: &'static GString = obj_ref1;

            let thr1 = std::thread::spawn(move || {
                // Simulate some work with the GString
                godot_print!("thread1 started");
                for iter in 0..2000000 {
                    let mut gstring = black_box(obj_ref1.erase(0..1));
                    black_box(&mut gstring);
                    if iter % 100000 == 0 {
                        godot_print!("GString iter {iter} in thread1: {}", gstring);
                    }
                }
            });
            let thr2 = std::thread::spawn(move || {
                // Simulate some work with the GString
                godot_print!("thread2 started");
                for iter in 0..2000000 {
                    let mut gstring = black_box(obj_ref2.clone());
                    black_box(&mut gstring);
                    if iter % 100000 == 0 {
                        godot_print!("GString iter {iter} in thread2: {}", gstring);
                    }
                }
            });
            thr1.join().unwrap();
            thr2.join().unwrap();
            godot_print!("GString thread safety test completed.");
        }
    }
}

First, we should double-check with Godot devs if this is really thread-safe

I've been a contributor on godot for 2 yrs and i've worked on thread safety before, im 100% sure strings really are thread-safe.

[GodotRust]

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

[Bromeon]

I've been a contributor on godot for 2 yrs and i've worked on thread safety before, im 100% sure strings really are thread-safe.

I checked with some developers working on GDExtension, and the consensus was:

Important

Godot's String type is thread-safe as long as you create a copy of it.

You mentioned yourself that String uses copy-on-write. This implies that there is a "write" part, localized to one thread. If you now shared a reference of one string instance to another thread, you can run into a concurrent write and read. Not every individual mutation is guarded by a lock, there is just an initial check _copy_on_write(), which copies if refcount > 1. After that, mutable pointers are handed out.

A Rust type isn't just thread-safe or not. Rust differentiates between sharing values, and sharing references to them.

TLDR: we can add Send, but we cannot add Sync.

[radiantgurl]

Not every individual mutation is guarded by a lock, there is just an initial check _copy_on_write(), which copies if refcount > 1. After that, mutable pointers are handed out.

Guess i was wrong

[Bromeon]

Thanks a lot!