Skip to content

Commit

Permalink
Avoid a panic when loading labelled assets (bevyengine#13506)
Browse files Browse the repository at this point in the history
# Objective

- Fixes bevyengine#10820.

## Solution

- Check that the asset ID to be inserted is still being managed.
- Since this route is only used by `AssetServer`-tracked handles, if the
`infos` map no longer contains the asset ID, all handles must have been
dropped. In this case, since nobody can be watching for the result,
we're safe to bail out. This avoids the panic when inserting the asset,
because when the handles are dropped, its slot in `Assets<A>` is
poisoned.
- Someone may be waiting for a labelled asset rather than the main
asset, these are handled with separate calls to `process_asset_load`, so
shouldn't cause any issues.
- Removed the workaround keeping asset info alive after the handle has
died, since we should no longer be trying to operate on any assets once
their handles have been dropped.

## Testing

- I added a `break` in `handle_internal_asset_events`
(`crates/bevy_asset/src/server/mod.rs` on line 1152). I don't believe
this should affect correctness, only efficiency, since it is effectively
only allowing one asset event to be handled per frame. This causes
examples like `animated_fox` to produce the issue fairly frequently.
- I wrote a small program which called `AssetServer::reload` and could
trigger it too.

---

## Changelog
- Fixed an issue which could cause a panic when loading an asset which
was no longer referenced.

---

## Remaining Work

~This needs more testing. I don't yet have a complete project that
reliably crashes without changes to bevy.~ We have at least one vote of
confidence so far from @Testare who had a project broken by this bug.

@cart, (sorry for the ping), I believe you added the code which delays
`remove_dropped`. Was there any other reason `track_assets` needed to
keep the dropped assets alive?
  • Loading branch information
ricky26 authored Jun 5, 2024
1 parent 2165f22 commit 9a123cd
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 16 deletions.
17 changes: 1 addition & 16 deletions crates/bevy_asset/src/assets.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
use crate::{self as bevy_asset};
use crate::{
Asset, AssetEvent, AssetHandleProvider, AssetId, AssetServer, Handle, LoadState, UntypedHandle,
};
use crate::{Asset, AssetEvent, AssetHandleProvider, AssetId, AssetServer, Handle, UntypedHandle};
use bevy_ecs::{
prelude::EventWriter,
system::{Res, ResMut, Resource},
Expand Down Expand Up @@ -545,18 +543,11 @@ impl<A: Asset> Assets<A> {
// re-loads are kicked off appropriately. This function must be "transactional" relative
// to other asset info operations
let mut infos = asset_server.data.infos.write();
let mut not_ready = Vec::new();
while let Ok(drop_event) = assets.handle_provider.drop_receiver.try_recv() {
let id = drop_event.id.typed();

if drop_event.asset_server_managed {
let untyped_id = id.untyped();
if let Some(info) = infos.get(untyped_id) {
if let LoadState::Loading | LoadState::NotLoaded = info.load_state {
not_ready.push(drop_event);
continue;
}
}

// the process_handle_drop call checks whether new handles have been created since the drop event was fired, before removing the asset
if !infos.process_handle_drop(untyped_id) {
Expand All @@ -568,12 +559,6 @@ impl<A: Asset> Assets<A> {
assets.queued_events.push(AssetEvent::Unused { id });
assets.remove_dropped(id);
}

// TODO: this is _extremely_ inefficient find a better fix
// This will also loop failed assets indefinitely. Is that ok?
for event in not_ready {
assets.handle_provider.drop_sender.send(event).unwrap();
}
}

/// A system that applies accumulated asset change events to the [`Events`] resource.
Expand Down
5 changes: 5 additions & 0 deletions crates/bevy_asset/src/server/info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,11 @@ impl AssetInfos {
world: &mut World,
sender: &Sender<InternalAssetEvent>,
) {
// Check whether the handle has been dropped since the asset was loaded.
if !self.infos.contains_key(&loaded_asset_id) {
return;
}

loaded_asset.value.insert(loaded_asset_id, world);
let mut loading_deps = loaded_asset.dependencies;
let mut failed_deps = HashSet::new();
Expand Down

0 comments on commit 9a123cd

Please sign in to comment.