Fix pipelined rendering shutdown deadlock

[bryancostanich]

Hey folks! Bryan Costanich here. I'm loving Bevy, but I keep getting bit by this issue, so I decided to take a whack at fixing it.

This is the macOS shutdown deadlock from #12912 -- the freeze that #23838 didn't catch. App fires AppExit, main thread parks forever, you have to force-quit. About 20-40% of the time on Apple Silicon / Metal in tight loops, which is what made it so annoying to track down.

The root cause is what @msvbg sketched out in his comment on #12912: when RenderAppChannels drops, the main thread calls recv_blocking and parks. Meanwhile the render thread is mid-SubApp::update() and has queued a task that needs to run on the main thread (anything with NonSendMarker, e.g. create_surfaces). The render thread's MultiThreadedExecutor::run blocks waiting for that task. The main thread can't run the task because it's parked on recv_blocking. Both sides stuck.

Sample trace (sample on macOS):

main thread:
  World::clear_resources
    -> drop RenderAppChannels
      -> async_channel::RecvInner::wait
        -> pthread_cond_wait        (parked)

render thread:
  SubApp::update
    -> MultiThreadedExecutor::run
      -> block_on
        -> Inner::park
          -> pthread_cond_wait      (parked, waiting on a main-thread task)

If you want to repro: any DefaultPlugins app that fires AppExit shortly after the window comes up, run it in a loop of 20 with a 5-second timeout. You'll see 3-8 hangs per batch on a recent M-series Mac.

What I did

Stashed an Arc<ThreadExecutor> clone of MainThreadExecutor inside RenderAppChannels. In Drop, swapped recv_blocking for ComputeTaskPool::scope_with_executor so the main-thread executor keeps getting pumped while we wait for the render thread to return. That's already how renderer_extract handles the steady-state path -- I'm just applying it to the shutdown path too. Once main-thread tasks can complete, the render thread's block_on resolves, the SubApp comes back, and the channel's recv returns.

It's a small change: one new field on RenderAppChannels, one new constructor arg, and a rewrite of the Drop body.

Heads up: tiny breaking change

RenderAppChannels::new takes a third arg now -- the Arc<ThreadExecutor<'static>> from MainThreadExecutor.0. The struct is internal plumbing, but the constructor is pub, so I'm flagging it. Here's what PipelinedRenderingPlugin::cleanup looks like now:

let executor = app.world().get_resource::<MainThreadExecutor>().unwrap();
let main_thread_executor = executor.0.clone();
// ...
RenderAppChannels::new(
    app_to_render_sender,
    render_to_app_receiver,
    main_thread_executor,
)

If anyone downstream is constructing RenderAppChannels directly (probably nobody, this is pretty internal), they'll need to pass the executor in.

Testing

Added a regression test: pipelined_rendering::tests::drop_pumps_main_thread_executor_to_avoid_shutdown_deadlock. The fake "render thread" in the test does the same thing the real one does in the bad case -- spawns a task on the shared MainThreadExecutor and awaits it before sending the SubApp back. Without the fix the test hangs forever (I verified -- temporarily reverted Drop back to recv_blocking and watched it block past the 60s mark). With the fix it finishes immediately. Gated on feature = "multi_threaded" since ThreadExecutor::spawn only exists there.

I also stress-tested it end-to-end on a real wgpu/Metal app on macOS / Apple Silicon, with this patch cherry-picked onto release-0.18.1 and PipelinedRenderingPlugin re-enabled. Before the fix: 20-40% flake rate across 20+ runs through a few different exit paths (scheduled AppExit, programmatic WindowCloseRequested, timer-driven exit). After the fix: 30/30 clean across the same paths.

Happy to take review feedback. Thanks for all the great work on Bevy!

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide, as well as our policy regarding AI usage, and we look forward to reviewing your pull request shortly ✨

[bryancostanich]

hey all right, made it through the CI checks. that took some work. :D

[bryancostanich]

FYI, i added a repro here: #24035 (comment)

[bryancostanich]

@atlv24 can we get a review on this, please?

[bryancostanich]

what am i doing wrong here? i submitted an issue, other folks have commented that they've hit it as well. i filed a repro that hits 100% of the time, and i filed this PR with the actual fix. how do i get eyes on this?

[Zeophlite]

what am i doing wrong here? i submitted an issue, other folks have commented that they've hit it as well. i filed a repro that hits 100% of the time, and i filed this PR with the actual fix. how do i get eyes on this?

It got added to the 0.20 milestone, so would of been looked at eventually, but I find sometimes you need to bump the PR in Discord

[stuartparmenter]

You can simplify/not need to bring Arc & ThreadExecutor in to this and can just directly use MainThreadExecutor.

I had just fixed this on one of my branches, in a nearly identical way -- see 4318f89

If you can just simplify the main_thread_executor part a little, this fix is solid.

For the test, it currently passes after the fix, but if the bug comes back it will hang rather than fail with the intended assertion. Can we structure it so the test thread waits on a bounded recv_timeout, while the potentially-deadlocking drop(channels) happens on another thread?

Maybe we structure this as a bounded progress test instead of timing drop(channels) after it returns?

Something like:

1. Spawn a synthetic “main thread” that creates the MainThreadExecutor.
2. In the render thread, queue work onto that executor and signal over a channel once the work is definitely queued.
3. Only then call drop(channels) on the synthetic main thread.
4. Have the actual test thread wait on a done channel with recv_timeout.

[hymm]

This can double panic since it's running arbitrary tasks. We will need to suppress the panic somehow if the scope panics.