Panics in one task pool scope can panic a separate scope

[hymm]

Bevy version

commit: 54a1e51

What you did

This is a continued investigation into #4998, #4996, #6081, #5285, and #5054.

A quick summary is that the panic in the panic_when_hierachy_cycle test infests other tests and causes them to panic. This was fixed by changing the panicking test to be single threaded and not use the thread pool. While this is a fine solution for testing, it unearthed a problem with how the task pool scopes work.

What went wrong

A task_pool.scope runs it's own instance of the executor so tasks can be run on the thread the scope is running on. In the case of these tests each test has it's own instance of scope running. When we get the spurious failures, the panicking system is actually getting run on the scope executor of the failing test instead of the scope in the should_panic test or one of the task pool threads.

It definitely feels incorrect that a panic from one scope's tasks is causing problems in a scope that is only related by using the same task pool.

Solutions?

Wrapping the try_tick's in a catch_unwind seems to prevent this problem.

let res = std::panic::catch_unwind(|| {
    executor.try_tick();
    task_scope_executor.try_tick();
});

But I'm not sure about the correctness of this and it probably has bad effects on performance since this is a pretty hot loop.

Another solution could be to not allow tasks from one scope executing on another scope. But this would limit the number of threads available to a scope to run tasks.

Overall I'm not sure what the correct fix for this would be or look like.

[james7132]

Could we make Scope a Future instead of a synchronous function? This would allow yielding back to the executor/TaskPool, relying on the catch_unwind added in #6443, and allow transitively carrying the panic through via bubbling.

[hymm]

async scopes have a lot of open questions on how to implement correctly. https://rust-lang.github.io/wg-async/vision/roadmap/scopes.html. We probably don't want to open that can of worms to just solve this problem.

[james7132]

Thinking on this more. I think the solution in the original issue is correct for the same reasons #6443's changes are correct. By dropping the runnable during the panic, it and causes the top level Task that spawned it to propagate the panic.