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

Pipelined rendering thread uses panic for control flow #8936

Closed
bonsairobo opened this issue Jun 23, 2023 · 0 comments · Fixed by #9052
Closed

Pipelined rendering thread uses panic for control flow #8936

bonsairobo opened this issue Jun 23, 2023 · 0 comments · Fixed by #9052
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior P-Crash A sudden unexpected crash

Comments

@bonsairobo
Copy link
Contributor

bonsairobo commented Jun 23, 2023

Bevy version

0.10

What you did

This just happens sometimes when my app exits.

What went wrong

The app prints a panic message (non-deterministically) when you close the window.

2023-06-23T06:03:07.402509Z  INFO bevy_app:winit event_handler:main app:schedule{name=Outer}:system{name="bevy_app::CoreSchedule::outer_loop"}:schedule{name=Main}:multithreaded executor:system{name="bevy_winit::system::despawn_window"}: bevy_winit::system: Closing window 0v0

thread 'Compute Task Pool (6)' panicked at 'called `Result::unwrap()` on an `Err` value: RecvError', /home/duncan/.cargo/registry/src/github.com-1ecc6299db9ec823/bevy_render-0.10.1/src/pipelined_rendering.rs:111:77
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
   0: bevy_render::pipelined_rendering::render thread
             at /home/duncan/.cargo/registry/src/github.com-1ecc6299db9ec823/bevy_render-0.10.1/src/pipelined_rendering.rs:105
thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', /home/duncan/.cargo/registry/src/github.com-1ecc6299db9ec823/bevy_tasks-0.10.1/src/task_pool.rs:376:49

Additional information

I'm guessing the error is non-deterministic because there is a race between the render thread panicking and the app shutting down (killing the thread). But it's clear from reading this code that the render thread can only exit via panicking:

std::thread::spawn(move || {
#[cfg(feature = "trace")]
let _span = bevy_utils::tracing::info_span!("render thread").entered();
loop {
// run a scope here to allow main world to use this thread while it's waiting for the render app
let mut render_app = ComputeTaskPool::get()
.scope(|s| {
s.spawn(async { app_to_render_receiver.recv().await.unwrap() });
})
.pop()
.unwrap();
#[cfg(feature = "trace")]
let _sub_app_span =
bevy_utils::tracing::info_span!("sub app", name = ?RenderApp).entered();
render_app.run();
render_to_app_sender.send_blocking(render_app).unwrap();
}
});

@bonsairobo bonsairobo added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Jun 23, 2023
@nicopap nicopap added A-Rendering Drawing game state to the screen P-Crash A sudden unexpected crash and removed S-Needs-Triage This issue needs to be labelled labels Jun 23, 2023
github-merge-queue bot pushed a commit that referenced this issue Jul 23, 2023
# Objective
Fix #8936.

## Solution
Stop using `unwrap` in the core pipelined rendering logic flow.

Separately also scoped the `sub app` span to just running the render app
instead of including the blocking send.

Current unknowns: should we use `std::panic::catch_unwind` around
running the render app? Other engine threads use it defensively, but
we're letting it bubble up here, and a user-created panic could cause a
deadlock if it kills the thread.

---

## Changelog
Fixed: Pipelined rendering should no longer have spurious panics upon
app exit.
cart pushed a commit that referenced this issue Aug 10, 2023
# Objective
Fix #8936.

## Solution
Stop using `unwrap` in the core pipelined rendering logic flow.

Separately also scoped the `sub app` span to just running the render app
instead of including the blocking send.

Current unknowns: should we use `std::panic::catch_unwind` around
running the render app? Other engine threads use it defensively, but
we're letting it bubble up here, and a user-created panic could cause a
deadlock if it kills the thread.

---

## Changelog
Fixed: Pipelined rendering should no longer have spurious panics upon
app exit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior P-Crash A sudden unexpected crash
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants