Unexpected "attempted to access binding from different thread than main thread" (regression?)

[bluenote10]

The following code used to run fine in older versions of gdext, but when switching to a more recent commit it starts panicking with:

thread '<unnamed>' panicked at /home/fabian/.cargo/git/checkouts/gdext-76630c89719e160c/a0d5799/godot-ffi/src/binding/single_threaded.rs:159:13:
assertion `left == right` failed: attempted to access binding from different thread than main thread; this is UB - use the "experimental-threads" feature.
  left: ThreadId(1)
 right: ThreadId(2)

The example is basically the "hello world" of a custom audio stream:

use godot::classes::native::AudioFrame;
use godot::classes::{AudioServer, AudioStreamPlayback, IAudioStream, IAudioStreamPlayback};
use godot::prelude::*;

#[derive(GodotClass)]
#[class(base=Node)]
pub struct Demo {
    audio_player: Gd<AudioStreamPlayer>,
}

#[godot_api]
impl INode for Demo {
    fn init(base: Base<Self::Base>) -> Self {
        println!("Demo::init");

        let mut audio_player = AudioStreamPlayer::new_alloc();
        audio_player.set_stream(Gd::<CustomAudioStream>::from_init_fn(|_| {
            CustomAudioStream::new()
        }));
        base.to_gd().add_child(audio_player.clone());

        Self { audio_player }
    }

    fn ready(&mut self) {
        self.audio_player.play();
    }
}

// CustomAudioStream

#[derive(GodotClass)]
#[class(base=AudioStream, no_init)]
pub struct CustomAudioStream {}

#[godot_api]
impl IAudioStream for CustomAudioStream {
    fn instantiate_playback(&self) -> Option<Gd<AudioStreamPlayback>> {
        println!("instantiate_playback");
        Some(
            Gd::<CustomAudioStreamPlayback>::from_init_fn(|_base| {
                CustomAudioStreamPlayback::new(Sequencer {
                    sample_rate: AudioServer::singleton().get_mix_rate(),
                    sample_index: 0,
                })
            })
            .upcast(),
        )
    }
}

impl CustomAudioStream {
    pub fn new() -> Self {
        Self {}
    }
}

// CustomAudioStreamPlayback

#[derive(GodotClass)]
#[class(base=AudioStreamPlayback, no_init)]
pub struct CustomAudioStreamPlayback {
    sequencer: Sequencer,
}

#[godot_api]
impl IAudioStreamPlayback for CustomAudioStreamPlayback {
    unsafe fn mix(
        &mut self,
        buffer: *mut AudioFrame,
        _rate_scale: f32,
        num_requested_frames: i32,
    ) -> i32 {
        self.sequencer.render_audio(num_requested_frames, buffer);
        num_requested_frames
    }

    fn start(&mut self, _from_pos: f64) {}
    fn stop(&mut self) {}
    fn is_playing(&self) -> bool {
        true
    }
}

impl CustomAudioStreamPlayback {
    fn new(sequencer: Sequencer) -> Self {
        Self { sequencer }
    }
}

// Sequencer

pub struct Sequencer {
    sample_rate: f32,
    sample_index: usize,
}

impl Sequencer {
    fn render_audio(&mut self, num_requested_frames: i32, buffer: *mut AudioFrame) {
        const FREQUENCY: f32 = 440.0;
        for i in 0..num_requested_frames {
            let phase = 2.0 * std::f32::consts::PI * FREQUENCY * (self.sample_index as f32)
                / self.sample_rate;
            let sample = 0.5 * phase.sin();
            unsafe {
                *buffer.offset(i as isize) = AudioFrame {
                    left: sample,
                    right: sample,
                };
            }
            self.sample_index += 1;
        }
    }
}

The full traceback is as follows, but not very insightful:

Full output

``` Demo::init instantiate_playback thread '' panicked at /home/fabian/.cargo/git/checkouts/gdext-76630c89719e160c/a0d5799/godot-ffi/src/binding/single_threaded.rs:159:13: assertion `left == right` failed: attempted to access binding from different thread than main thread; this is UB - use the "experimental-threads" feature. left: ThreadId(1) right: ThreadId(2) note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace thread '' panicked at core/src/panicking.rs:221:5: panic in a function that cannot unwind stack backtrace: 0: 0x7f0c3d48345a - std::backtrace_rs::backtrace::libunwind::trace::h99efb0985cae5d78 at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/../../backtrace/src/backtrace/libunwind.rs:116:5 1: 0x7f0c3d48345a - std::backtrace_rs::backtrace::trace_unsynchronized::he2c1aa63b3f7fad8 at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5 2: 0x7f0c3d48345a - std::sys::backtrace::_print_fmt::h8a221d40f5e0f88b at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/sys/backtrace.rs:66:9 3: 0x7f0c3d48345a - ::fmt::h304520fd6a30aa07 at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/sys/backtrace.rs:39:26 4: 0x7f0c3d4a45db - core::fmt::rt::Argument::fmt::h5da9c218ec984eaf at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/fmt/rt.rs:177:76 5: 0x7f0c3d4a45db - core::fmt::write::hf5713710ce10ff22 at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/fmt/mod.rs:1178:21 6: 0x7f0c3d481363 - std::io::Write::write_fmt::hda708db57927dacf at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/io/mod.rs:1823:15 7: 0x7f0c3d484982 - std::sys::backtrace::BacktraceLock::print::hbcdbec4d97c91528 at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/sys/backtrace.rs:42:9 8: 0x7f0c3d484982 - std::panicking::default_hook::{{closure}}::he1ad87607d0c11c5 at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/panicking.rs:266:22 9: 0x7f0c3d4845ee - std::panicking::default_hook::h81c8cd2e7c59ee33 at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/panicking.rs:293:9 10: 0x7f0c3d4851b2 - as core::ops::function::Fn>::call::h375ef7f99b271d16 at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/alloc/src/boxed.rs:2245:9 11: 0x7f0c3d4851b2 - std::panicking::rust_panic_with_hook::had2118629c312a4a at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/panicking.rs:805:13 12: 0x7f0c3d484ec3 - std::panicking::begin_panic_handler::{{closure}}::h7fa5985d111bafa2 at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/panicking.rs:664:13 13: 0x7f0c3d483939 - std::sys::backtrace::__rust_end_short_backtrace::h704d151dbefa09c5 at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/sys/backtrace.rs:170:18 14: 0x7f0c3d484b84 - rust_begin_unwind at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/panicking.rs:662:5 15: 0x7f0c3d21e425 - core::panicking::panic_nounwind_fmt::runtime::h1c669551f619867f at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/panicking.rs:112:18 16: 0x7f0c3d21e425 - core::panicking::panic_nounwind_fmt::hc0ae93930ea8f76c at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/panicking.rs:122:5 17: 0x7f0c3d21e4b2 - core::panicking::panic_nounwind::h9f485ff9b02bac75 at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/panicking.rs:221:5 18: 0x7f0c3d21e6d6 - core::panicking::panic_cannot_unwind::hea865182d7ce50af at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/panicking.rs:310:5 19: 0x7f0c3d24ef84 - godot_core::registry::callbacks::get_virtual::h191b2b16f0b857ce at /home/fabian/.cargo/git/checkouts/gdext-76630c89719e160c/a0d5799/godot-core/src/registry/callbacks.rs:102:1 20: 0x35a786c - 21: 0x3326f03 - 22: 0x4751551 - 23: 0x1192209 - 24: 0x3e474d5 - 25: 0x4ae7b23 - 26: 0x7f0c4084aac3 - start_thread at ./nptl/pthread_create.c:442:8 27: 0x7f0c408dc850 - __GI___clone3 at ./misc/../sysdeps/unix/sysv/linux/x86_64/clone3.S:81 28: 0x0 - thread caused non-unwinding panic. aborting. [1] 31441 IOT instruction (core dumped) godot4 . ```

It looks like it only panics in debug builds, and everything seems to work fine in release builds.

What surprises me: In older versions of gdext this was working without having to enable the experimental-threads feature. In general, I wanted to avoid the full overhead of activating this feature, and it seemed to be possible previously. From the traceback it is actually not quite obvious where the access to the binding from a different thread is happening. Is there a way to use this pattern without having to enable the feature?

Isn't this pattern actually valid, considering that the pattern used to work and seems to work fine in release builds?

As far as I can see instantiate_playback does run on the main thread, and it is only the unsafe mix function that gets called from the audio thread. However, that function is "pure Rust", and doesn't do anything in terms of calling any binding, no?

[Bromeon]

Releated to the issues in https://github.com/godot-rust/gdext/issues/713, in particular https://github.com/godot-rust/gdext/issues/709 (although that one got a tailored fix, which seems to not apply here).

What surprises me: In older versions of gdext this was working without having to enable the experimental-threads feature.

This was "working" as a result of us not checking it. But Godot calling into Rust code through different threads can easily introduce unsoundness (one can bypass Send/Sync and other safety measures), so we can't safely allow it -- at least not without some sort of synchronization.

In fact experimental-threads currently just disables these checks, but eventually we need to find a proper solution in https://github.com/godot-rust/gdext/issues/18... Part of that would also be making sure that regular Gd stays single-threaded, so there's no overhead except in places where multithreaded is actually used.

If you have insights into how concretely Godot's multithreaded audio/video classes concretely interact with Rust, that would be very valuable... Maybe we can also think about specific patterns to make such interactions safer :thinking:

[bluenote10]

This was "working" as a result of us not checking it.

What I do not understand is that there is no Godot binding activity happening from the audio thread. Adding some Os::singleton().get_thread_caller_id() debug output to all methods yields:

Example with debug statements

```rust use godot::classes::native::AudioFrame; use godot::classes::{AudioServer, AudioStreamPlayback, IAudioStream, IAudioStreamPlayback, Os}; use godot::prelude::*; #[derive(GodotClass)] #[class(base=Node)] pub struct Demo { audio_player: Gd, } #[godot_api] impl INode for Demo { fn init(base: Base) -> Self { println!( "Demo::init is running on thread {}", Os::singleton().get_thread_caller_id() ); let mut audio_player = AudioStreamPlayer::new_alloc(); audio_player.set_stream(Gd::::from_init_fn(|_| { CustomAudioStream::new() })); base.to_gd().add_child(audio_player.clone()); Self { audio_player } } fn ready(&mut self) { println!( "Demo::ready is running on thread {}", Os::singleton().get_thread_caller_id() ); self.audio_player.play(); } } // CustomAudioStream #[derive(GodotClass)] #[class(base=AudioStream, no_init)] pub struct CustomAudioStream {} #[godot_api] impl IAudioStream for CustomAudioStream { fn instantiate_playback(&self) -> Option> { println!( "CustomAudioStream::instantiate_playback is running on thread {}", Os::singleton().get_thread_caller_id() ); Some( Gd::::from_init_fn(|_base| { CustomAudioStreamPlayback::new(Sequencer { sample_rate: AudioServer::singleton().get_mix_rate(), sample_index: 0, }) }) .upcast(), ) } } impl CustomAudioStream { pub fn new() -> Self { println!( "CustomAudioStream::new is running on thread {}", Os::singleton().get_thread_caller_id() ); Self {} } } // CustomAudioStreamPlayback #[derive(GodotClass)] #[class(base=AudioStreamPlayback, no_init)] pub struct CustomAudioStreamPlayback { sequencer: Sequencer, } #[godot_api] impl IAudioStreamPlayback for CustomAudioStreamPlayback { unsafe fn mix( &mut self, buffer: *mut AudioFrame, _rate_scale: f32, num_requested_frames: i32, ) -> i32 { println!( "CustomAudioStreamPlayback::mix is running on thread {}", Os::singleton().get_thread_caller_id() ); self.sequencer.render_audio(num_requested_frames, buffer); num_requested_frames } fn start(&mut self, _from_pos: f64) {} fn stop(&mut self) {} fn is_playing(&self) -> bool { true } } impl CustomAudioStreamPlayback { fn new(sequencer: Sequencer) -> Self { println!( "CustomAudioStreamPlayback::new is running on thread {}", Os::singleton().get_thread_caller_id() ); Self { sequencer } } } // Sequencer pub struct Sequencer { sample_rate: f32, sample_index: usize, } impl Sequencer { fn render_audio(&mut self, num_requested_frames: i32, buffer: *mut AudioFrame) { const FREQUENCY: f32 = 440.0; for i in 0..num_requested_frames { let phase = 2.0 * std::f32::consts::PI * FREQUENCY * (self.sample_index as f32) / self.sample_rate; let sample = 0.5 * phase.sin(); unsafe { *buffer.offset(i as isize) = AudioFrame { left: sample, right: sample, }; } self.sample_index += 1; } } } ```

Demo::init is running on thread 1
CustomAudioStream::new is running on thread 1
Demo::ready is running on thread 1
CustomAudioStream::instantiate_playback is running on thread 1
CustomAudioStreamPlayback::new is running on thread 1
CustomAudioStreamPlayback::mix is running on thread 11
CustomAudioStreamPlayback::mix is running on thread 11
CustomAudioStreamPlayback::mix is running on thread 11
[...]

I.e., it is only that unsafe mix method that gets called from the audio thread. But this method doesn't call into anything Godot related, no?

This raises the question where the check is actually happening? The traceback doesn't make that very clear -- or it least it is not pointing to anything on user side. So is it just happening on the wrapper side of invoking mix? Considering that mix is unsafe anyway, would it make sense to skip that check for unsafe methods -- there are not safety guarantees for that method anyway?

[Bromeon]

The unsafe is due to a different reason, namely mix accepting a raw pointer parameter.

It can make sense to have methods that are called from other threads unsafe (until we have a proper solution), but how do we reliably know this? We'd probably need to hardcode such APIs in a list. I would not want to go about it the other way (turn off checks for those which happen to be unsafe). It's also not that trivial implementation-wise, as the instance access code knows nothing about the Godot method.

The checks happen here: https://github.com/godot-rust/gdext/blob/19147e8f9cfaf962e3ec01db291e4b41a7b5ee28/godot-ffi/src/binding/single_threaded.rs#L159-L163