Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
bluenote10 opened this issue Nov 3, 2024 · 3 comments

Comments

@bluenote10
Copy link
Contributor

bluenote10 commented Nov 3, 2024

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 '<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)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread '<unnamed>' 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 - <std::sys::backtrace::BacktraceLock::print::DisplayBacktrace as core::fmt::Display>::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 - <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::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 - <unknown>
  21:          0x3326f03 - <unknown>
  22:          0x4751551 - <unknown>
  23:          0x1192209 - <unknown>
  24:          0x3e474d5 - <unknown>
  25:          0x4ae7b23 - <unknown>
  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 - <unknown>
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
Copy link
Member

Bromeon commented Nov 3, 2024

Releated to the issues in #713, in particular #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 #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 🤔

@bluenote10
Copy link
Contributor Author

bluenote10 commented Nov 3, 2024

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
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<AudioStreamPlayer>,
}

#[godot_api]
impl INode for Demo {
    fn init(base: Base<Self::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::<CustomAudioStream>::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<Gd<AudioStreamPlayback>> {
        println!(
            "CustomAudioStream::instantiate_playback is running on thread {}",
            Os::singleton().get_thread_caller_id()
        );
        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 {
        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
Copy link
Member

Bromeon commented Nov 3, 2024

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:

assert_eq!(
main_thread_id,
std::thread::current().id(),
"attempted to access binding from different thread than main thread; this is UB - use the \"experimental-threads\" feature."
);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants