Skip to content

Pass command panics to the fallback error handler#24911

Open
chescock wants to merge 12 commits into
bevyengine:mainfrom
chescock:command-panics
Open

Pass command panics to the fallback error handler#24911
chescock wants to merge 12 commits into
bevyengine:mainfrom
chescock:command-panics

Conversation

@chescock

@chescock chescock commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Objective

Improve the handling of commands that panic.

Currently, if a command panics, any unapplied commands are left in the command queue. This means they will get run, but at an unpredictable time in the future.

And when running in no_std, cleanup is not run at all and the commands simply leak.

Solution

Catch panics from commands and pass them to the fallback error handler, just as we now do for systems from #24240. This means if the error handler is set to something non-panicking like warn, then flush_commands() will never panic and all commands in the queue will be applied.

Introduce a RAII guard during command application that will drop any commands still in the queue and perform cleanup during unwinding, even on no_std.

To make the command's type name available in the error handler, add a field to the RAII guard that gets written by consume_command_and_get_size before applying the command.

@chescock chescock added A-ECS Entities, components, systems, and events S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 7, 2026
@github-project-automation github-project-automation Bot moved this to Needs SME Triage in ECS Jul 7, 2026

@SpecificProtagonist SpecificProtagonist left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great; recovering as early as possible when the fallback error handler is set to swallow the panic is the right thing to do.

Comment thread crates/bevy_ecs/src/world/command_queue.rs Outdated
Comment thread crates/bevy_ecs/src/world/command_queue.rs Outdated
Comment thread crates/bevy_ecs/src/world/command_queue.rs Outdated
chescock added 3 commits July 7, 2026 17:53
This reverts commit ae39893.

Revert "Expose the type name for type-erased commands."

This reverts commit 25ecdc7.

@urben1680 urben1680 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR just moves some code, I just kinda squinted on these parts and did not check too attentive that these not have changed.

I would only like one more test to be added.

runner.local_cursor += size_of::<C>();
#[cfg(all(feature = "debug", feature = "std"))]
{
runner.current_command_name = core::any::type_name::<C>();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda wish DebugName worked like MaybeLocation where it is generic with a good default type but you could change it, like to &'static str in this case.

queue.push(SpawnCommand);

let mut world = World::new();
world.insert_resource(FallbackErrorHandler(warn));

@urben1680 urben1680 Jul 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test and the other basically only test how the program continues after the panic was kind of ignored. Since this PR is about passing the error to the handler, I would like a more explicit test where you add a custom handler that asserts that it actually received the panic with the specific message. Such a test would also need to assert the handler was used, maybe by flipping a static AtomicBool.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test and the other basically only test how the program continues after the panic was kind of ignored. Since this PR is about passing the error to the handler, I would like a more explicit test where you add a custom handler that asserts that it actually received the panic with the specific message. Such a test would also need to assert the handler was used, maybe by flipping a static AtomicBool.

Oh, that's a very good idea!

... And having started writing a test there, I now discover that the context wasn't actually getting passed because the debug feature is on bevy_utils and not bevy_ecs. Which ties into your other comment :).

Let me try one more rewrite on the context passing. I didn't want to put the unwind handling in the generic code because it would get monomorphized a lot, but I think if I just call catch_unwind and DebugName::type_name there and then extract everything else to a #[cold] non-generic function that it shouldn't cause very much codegen.

chescock added 4 commits July 11, 2026 09:08
…unt of code generated."

This reverts commit e674899.

Revert "Provide the command's type name to the error context by writing to a field in the `CommandRunner`."

This reverts commit 1a653b4.
…ble,

but move all the handling code into a `#[cold]` function so that it is not monomorphized.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ECS Entities, components, systems, and events S-Needs-Review Needs reviewer attention (from anyone!) to move forward

Projects

Status: Needs SME Triage

Development

Successfully merging this pull request may close these issues.

3 participants