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.

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

Jump to bottom

[Merged by Bors] - drop old value in insert_resource_by_id if exists #5587

Closed
wants to merge 4 commits into from
Closed

[Merged by Bors] - drop old value in insert_resource_by_id if exists #5587

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
64d01dd
drop old value in insert_resource_by_id
jakobhellermann Aug 5, 2022
df3ec96
use DropTestHelper
jakobhellermann Aug 8, 2022
a7f6c92
remove AssertUnwindSafe
jakobhellermann Aug 8, 2022
d3b7c4d
derive resource
jakobhellermann Aug 9, 2022
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
52 changes: 44 additions & 8 deletions crates/bevy_ecs/src/world/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1248,13 +1248,7 @@ impl World {
// SAFETY: column is of type R and has been allocated above
column.push(value, ComponentTicks::new(change_tick));
} else {
let ptr = column.get_data_unchecked_mut(0);
std::ptr::copy_nonoverlapping::<u8>(
value.as_ptr(),
ptr.as_ptr(),
column.item_layout().size(),
);
column.get_ticks_unchecked_mut(0).set_changed(change_tick);
column.replace(0, value, change_tick);
}
}

Expand Down Expand Up @@ -1593,7 +1587,7 @@ mod tests {
Drop(ID),
}

#[derive(Component)]
#[derive(Resource, Component)]
struct MayPanicInDrop {
drop_log: Arc<Mutex<Vec<DropLogItem>>>,
expected_panic_flag: Arc<AtomicBool>,
Expand Down Expand Up @@ -1786,6 +1780,48 @@ mod tests {
assert_eq!(DROP_COUNT.load(std::sync::atomic::Ordering::SeqCst), 1);
}

#[test]
fn insert_resource_by_id_drop_old() {
Copy link
Member

Choose a reason for hiding this comment

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

You should use DropTestHelper and a regular rust type for the resource instead of doing this all dynamically. You can get the ComponentId representing the rust type via world.components.get_resource_id(TypeId::of::<Whatever>()).unwrap() (after you've inserted the resource the first time otherwise it wont have been registered lol)

Copy link
Contributor Author

@jakobhellermann jakobhellermann Aug 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I changed the test to use DropTestHelper: https://github.com/bevyengine/bevy/pull/5587/files#diff-8799631eaab5e61e5dbc20251e08d83474df566ba159061cf2a78ce8f1fd59d5R1810-R1854

Unfortunately I had to use AssertUnwindSafe to be able to create the world outside of the catch_unwind and reference it afterwards, otherwise I was getting
| `&mut world::World` may not be safely transferred across an unwind boundary
I don't know much about unwind safety, do you know if this assertion is justified?
Alternatively I can create the world inside the catch_unwind and not remove the !contains_resource assert.

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively I can create the world inside the catch_unwind and not remove the !contains_resource assert.

I prefer this approach as it's a bit less spooky.

Copy link
Member

Choose a reason for hiding this comment

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

You can set make_component(false, 1) and get rid of the catch unwind/assertunwindsafe. I don't think panic in drop was relevent to this issue was it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can set make_component(false, 1) and get rid of the catch unwind/assertunwindsafe. I don't think panic in drop was relevent to this issue was it?

That is currently not possible with the DropTestHelper because it needs at least one dropped make_component(true) to "defuse" itself: #5615

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 removed the AssertUnwindSafe and the !contains_resource assert.

Copy link
Member

Choose a reason for hiding this comment

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

Oh thats unfortunate oh well ...

let drop_test_helper = DropTestHelper::new();
let res = std::panic::catch_unwind(|| {
let mut world = World::new();

world.insert_resource(drop_test_helper.make_component(false, 0));
let component_id = world
.components()
.get_resource_id(std::any::TypeId::of::<MayPanicInDrop>())
.unwrap();

OwningPtr::make(drop_test_helper.make_component(true, 1), |ptr| {
// SAFETY: value is valid for the component layout
unsafe {
world.insert_resource_by_id(component_id, ptr);
}
});

OwningPtr::make(drop_test_helper.make_component(false, 2), |ptr| {
// SAFETY: value is valid for the component layout
unsafe {
world.insert_resource_by_id(component_id, ptr);
}
});
});

let drop_log = drop_test_helper.finish(res);

assert_eq!(
drop_log,
[
DropLogItem::Create(0),
DropLogItem::Create(1),
DropLogItem::Drop(0), // inserting 1 drops 0
DropLogItem::Create(2),
DropLogItem::Drop(1), // inserting 2 drops 1
DropLogItem::Drop(2), // 2 could not be inserted because dropping 1 panicked, so it is dropped as well
]
);
}

#[test]
#[should_panic = "insert_resource_by_id called with component id which doesn't exist in this world"]
fn insert_resource_by_id_invalid_component_id() {
Expand Down