Safely integrate Godot cross-thread calls (e.g. `AudioStreamPlayback::mix`)

[djcsdy]

This is a rough proposal for how to safely implement functions that currently require experimental-threads. It's not completely fleshed out but I wanted to see if there's any interest before I expand on it further.

If you're not taking proposals on how to solve this problem at the moment, or you don't find this proposal useful, I won't be offended if you close the issue.

Take AudioStreamPlayback as an example:

unsafe fn mix(&mut self, buffer: * mut AudioFrame, rate_scale: f32, frames: i32,) -> i32;

Godot calls mix from a non-main thread, which means that access to self in this function is unsound. (Without experimental-threads this condition is checked at runtime by godot-rust, which will panic before mix is even called).

My proposal is to remove the self parameter from functions that are called from a non-main thread, and replace it with a user-defined state parameter, which must impl Clone + Send + 'static and can therefore be passed between threads safely. For example:

trait IAudioStreamPlayback<AudioState> /* : ... */
where
    AudioState: Clone + Send + 'static
{
    /* ... */
    unsafe fn mix(state: AudioState, buffer: * mut AudioFrame, rate_scale: f32, frames: i32) -> i32;
    /* ... */
}

A sensible choice for AudioState might be for example Arc<RwLock<S>>, where S is some arbitrary struct. The AudioState could then be safely read and mutated from multiple threads. But the idea is that the actual type of AudioState should be left up to the programmer as long as it implements Clone + Send + 'static.

The state object should be attached to the class struct, for example:

#[derive(GodotClass)]
#[class(base=AudioStreamPlayback)]
struct Example {
    #[audio_state]
    state: Arc<RwLock<ExampleState>>,
}

#[godot_api]
impl IAudioStreamPlayback<Arc<RwLock<ExampleState>> for Example {
    unsafe fn mix(state: Arc<RwLock<ExampleState>>, buffer: * mut AudioFrame, rate_scale: f32, frames: i32) -> i32 {
        / * ... */
        frames
    }
}

struct ExampleState {
    position: usize,
}

godot-rust would then have some glue code that takes care of cloning state and passing it through to IAudioStreamPlayback::mix when mix is called from Godot.

It would be illegal to mark a field #[export] if it is also marked #[audio_state].

This approach is roughly what I'm using in my own project, although I of course have to implement the glue manually each time I extend AudioStreamPlayback.

I think this general approach should be logically extensible to other cases where Godot calls functions from non-main threads.

If you're interested in something like this approach I would be interested in having a go at implementing it.

[Bromeon]

Thanks for raising this! Input on thread safety is definitely appreciated. 👍
This particular example is pretty much a duplicate of #938, but maybe your approach can be generalized.

A few things we need to clarify:

1. Like in #1129 and #1130, Godot has zero information about thread-safety of its APIs. So conservatively, we need to assume everything is not thread-safe, and manually whitelist classes that can are designed to be invoked cross-thread. It looks like this even goes down on the method level, not just class level?

2. If we go with generics, Arc<RwLock<ExampleState>> should probably be an associated type rather than a generic parameter of the trait. After all, we're not going to implement the trait multiple times with different state types for the same class.

3. Is genericity needed at all? I'd like to avoid premature abstraction if that would allow us to build more specialized and easier-to-understand APIs 🤔 Your bounds Clone + Send + 'static indicate that one could technically use simple scalar types such as i32 without the need for Arc. I wonder how often this occurs in practice and if saving the atomic ref-counting is worthwhile (given that FFI and other Godot operations likely outweigh the perf impact). On the other hand, there might be a usability impact, Arc<Mutex<...>> is a bit unwieldy to use compared to plain Copy types.

4. Extracting a state of self to pass to non-thread-safe APIs seems like a good idea, but would it be enough? mix() can then not access anything else of self and of the class' base (base() and base_mut()) -- which is probably OK?

There is an entire rabbit hole of thread safety discussed in #18. No need to read the whole thread, but there are some ideas towards the end that we might end up integrating, such as UniqueGd. I don't know to which extent those would be applicable here, but I thought it's good to be aware.

Planning-wise, we're currently finalizing v0.3, and this being a bigger endeavour means it would like aim for v0.4, given it probably introduces breaking changes. But I think it should be fine, it would likely also take some time to settle on an approach, and maybe we can start with v0.4 sooner than with past minor releases.

[djcsdy]

• Like in #1129 and #1130, Godot has zero information about thread-safety of its APIs. So conservatively, we need to assume everything is not thread-safe, and manually whitelist classes that can are designed to be invoked cross-thread. It looks like this even goes down on the method level, not just class level?

I must be completely honest here: I have exactly 11 days of experience with Godot so far. But based on what little I've seen, I think it's safe to say that it's definitely necessary to consider thread safety down to the method level, at least for some classes.

Just considering AudioStreamPlayback, it is clearly the case that some functions are always called from the main thread and some never called from the main thread. We know that mix is called from an audio thread. But all of the others are always called from the main thread. I know this because in the process of figuring things out I've run code with all of them implemented with experimental-threads turned off and none of the others panicked.

For some of the subclasses of AudioStreamPlayback there are other functions that are also called from the audio thread (e.g. AudioStreamPlaybackResampled::mix_resampled) but they otherwise follow the same pattern.

My working assumption at the moment is that if a function can be overridden in GDScript, then it is always called from the main thread. But if it can only be overridden in native code then there is a good chance it is called from some other thread.

This makes sense because GDScript would surely blow up horribly if called from a non-main thread. For that to work it'd be necessary to do something like Web Workers where an individual GDScript runs in its own isolated container, and I'm not aware of anything like that in Godot.

I suppose it's possible that there's some function that Godot calls in the main thread if its overridden in GDScript but calls in some other thread otherwise, but that seems unlikely.

• If we go with generics, Arc<RwLock<ExampleState>> should probably be an associated type rather than a generic parameter of the trait. After all, we're not going to implement the trait multiple times with different state types for the same class.

Yeah, agreed. I was getting my thoughts down quickly and made a silly choice here.

• Is genericity needed at all? I'd like to avoid premature abstraction if that would allow us to build more specialized and easier-to-understand APIs 🤔 Your bounds Clone + Send + 'static indicate that one could technically use simple scalar types such as i32 without the need for Arc. I wonder how often this occurs in practice and if saving the atomic ref-counting is worthwhile (given that FFI and other Godot operations likely outweigh the perf impact). On the other hand, there might be a usability impact, Arc<Mutex<...>> is a bit unwieldy to use compared to plain Copy types.

Yeah, I weighed up the same debate in my own mind. There are good arguments on both sides to be sure.

There are a couple of use cases I can think of for abstracting the type.

Firstly, what if having one lock around the whole state object doesn't make sense for a particular use case? You might want to do something like:

#[derive(Clone)]
struct State {
    pub messages: std::sync::mpsc::Receiver<Message>,
    pub a: RwLock<Something>,
    pub b: RwLock<SomethingElse>,
}

Requiring the above to be wrapped in a Mutex wouldn't be the end of the world but it doesn't make a lot of sense.

A middle ground might be to say that the State is always an Arc<_>.

But then the second use case is if the threaded function doesn't actually need to modify any state. This is a weak argument because I can't give a specific example, but I can imagine a case where a threaded function just needs some data that is initialized once per class instance, in which case a plain struct that implements Clone or even a scalar type might be sufficient for the 'state'.

On balance I am sure I would be happy with the middle ground of Arc<_>, and I don't suppose I would be terribly upset if it ended up being even more concrete than that. It's very probable that in the process of attempting to implement a solution it becomes obvious that one choice is better than the other.

• Extracting a state of self to pass to non-thread-safe APIs seems like a good idea, but would it be enough? mix() can then not access anything else of self and of the class' base (base() and base_mut()) -- which is probably OK?

Yeah... this is where my proposal is least fleshed out. I think it'd be necessary to experiment and implement some ideas to see if this works or not.

Speaking specifically about AudioStreamPlayback::mix, this approach is sufficient. I did notice after posting this proposal that in my own project I have called AudioServer::singleton().get_mix_rate() from mix, but that is just carelessness on my part. It would have been straightforward to call that function in start instead and store the information in the state object. In principle if calling AudioServer::singleton().get_mix_rate() is not thread-safe then godot-rust could have caught that mistake at runtime, except that I had to disable checks by enabling experimental-threads in order to implement mix at all. Edit: I see from #18 that calling functions on Godot's built-in singletons is considered thread-safe.

Apart from that, there's no sensible reason to call any Godot API from mix. It performs a pretty well-isolated task. There's no good reason to query the overall state of the game from inside sound code, and certainly no reason to attempt to modify it. If there is some information that mix needs from the outside world, there's plenty of opportunity to pass that in via the state object.

I really hope it is generally the case that any functions Godot calls from a non-main thread are going to be well-isolated in this way because if not that is going to be a horrendous can of worms. I am hoping for something approaching a one-way data flow type of solution (my proposal does not achieve that on its own, but it facilitates that approach). Whether that's feasible absolutely depends the design of bits of Godot that I haven't dug into yet.

I will have a look through the other proposals and think on this further.

Thanks!

[TitanNano]

Godot calls mix from a non-main thread, which means that access to self in this function is unsound. (Without experimental-threads this condition is checked at runtime by godot-rust, which will panic before mix is even called).

Just to clarify, this is not quite true. All implementations, with and without the experimental-threads feature, GdCell uses a Mutex to synchronize access to self. With the feature enabled we just also switch to atomic reference counting inside the InstanceStorage.

[djcsdy]

@TitanNano Thank you! I wasn't aware of the Mutex in GdCell but that does explain some things. I also see some other differences in bindings when experimental-threads is enabled, other than the absence of the check I was referring to.

I like to state "obvious" assumptions like this precisely because if they're incorrect stating them gives people a chance to correct my understanding :-).

[TitanNano]

So the change that is proposed here could be an optimization, but is not strictly necessary to make such classes usable. As far as I know, all rust code should be thread-safe and sound when then experimental-threads feature is enabled.

[Bromeon]

As far as I know, all rust code should be thread-safe and sound when then experimental-threads feature is enabled.

This is explicitly not the case, I even renamed the feature from threads to experimental-threads for this reason 😉
The API docs state:

The safety aspects are not ironed out yet; there is a high risk of unsoundness at the moment. As this evolves, it is very likely that the API becomes stricter.

[TitanNano]

@Bromeon hmm yea want I was thinking of is calling into rust code. There is a risk of unsoundness, but this relates to the interaction with the engine types. Godot classes that have been defined in rust should be safe to be accessed from multiple threads at the same time.