Skip to content

Fix handling of component drop panics during insert #474

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.

Sign up for GitHub

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

Merged
Ralith merged 1 commit into from
Oct 3, 2026
Merged

Fix handling of component drop panics during insert #474

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
154 changes: 115 additions & 39 deletions src/world.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,6 +642,73 @@ impl World {
graph_origin: u32,
loc: Location,
) {
/// Guard object to ensure that we finish creating the new entity, and cleaning up the old
/// one, even if a component's drop panics
struct Guard<'a, T: DynamicBundle> {
archetypes: &'a mut ArchetypeSet,
entities: &'a mut Entities,
/// `entity`'s previous location
loc: Location,
/// `entity`'s new archetype, and information about components affected
target: &'a InsertTarget,
/// `entity`'s new ID, if moved
target_index: u32,
/// Components being updated
components: Option<T>,
}

impl<T: DynamicBundle> Drop for Guard<'_, T> {
fn drop(&mut self) {
if self.target.index == self.loc.archetype {
// Update components in the current archetype
let arch = &mut self.archetypes.archetypes[self.loc.archetype as usize];
unsafe {
self.components.take().unwrap().put(|ptr, ty| {
arch.put_dynamic(ptr, ty.id(), ty.layout().size(), self.loc.index);
});
}

return;
}

let (source_arch, target_arch) = index2(
&mut self.archetypes.archetypes,
self.loc.archetype as usize,
self.target.index as usize,
);

unsafe {
// Move the new components
self.components.take().unwrap().put(|ptr, ty| {
target_arch.put_dynamic(
ptr,
ty.id(),
ty.layout().size(),
self.target_index,
);
});

// Move the components we're keeping
for &ty in &self.target.retained {
let src = source_arch
.get_dynamic(ty.id(), ty.layout().size(), self.loc.index)
.unwrap();
target_arch.put_dynamic(
src.as_ptr(),
ty.id(),
ty.layout().size(),
self.target_index,
)
}

// Free storage in the old archetype
if let Some(moved) = source_arch.remove(self.loc.index, false) {
self.entities.meta[moved as usize].location.index = self.loc.index;
}
}
}
}

let target_storage;
let target = match components.key() {
None => {
Expand All @@ -657,53 +724,33 @@ impl World {
},
};

let source_arch = &mut self.archetypes.archetypes[loc.archetype as usize];
unsafe {
// Drop the components we're overwriting
for &ty in &target.replaced {
let ptr = source_arch
.get_dynamic(ty.id(), ty.layout().size(), loc.index)
.unwrap();
ty.drop(ptr.as_ptr());
}

if target.index == loc.archetype {
// Update components in the current archetype
let arch = &mut self.archetypes.archetypes[loc.archetype as usize];
components.put(|ptr, ty| {
arch.put_dynamic(ptr, ty.id(), ty.layout().size(), loc.index);
});
return;
}

let (source_arch, target_arch) = index2(
&mut self.archetypes.archetypes,
loc.archetype as usize,
target.index as usize,
);

let mut target_index = 0;
if target.index != loc.archetype {
// Allocate storage in the archetype and update the entity's location to address it
let target_index = target_arch.allocate(entity.id);
target_index =
unsafe { self.archetypes.archetypes[target.index as usize].allocate(entity.id) };
let meta = &mut self.entities.meta[entity.id as usize];
meta.location.archetype = target.index;
meta.location.index = target_index;
}

// Move the new components
components.put(|ptr, ty| {
target_arch.put_dynamic(ptr, ty.id(), ty.layout().size(), target_index);
});
let guard = Guard {
archetypes: &mut self.archetypes,
entities: &mut self.entities,
loc,
target,
target_index,
components: Some(components),
};

// Move the components we're keeping
for &ty in &target.retained {
let src = source_arch
let source_arch = &mut guard.archetypes.archetypes[loc.archetype as usize];
unsafe {
// Drop the components we're overwriting
for &ty in &target.replaced {
let ptr = source_arch
.get_dynamic(ty.id(), ty.layout().size(), loc.index)
.unwrap();
target_arch.put_dynamic(src.as_ptr(), ty.id(), ty.layout().size(), target_index)
}

// Free storage in the old archetype
if let Some(moved) = source_arch.remove(loc.index, false) {
self.entities.meta[moved as usize].location.index = loc.index;
ty.drop(ptr.as_ptr());
}
}
}
Expand Down Expand Up @@ -1389,6 +1436,8 @@ impl Hasher for IndexTypeIdHasher {

#[cfg(test)]
mod tests {
use core::{mem, panic::AssertUnwindSafe};

use super::*;

#[test]
Expand Down Expand Up @@ -1517,4 +1566,31 @@ mod tests {
assert_eq!(&*world.get::<&String>(e0).unwrap(), "b");
assert_eq!(&*world.get::<&String>(e1).unwrap(), "c");
}

/// Verify that insert has the expected effect even if a component panics on drop
#[test]
#[cfg(feature = "std")]
fn panic_on_insert_drop() {
use std::panic::catch_unwind;

struct Bomb;
impl Drop for Bomb {
fn drop(&mut self) {
panic!();
}
}

let mut world = World::new();
let e = world.spawn((Bomb,));

// Overwrite e's Bomb with a new Bomb, causing the old one to be dropped and panic
assert!(catch_unwind(AssertUnwindSafe(|| world.insert(e, (Bomb, true)))).is_err());
// Confirm the bool still got inserted
assert!(*world.get::<&bool>(e).unwrap());
// Confirm the old entity location got cleaned up
assert_eq!(world.iter().count(), 1);

// Disarm remaining bomb so World drop doesn't panic
mem::forget(world.remove_one::<Bomb>(e));
}
}
Loading