Commands queued by component hooks are not applied during exclusive world access #16034
Labels
A-ECS
Entities, components, systems, and events
C-Bug
An unexpected or incorrect behavior
D-Complex
Quite challenging from either a design or technical perspective. Ask for help!
S-Needs-Design
This issue requires design work to think about how it would best be accomplished
Milestone
Comments
alice-i-cecile
added
A-ECS
|
Of course, it's not clear that this is fully undesirable behavior! Better control over when hooks are applied in the context of exclusive world access exposes more power for users, particularly around performance optimizations (e.g. #10154). We could decide that this is intended behavior, and simply clearly document it. Relying on fully valid hierarchies for soundness is very challenging already (see #15575), we should consider making this code robust or marking the system itself as unsafe. |
From @james-j-obrien on Discord. |
alice-i-cecile
changed the title
This was referenced Nov 2, 2024
github-merge-queue bot
pushed a commit
that referenced
this issue
Dec 5, 2024
# Objective - Currently adding observers spawns an entity which implicitly flushes the command queue, which can cause undefined behaviour if the `WorldEntityMut` is used after this - The reason `WorldEntityMut` attempted to (unsuccessfully) avoid flushing commands until finished was that such commands may move or despawn the entity being referenced, invalidating the cached location. - With the introduction of hooks and observers, this isn't sensible anymore as running the commands generated by hooks immediately is required to maintain correct ordering of operations and to not expose the world in an inconsistent state - Objective is to make command flushing deterministic and fix the related issues - Fixes #16212 - Fixes #14621 - Fixes #16034 ## Solution - Allow `WorldEntityMut` to exist even when it refers to a despawned entity by allowing `EntityLocation` to be marked invalid - Add checks to all methods to panic if trying to access a despawned entity - Flush command queue after every operation that might trigger hooks or observers - Update entity location always after flushing command queue ## Testing - Added test cases for currently broken behaviour - Added test cases that flushes happen in all operations - Added test cases to ensure hooks and commands are run exactly in correct order when nested --- Todo: - [x] Write migration guide - [x] Add tests that using `EntityWorldMut` on a despawned entity panics - [x] Add tests that commands are flushed after every operation that is supposed to flush them - [x] Add tests that hooks, observers and their spawned commands are run in the correct order when nested --- ## Migration Guide Previously `EntityWorldMut` triggered command queue flushes in unpredictable places, which could interfere with hooks and observers. Now the command queue is flushed always immediately after any call in `EntityWorldMut` that spawns or despawns an entity, or adds, removes or replaces a component. This means hooks and observers will run their commands in the correct order. As a side effect, there is a possibility that a hook or observer could despawn the entity that is being referred to by `EntityWorldMut`. This could already currently happen if an observer was added while keeping an `EntityWorldMut` referece and would cause unsound behaviour. If the entity has been despawned, calling any methods which require the entity location will panic. This matches the behaviour that `Commands` will panic if called on an already despawned entity. In the extremely rare case where taking a new `EntityWorldMut` reference or otherwise restructuring the code so that this case does not happen is not possible, there's a new `is_despawned` method that can be used to check if the referred entity has been despawned.
ecoskey
pushed a commit
to ecoskey/bevy
that referenced
this issue
Jan 6, 2025
…6219) # Objective - Currently adding observers spawns an entity which implicitly flushes the command queue, which can cause undefined behaviour if the `WorldEntityMut` is used after this - The reason `WorldEntityMut` attempted to (unsuccessfully) avoid flushing commands until finished was that such commands may move or despawn the entity being referenced, invalidating the cached location. - With the introduction of hooks and observers, this isn't sensible anymore as running the commands generated by hooks immediately is required to maintain correct ordering of operations and to not expose the world in an inconsistent state - Objective is to make command flushing deterministic and fix the related issues - Fixes bevyengine#16212 - Fixes bevyengine#14621 - Fixes bevyengine#16034 ## Solution - Allow `WorldEntityMut` to exist even when it refers to a despawned entity by allowing `EntityLocation` to be marked invalid - Add checks to all methods to panic if trying to access a despawned entity - Flush command queue after every operation that might trigger hooks or observers - Update entity location always after flushing command queue ## Testing - Added test cases for currently broken behaviour - Added test cases that flushes happen in all operations - Added test cases to ensure hooks and commands are run exactly in correct order when nested --- Todo: - [x] Write migration guide - [x] Add tests that using `EntityWorldMut` on a despawned entity panics - [x] Add tests that commands are flushed after every operation that is supposed to flush them - [x] Add tests that hooks, observers and their spawned commands are run in the correct order when nested --- ## Migration Guide Previously `EntityWorldMut` triggered command queue flushes in unpredictable places, which could interfere with hooks and observers. Now the command queue is flushed always immediately after any call in `EntityWorldMut` that spawns or despawns an entity, or adds, removes or replaces a component. This means hooks and observers will run their commands in the correct order. As a side effect, there is a possibility that a hook or observer could despawn the entity that is being referred to by `EntityWorldMut`. This could already currently happen if an observer was added while keeping an `EntityWorldMut` referece and would cause unsound behaviour. If the entity has been despawned, calling any methods which require the entity location will panic. This matches the behaviour that `Commands` will panic if called on an already despawned entity. In the extremely rare case where taking a new `EntityWorldMut` reference or otherwise restructuring the code so that this case does not happen is not possible, there's a new `is_despawned` method that can be used to check if the referred entity has been despawned.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Problem
When an entity is spawned (or other structural changes occur) as part of exclusive world access, commands to apply changes via hooks are generated, but are not immediately applied.
In order to apply these commands, we must call
world.flush, as demonstrated here.This allows users to accidentally observe the state of the world before component lifecycle hooks are applied, but only when working with exclusive world access. As a result, we must be extremely careful if we are relying on hooks for soundness.
Bevy version
0.14.1
Additional context
This is related to #14621, but is intended as a clearer demonstration of a specific problem.
#12235 may end up being blocked on this, as otherwise we could get UB by calling unsafe code on an invalid entity hierarchy.
The text was updated successfully, but these errors were encountered: