Registered systems that unregister themselves panic

[CorvusPrudens]

Bevy version and features

bevy_ecs@0.18.0-rc2

What you did

Reproduction repo

use bevy_ecs::{lifecycle::HookContext, prelude::*, system::SystemId, world::DeferredWorld};

fn main() {
    let mut world = World::new();

    let system_cleanup = world.spawn_empty().id();
    let system_id = world.register_system(move |mut commands: Commands| {
        commands.entity(system_cleanup).despawn();
    });
    world
        .entity_mut(system_cleanup)
        .insert(SystemCleanup(system_id));

    world.run_system(system_id).unwrap();
}

#[derive(Component)]
#[component(on_replace = on_replace)]
struct SystemCleanup(SystemId);

fn on_replace(mut world: DeferredWorld, context: HookContext) {
    let system = world.get_mut::<SystemCleanup>(context.entity).unwrap().0;
    world.commands().unregister_system(system);
}

While this may seem contrived at first glance, it's a pretty useful pattern in my code base for ephemeral, self-cleaning event handlers.

What went wrong

thread 'main' (22524110) panicked at /Users/corvus/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/bevy_ecs-0.18.0-rc.2/src/world/entity_access/world_mut.rs:1806:14:
Attempted to update the location of a despawned entity, which is impossible. This was the result of performing an operation on this EntityWorldMut that queued a despawn command: InvalidEntityError { entity: 1v0, current_generation: EntityGeneration(1) }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Discussion

As far as I can tell, the main contributing factor to this bug was introduced in #19451, where update_location can now panic. I tried a trivial fix for the offending insert:

        if let Ok(mut entity) = self.get_entity_mut(id.entity)
            && !entity.is_despawned()
        {
            entity.insert::<RegisteredSystem<I, O>>(RegisteredSystem {
                initialized,
                system,
            });
        }

But this can't actually determine whether the queued commands will despawn it. I believe the despawning commands are flushed here, so there's no opportunity to check before updating the location.

A proper fix isn't obvious to me. Without larger changes to update_location such as making it fallible, the only solution I can see is simply not panicking (assuming it's not preserving any safety invariants). Naturally, not panicking poses its own issues.

[ElliottjPierce]

Yikes. Yeah, not a fun problem.

Let me shed some light on the rug that I pulled out from under you. After #19451, a "despawn" is represented as an entity with no location. The entity still exists. That is, it's id is still the same, and you can still access it to, for example, respawn the entity. Only when we free the entity does the id change, making World::get_entity_mut fail. Previously, The despawn was represented by immediately changing the id, which had its own problems.

In my initial design, I wanted to make a lot of the location methods fallible, but that was thrown out in review due to migration concerns. (Understandably.) So, without the individual methods being fallible, you have to check for them ahead of time, which I think actually is the design direction reviewers preferred, as we expected cases where this even mattered to be quite rare.

Still, in Bevy's system running code, we have to think of everything. Here's the thing: We need EntityWorldMut to support accessing entities that aren't spawned yet. That's one of the major goals of my entity work and is the most outstanding requirement for assets as entities, etc. Or at least we need some way of interacting with this kind of entity for those uses. So we can't just get rid of the None location issues, and we can't just make it fallible for the users, as much as I'd like to.

Here's some options:

1. Add some docs reminding people to use is_despawned before doing certain actions on an EntityWorldMut. This makes me gag because it goes against the errors as values principle, but we could choose to consider it a program error (which normally does just panic in rust) and keep the panic behavior. That would mean adding your fix above and just telling people, "Make sure it's actually spawned or this could crash".
2. Make World::get_entity_mut only return entities that are spawned, and add a World::get_maybe_spawned_entity_mut or something to handle other cases. Maybe even add a MaybeSpawnedEntityWorldMut or something. This is a lot more code to maintain for what feels like a pretty rare case: using an EntityWorldMut that could magically be despawned from unknown code. I think the fact that it was only just now discovered in a real use case is telling. (Also, really cool use case btw; I've never seen that technique.) Anyway, this option makes me want to scream into a pillow, because we need to write and then maintain double the code for EntityWorldMut stuff for a very rare issue.
3. We could make World::get_entity_mut return a Result<EntityWorldMut, EntityWorldError>, where EntityWorldError had a variant InvalidEntityId and EntityNotSpawned(EntityWorldMut), kinda like the errors for RwLock's try_write or something. This is a little more code, but not too bad; it's mostly just a more involved WorldEntityFetch. It would also mean a lifetime in the Err variant, which I think is frowned upon. This also means that we have more migration concerns again. Not as much as making the location methods fallible, but still more than I think would be appreciated for a release candidate.

We can mix and match these options, too, but I don't see a good option here, unfortunately.

Personally, my vote would be to do the first option for now and then do some version of the third option in the future. But I'm all for other ideas! None of mine seem good to me lol.

[CorvusPrudens]

Thanks for looking into it!

To be clear, my attempted fix doesn't work. The repro still panics.

I believe this is because the EntityWorldMut is fetched before the commands generated by the system are applied, and they're completely opaque when the entity is fetched. As far as the EntityWorldMut is concerned, it is valid and not despawned right up until this call.

Even if we check is_despawned just before the call to update_location, it won't catch the issue. Flushing those commands (and thus despawning the entity) doesn't update the existing EntityWorldMut's location. I think you'd have to check self.world.entities().get(self.entity).is_ok(), which doesn't feel great since update_location will then immediately use the value from self.world.entities().get(self.entity).

In other words, I think the only good solution in this specific case would be making update_location fallible. I'm sure that would not be a small change as you mentioned, and it seems especially problematic to attempt during the rc.

With that said, I think the attempt here to wait to apply commands until the system state is reinserted is thwarted by this inner call to World::flush. The outer flush seems to be useless if the insert succeeds.

If the commands are applied before the entity is fetched, then it doesn't panic (this branch is not taken). At the same time, it clearly denies the possibility of running a system recursively as the comment suggests. The fact that recursive calling does work seems almost like an accident. Checking the callstack of a recursive registered system call, it does indeed appear that this flush within insert is the point of recursion, which runs before the bundle effects of the insertion are even applied. Maybe this was known at the time, but it seems a bit odd at least.

[ElliottjPierce]

Thanks for looking into it!

To be clear, my attempted fix doesn't work. The repro still panics.

I believe this is because the EntityWorldMut is fetched before the commands generated by the system are applied, and they're completely opaque when the entity is fetched. As far as the EntityWorldMut is concerned, it is valid and not despawned right up until this call.

Even if we check is_despawned just before the call to update_location, it won't catch the issue. Flushing those commands (and thus despawning the entity) doesn't update the existing EntityWorldMut's location. I think you'd have to check self.world.entities().get(self.entity).is_ok(), which doesn't feel great since update_location will then immediately use the value from self.world.entities().get(self.entity).

Oh, I hadn't realized that, but that makes sense to me.

In other words, I think the only good solution in this specific case would be making update_location fallible. I'm sure that would not be a small change as you mentioned, and it seems especially problematic to attempt during the rc.

I agree that this is the best solution, but I got a lot of pushback here originally, and yeah, doesn't look good in an RC. Here's what Cart had said: "I concede that there is the "entity could be despawned by commands" path on EntityWorldMut (which we already handled), but I'm content treating that as an "invalidation" of the "spawned state machine", rather than trying to encode both the "spawned state" and the "despawned state" as valid paths here." The "here" being by making update_location deal in Option<EntityLocation> terms. He actually has a lot of reasons for this you can see here and in his other comments. I still disagree, but he absolutely has valid points for this decision.

With that said, I think the attempt here to wait to apply commands until the system state is reinserted is thwarted by this inner call to World::flush. The outer flush seems to be useless if the insert succeeds.

If the commands are applied before the entity is fetched, then it doesn't panic (this branch is not taken). At the same time, it clearly denies the possibility of running a system recursively as the comment suggests. The fact that recursive calling does work seems almost like an accident. Checking the callstack of a recursive registered system call, it does indeed appear that this flush within insert is the point of recursion, which runs before the bundle effects of the insertion are even applied. Maybe this was known at the time, but it seems a bit odd at least.

This seems like a better place to explore to me. Maybe changing how systems are run is the better option here. That said, I have practically no knowledge of how Bevy does systems. I've done quite a lot with entities, and a little with components and world storage backends, but systems are still somewhat alien to me. So, I can't advise much on this path. But it does seem best just because of process of elimination.

[cart]

Did some thinking about this today: wrapping RegisteredSystem::system in Option and replacing the RegisteredSystem removal/insert code with taking the value from the option fixes this particular crash, as it allows us to add/remove the system without triggering a flush.

Not my favorite, as I like that RegisteredSystem's presence indicates that the system is "active" and "callable". But that is largely just an aesthetic preference.