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

coroutine_pause.rs test broken on head #693

Open
mandroll opened this issue Jan 26, 2024 · 4 comments · May be fixed by #744
Open

coroutine_pause.rs test broken on head #693

mandroll opened this issue Jan 26, 2024 · 4 comments · May be fixed by #744
Labels
bug Something isn't working

Comments

@mandroll
Copy link
Contributor

When running cargo test in my fork, I see the following error:

cargo test
    Finished test [unoptimized + debuginfo] target(s) in 0.07s
     Running unittests src\lib.rs (target\debug\deps\macroquad-39e857bc1218d84e.exe)

running 2 tests
test color::color_from_bytes ... ok
test material::shaders::preprocessor_test ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests\back_to_the_future.rs (target\debug\deps\back_to_the_future-29cb4bf128cc5937.exe)

running 1 test
test back_to_the_future ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.28s

     Running tests\back_to_the_future_coroutines.rs (target\debug\deps\back_to_the_future_coroutines-b2e3d9943e110d30.exe)

running 1 test
test back_to_the_future_coroutine ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.41s

     Running tests\coroutine_pause.rs (target\debug\deps\coroutine_pause-8289b31e25dc5fee.exe)

running 3 tests
test coroutine_execution_order ... ok
error: test failed, to rerun pass `--test coroutine_pause`

Caused by:
  process didn't exit successfully: `C:\Users\mandroll\Documents\GitHub\macroquad\target\debug\deps\coroutine_pause-8289b31e25dc5fee.exe` (exit code: 0xc0000374, STATUS_HEAP_CORRUPTION)
note: test exited abnormally; to see the full output pass --nocapture to the harness.
@cyrgani
Copy link
Contributor

cyrgani commented Jun 12, 2024

The error comes from this Drop implementation:

impl Drop for GlPipelineGuarded {
    fn drop(&mut self) {
        get_context().gl.delete_pipeline(self.0);
    }
}

The mutable static CONTEXT contains a Option<Context> object. get_context returns a &'static mut Context.

A Context contains a UiContext, which contains a Option<Material>. A Material finally contains an Arc<GlPipelineGuarded>.

Since CONTEXT is a static variable, its value is not dropped at the end of the program.

When starting the first test, CONTEXT is None. While creating the window, CONTEXT will become Some(Context {...}).

At the end of the first test, this object will not be dropped and remain the same when the second test begins.

The second test will then try to reassign a new Some(Context {...}) to CONTEXT. This overwriting causes the old Context to be dropped. When its fields are being dropped, eventually a GlPipelineGuarded will be dropped as well. When dropping the GlPipelineGuarded, it will try to get a mutable reference to CONTEXT and modify it. Since the old Context is already partially dropped, this causes Undefined Behavior.

Now, I see these options:

  1. Remove this implementation and hope nothing depends on it (removing it causes the test to succeed again).
  2. Replace static mut with a type providing interior mutability (seems like the best way, given that references to mutable statics will be illegal in the 2024 edition).
  3. Use a crate such as ctor to drop the static value at the end.

@cyrgani cyrgani linked a pull request Jun 22, 2024 that will close this issue
@cyrgani
Copy link
Contributor

cyrgani commented Jul 12, 2024

The error changed with the release of the latest miniquad version: it panics now instead of causing visible UB:

thread 'coroutine_execution_order' panicked at /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/miniquad-0.4.5/src/lib.rs:90:29:
NATIVE_DISPLAY already set

@cyrgani
Copy link
Contributor

cyrgani commented Jul 12, 2024

This needs to be fixed within miniquad as even this minimized example of two macroquad tests without content panics:

pub mod test {
    pub static mut MUTEX: Option<std::sync::Mutex<()>> = None;
    pub static ONCE: std::sync::Once = std::sync::Once::new();
}

struct Stage {}

impl miniquad::EventHandler for Stage {
    fn update(&mut self) {}

    fn draw(&mut self) {
        miniquad::window::quit();
    }
}

pub fn new_window() {
    miniquad::start(miniquad::conf::Conf::default(), || Box::new(Stage {}));
}

#[test]
fn x() {
    let _lock = unsafe {
        test::ONCE.call_once(|| {
            test::MUTEX = Some(std::sync::Mutex::new(()));
        });
        test::MUTEX.as_mut().unwrap().lock()
    };
    new_window();
}

#[test]
fn y() {
    let _lock = unsafe {
        test::ONCE.call_once(|| {
            test::MUTEX = Some(std::sync::Mutex::new(()));
        });
        test::MUTEX.as_mut().unwrap().lock()
    };
    new_window();
}

@cyrgani
Copy link
Contributor

cyrgani commented Jul 13, 2024

This seems hard to fix:
This is the relevant section from miniquads lib.rs:

static NATIVE_DISPLAY: OnceLock<Mutex<native::NativeDisplayData>> = OnceLock::new();

fn set_display(display: native::NativeDisplayData) {
    NATIVE_DISPLAY
        .set(Mutex::new(display))
        .unwrap_or_else(|_| panic!("NATIVE_DISPLAY already set"));
}
fn native_display() -> &'static Mutex<native::NativeDisplayData> {
    NATIVE_DISPLAY
        .get()
        .expect("Backend has not initialized NATIVE_DISPLAY yet.") //|| Mutex::new(Default::default()))
}

It would be necessary to drop this value at the end of each test, which would require replacing OnceLock with some other type. So far, I was not able to come up with a design to replace OnceLock that would avoid static mut, allow dropping the native::NativeDisplayData and allow returning &'static Mutex<native::NativeDisplayData>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants