Skip to content

Commit

Permalink
bevy_ecs: flush entities after running observers and hooks in despawn (
Browse files Browse the repository at this point in the history
…#15398)

# Objective

Fixes #14467

Observers and component lifecycle hooks are allowed to perform
operations that subsequently require `Entities` to be flushed, such as
reserving a new entity. If this occurs during an `on_remove` hook or an
`OnRemove` event trigger during an `EntityWorldMut::despawn`, a panic
will occur.

## Solution

Call `world.flush_entities()` after running `on_remove` hooks/observers
during `despawn`

## Testing

Added a new test that fails before the fix and succeeds afterward.
  • Loading branch information
jrobsonchase authored Sep 24, 2024
1 parent 1a41c73 commit 3d0f240
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 1 deletion.
22 changes: 22 additions & 0 deletions crates/bevy_ecs/src/observer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1160,4 +1160,26 @@ mod tests {
world.flush();
assert_eq!(vec!["event", "event"], world.resource::<Order>().0);
}

// Regression test for https://github.com/bevyengine/bevy/issues/14467
// Fails prior to https://github.com/bevyengine/bevy/pull/15398
#[test]
fn observer_on_remove_during_despawn_spawn_empty() {
let mut world = World::new();

// Observe the removal of A - this will run during despawn
world.observe(|_: Trigger<OnRemove, A>, mut cmd: Commands| {
// Spawn a new entity - this reserves a new ID and requires a flush
// afterward before Entities::free can be called.
cmd.spawn_empty();
});

let ent = world.spawn(A).id();

// Despawn our entity, which runs the OnRemove observer and allocates a
// new Entity.
// Should not panic - if it does, then Entities was not flushed properly
// after the observer's spawn_empty.
world.despawn(ent);
}
}
5 changes: 4 additions & 1 deletion crates/bevy_ecs/src/world/entity_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1297,7 +1297,6 @@ impl<'w> EntityWorldMut<'w> {
/// See [`World::despawn`] for more details.
pub fn despawn(self) {
let world = self.world;
world.flush_entities();
let archetype = &world.archetypes[self.location.archetype_id];

// SAFETY: Archetype cannot be mutably aliased by DeferredWorld
Expand All @@ -1323,6 +1322,10 @@ impl<'w> EntityWorldMut<'w> {
world.removed_components.send(component_id, self.entity);
}

// Observers and on_remove hooks may reserve new entities, which
// requires a flush before Entities::free may be called.
world.flush_entities();

let location = world
.entities
.free(self.entity)
Expand Down

0 comments on commit 3d0f240

Please sign in to comment.