`#[should_panic(expected = ...)]` non-functional.

[FraserLee]

Bevy version

Release 0.7

What you did

Repeatedly run the following minimum code sample with cargo test

use bevy::prelude::*;

#[test]
#[should_panic(expected = "expected panic")]
fn mvs_test() {
    App::new()
        .add_plugins(MinimalPlugins)
        .add_system( || {
            panic!("expected panic");
        })
        .run();
}

About 9/10 times the result is as expected

    Finished test [unoptimized + debuginfo] target(s) in 0.18s
     Running unittests src/main.rs (target/debug/deps/temp_test-698e95098388382a)

running 1 test
test mvs_test - should panic ... ok

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

What went wrong

About 1/10 times the test fails, with the message

    Finished test [unoptimized + debuginfo] target(s) in 0.18s
     Running unittests src/main.rs (target/debug/deps/temp_test-698e95098388382a)

running 1 test
test mvs_test - should panic ... FAILED

failures:

---- const_change_test stdout ----
thread 'Compute Task Pool (2)' panicked at 'expected panic', src/main.rs:9:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'mvs_test' panicked at 'task has failed', /Users/fraser/.cargo/registry/src/github.com-1ecc6299db9ec823/async-task-4.2.0/src/task.rs:425:45
note: panic did not contain expected string
      panic message: `"task has failed"`,
 expected substring: `"expected panic"`

failures:
    mvs_test

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

error: test failed, to rerun pass '--bin temp_test'

The correct panic is triggered, but the wrong panic message is sent.

Additional information

I've tried both running tests with cargo test -- --test-threads=1, and using the serial_test crate - neither helping.

[alice-i-cecile]

This seems like another example of the recent panicking global task pool issue we encountered in our CI.

[mockersf]

the panic message will be different wether the panic happens on the main thread or on one of the other threads

[FraserLee]

@mockersf is that intentionally the case, and if so is there any way to change it?

I could be wrong, but it seems that at bare minimum there should be some way to lock-down things to always run on the main thread for testing, though I'd probably go further and say having the same error randomly show different messages depending on the thread setup is a really unergonomic corner, probably worth addressing even in non-test runtime.

[bjorn3]

I think the thread pool should catch panics using std::panic::catch_unwind and resume them on the main thread using std::panic::resume_unwind with the result of catch_unwind. This prevents showing a backtrace twice and it allows libtest to see the original panic message.

[james7132]

This seems like another example of the recent panicking global task pool issue we encountered in our CI.

If this is on 0.7 and not main, this seems to be more an issue with our general panic handling story, though the global task pool changes definitely exacerbate this.

I agree with @bjorn3 here that we should definitely use catch/resume unwind, though the behavior, or at least the default behavior likely needs some more discussion and thought.

If we catch the panic, we need to forward it to the main thread to resume it, but the main thread runs on its own accord and needs to explicitly call into the TaskPool to retrieve these panics. In the meantime, it's unclear what we should then do with the panicking thread.

• If we kill it, it drops all tasks scheduled to it, including local tasks, which in the context of the engine in general, may lead to deadlock: our tasks aren't made to be canceled by anything other than dropping the Task itself.
• We cannot migrate it to a new thread and kill the original as migrating thread local tasks may move non-Send tasks to another thread.
• For the same aforementioned reasons, we cannot stall the panicking thread by just not accepting any new tasks, it will otherwise deadlock whkle waiting for the main thread to check for panics on the pool, which may not happen if the main thread is waiting on local work on the exisiting thread.
• We could just abort. Though that is potentially an even worse solution.

It may be required to either stem all panics and never forward them to the main thread, force local tasks to be Send so they can be properly migrated in the case of a panic, or have the main thread operate as a worker thread in the pool and regularly yield to allow handling these panics.

[mockersf]

@mockersf is that intentionally the case, and if so is there any way to change it?

Unintended consequence of how we handle threads!

As mentioned by James, panicking in threads can cause deadlocks (it uses to happen quite often with asset loader tasks that panicked and deadlocked all the io threads). I think we should panic on the main thread when a panic happens somewhere else, at least until we have a way better error recovery than now

[mockersf]

just remembered this PR that is related: #2307

[hymm]

@FraserLee as a workaround you could add the system to a single threaded stage.