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

In use-after-despawn situation, make despawning system name available #2949

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,10 @@ path = "examples/ecs/system_sets.rs"
name = "timers"
path = "examples/ecs/timers.rs"

[[example]]
name = "use_after_despawn"
path = "examples/ecs/use_after_despawn.rs"

# Games
[[example]]
name = "alien_cake_addict"
Expand Down
5 changes: 5 additions & 0 deletions crates/bevy_ecs/src/schedule/stage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -740,6 +740,11 @@ fn find_ambiguities(systems: &[impl SystemContainer]) -> Vec<(usize, usize, Vec<

impl Stage for SystemStage {
fn run(&mut self, world: &mut World) {
if world.despawned.len() > 0 {
println!("despawned clear {}", world.despawned.len());
world.despawned.clear();
}

if let Some(world_id) = self.world_id {
assert!(
world.id() == world_id,
Expand Down
51 changes: 49 additions & 2 deletions crates/bevy_ecs/src/system/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ pub trait Command: Send + Sync + 'static {
pub struct Commands<'w, 's> {
queue: &'s mut CommandQueue,
entities: &'w Entities,
pub(crate) name: &'static str,
}

impl<'w, 's> Commands<'w, 's> {
Expand All @@ -43,9 +44,15 @@ impl<'w, 's> Commands<'w, 's> {
Self {
queue,
entities: world.entities(),
name: "",
}
}

pub fn with_name(mut self, name: &'static str) -> Self {
self.name = name;
self
}

/// Creates a new empty [`Entity`] and returns an [`EntityCommands`] builder for it.
///
/// To directly spawn an entity with a [`Bundle`] included, you can use
Expand Down Expand Up @@ -346,6 +353,10 @@ impl<'w, 's, 'a> EntityCommands<'w, 's, 'a> {
self.entity
}

pub fn system_name(&self) -> &'static str {
self.commands.name
}

/// Adds a [`Bundle`] of components to the entity.
///
/// # Example
Expand Down Expand Up @@ -506,6 +517,7 @@ impl<'w, 's, 'a> EntityCommands<'w, 's, 'a> {
pub fn despawn(&mut self) {
self.commands.add(Despawn {
entity: self.entity,
name: self.commands.name,
})
}

Expand Down Expand Up @@ -586,15 +598,19 @@ where
#[derive(Debug)]
pub struct Despawn {
pub entity: Entity,
pub name: &'static str,
}

impl Command for Despawn {
fn write(self, world: &mut World) {
println!("despawn from {}", self.name);
world.set_system_name(Some(self.name));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This set name thing is crude. I am sorry. It may could be a separate despawn* function on world instead which takes the despawn origin name.

if !world.despawn(self.entity) {
warn!("Could not despawn entity {:?} because it doesn't exist in this World.\n\
If this command was added to a newly spawned entity, ensure that you have not despawned that entity within the same stage.\n\
This may have occurred due to system order ambiguity, or if the spawning system has multiple command buffers", self.entity);
}
world.set_system_name(None);
}
}

Expand All @@ -611,9 +627,15 @@ where
if let Some(mut entity) = world.get_entity_mut(self.entity) {
entity.insert_bundle(self.bundle);
} else {
let name = world
.despawned
.iter()
.find_map(|(e, n)| if *e == self.entity { Some(n) } else { None })
.unwrap_or(&"unknown system");
panic!("Could not insert a bundle (of type `{}`) for entity {:?} because it doesn't exist in this World.\n\
Entity was despawned in {}.\n\
If this command was added to a newly spawned entity, ensure that you have not despawned that entity within the same stage.\n\
This may have occurred due to system order ambiguity, or if the spawning system has multiple command buffers", std::any::type_name::<T>(), self.entity);
This may have occurred due to system order ambiguity, or if the spawning system has multiple command buffers", std::any::type_name::<T>(), self.entity, name);
}
}
}
Expand All @@ -632,9 +654,15 @@ where
if let Some(mut entity) = world.get_entity_mut(self.entity) {
entity.insert(self.component);
} else {
let name = world
.despawned
.iter()
.find_map(|(e, n)| if *e == self.entity { Some(n) } else { None })
.unwrap_or(&"unknown system");
panic!("Could not add a component (of type `{}`) to entity {:?} because it doesn't exist in this World.\n\
Entity was despawned in {}.\n\
If this command was added to a newly spawned entity, ensure that you have not despawned that entity within the same stage.\n\
This may have occurred due to system order ambiguity, or if the spawning system has multiple command buffers", std::any::type_name::<T>(), self.entity);
This may have occurred due to system order ambiguity, or if the spawning system has multiple command buffers", std::any::type_name::<T>(), self.entity, name);
}
}
}
Expand Down Expand Up @@ -753,6 +781,25 @@ mod tests {
assert_eq!(results2, vec![]);
}

#[test]
#[should_panic(expected = "Entity was despawned in test commands")]
fn use_after_despawn() {
let mut world = World::default();
let mut command_queue = CommandQueue::default();
let entity = {
Commands::new(&mut command_queue, &world)
.spawn_bundle((1u32, 2u64))
.id()
};
command_queue.apply(&mut world);
{
let mut commands = Commands::new(&mut command_queue, &world).with_name("test commands");
commands.entity(entity).despawn();
commands.entity(entity).insert(5);
}
command_queue.apply(&mut world);
}

#[test]
fn remove_components() {
let mut world = World::default();
Expand Down
8 changes: 6 additions & 2 deletions crates/bevy_ecs/src/system/system_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -519,11 +519,15 @@ impl<'w, 's> SystemParamFetch<'w, 's> for CommandQueue {
#[inline]
unsafe fn get_param(
state: &'s mut Self,
_system_meta: &SystemMeta,
system_meta: &SystemMeta,
world: &'w World,
_change_tick: u32,
) -> Self::Item {
Commands::new(state, world)
let name: &'static str = match system_meta.name {
std::borrow::Cow::Borrowed(s) => s,
std::borrow::Cow::Owned(_) => "owned",
};
Commands::new(state, world).with_name(name)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should just pass the Cow here 🐄 . It is in both cases cheap, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it would be more appropriate to pass the whole SystemMeta, but well, I can't figure out the lifetime specifiers. And maybe the lifetime is not even guaranteed since the Despawn command, lands in a queue and the later the SystemMeta would land in some kind of buffer for the whole stage.

}
}

Expand Down
5 changes: 5 additions & 0 deletions crates/bevy_ecs/src/world/entity_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,11 @@ impl<'w> EntityMut<'w> {
let table_row;
let moved_entity;
{
if let Some(name) = world.system_name {
println!("world despawn from {}", name);
world.despawned.push((self.entity, name));
}

let archetype = &mut world.archetypes[location.archetype_id];
for component_id in archetype.components() {
let removed_components = world
Expand Down
10 changes: 10 additions & 0 deletions crates/bevy_ecs/src/world/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ pub struct World {
main_thread_validator: MainThreadValidator,
pub(crate) change_tick: AtomicU32,
pub(crate) last_change_tick: u32,
pub(crate) despawned: Vec<(Entity, &'static str)>,
pub(crate) system_name: Option<&'static str>,
}

impl Default for World {
Expand All @@ -101,6 +103,8 @@ impl Default for World {
// are detected on first system runs and for direct world queries.
change_tick: AtomicU32::new(1),
last_change_tick: 0,
despawned: Vec::new(),
system_name: None,
}
}
}
Expand All @@ -117,6 +121,12 @@ impl World {
World::default()
}

#[inline]
pub fn set_system_name(&mut self, name: Option<&'static str>) {
println!("set system name {:?}", name);
self.system_name = name;
}

/// Retrieves this [`World`]'s unique ID
#[inline]
pub fn id(&self) -> WorldId {
Expand Down
7 changes: 6 additions & 1 deletion crates/bevy_transform/src/hierarchy/hierarchy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use bevy_utils::tracing::debug;
#[derive(Debug)]
pub struct DespawnRecursive {
entity: Entity,
name: &'static str,
}

pub fn despawn_with_children_recursive(world: &mut World, entity: Entity) {
Expand Down Expand Up @@ -38,7 +39,10 @@ fn despawn_with_children_recursive_inner(world: &mut World, entity: Entity) {

impl Command for DespawnRecursive {
fn write(self, world: &mut World) {
println!("despawn recursive from {}", self.name);
world.set_system_name(Some(self.name));
despawn_with_children_recursive(world, self.entity);
world.set_system_name(None);
}
}

Expand All @@ -51,7 +55,8 @@ impl<'w, 's, 'a> DespawnRecursiveExt for EntityCommands<'w, 's, 'a> {
/// Despawns the provided entity and its children.
fn despawn_recursive(mut self) {
let entity = self.id();
self.commands().add(DespawnRecursive { entity });
let name = self.system_name();
self.commands().add(DespawnRecursive { entity, name });
}
}

Expand Down
44 changes: 44 additions & 0 deletions examples/ecs/use_after_despawn.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
use bevy::prelude::*;

fn main() {
App::new()
.add_startup_system(do_spawn)
// This system despawns the entity in the Update stage.
.add_system(do_despawn.system().label("despawn"))
// This system inserts into the entity in the Update stage.
// It is scheduled explicitly to run after do_despawn, so its Insert command
// will panic because in the meantime the Despawn command despawed the entity.
//
// Here it is simple. But it could be as complicated a schedule like
// the inserting system is some system of a third party plugin.
// About this imagined system you as a user,
// you don't immediately know it exists,
// you don't know when it is scheduled,
// you don't know if it is handling intermittent despawned entities well.
// But eventually the parallel schedule is run in a way where
// both systems, your despawner and the third party inserter, run in the same stage
// in an order where they clash.
// And when you don't know, which of my entity kinds was it? When was it despawned?
// How to reproduce it?
.add_system(do_insert.system().after("despawn"))
.run();
}

fn do_spawn(mut cmds: Commands) {
let entity = cmds.spawn().insert(5u32).id();
println!("{:?} spawn", entity);
}

fn do_despawn(query: Query<Entity, With<u32>>, mut cmds: Commands) {
for entity in query.iter() {
println!("{:?} despawn", entity);
cmds.entity(entity).despawn();
}
}

fn do_insert(query: Query<Entity, With<u32>>, mut cmds: Commands) {
for entity in query.iter() {
println!("{:?} insert", entity);
cmds.entity(entity).insert("panic".to_string());
}
}