[andriyDev]

Objective

• Fixes Dropping a handle from Assets::get_strong_handle has different behaviour to one from AssetServer::load. #20651.
• Alternative to unifying Asset<A> / AssetServer handles (#20651, #20776) #20874.
• Also a step towards Do not unload subassets or the root asset unless all subassets and root asset handles have been dropped. #21878.

Solution

• When allocating a new handle (whether for an existing asset or a new asset), send a "new" message to the same channel as "drop" events.
  • This essentially means we're "leaning in" to the fact that we can have multiple live handles to the same asset.
• Assets::track_assets now increments when a "new" message is received, and decrements when a "drop" message is received. The asset entry is dropped if the counter is at 0 after processing all messages.
• Remove the asset_server_managed flag, since we can just lookup the index to see if it's present.
• Remove the handle_drops_to_skip counter since now we count all new handles, not just those in get_strong_handle.

Testing

• Unbroke our reproduction test.
• Added a new test to verify that "reacquiring" an asset doesn't drop the asset.

[kristoff3r]

LGTM, the logic in track_assets felt a bit complicated but I couldn't find a way to simplify it.

[kristoff3r]

I don't see why this import needs to be inside this function.

[andriyDev]

This file defines a type Entry, so the hashmap Entry collides with it. Scoping it here sidesteps that issue.

[janis-bhm]

I might be wrong here, but isn't this technically a race condition? There are a bunch of lock-shaped things here, the &mut Assets<A> (which prevents get_strong_handle from being called during track_assets) and the write guard on the infos (which prevents access to the server's handle provider list, as well as load_* calls), but none of those actually prevent new HandleEvent::New(_) messages being sent or get_handle from being called to create a new strong handle.
I'm not sure if this is an actual problem: I don't think this is currently possible because the HandleProvider/Sender isn't exposed beyond the pub(crate) scope.

[andriyDev]

Yup that's exactly correct, the only thing preventing us from a race condition here is the fact that we have exclusive access to Assets and are locking the asset server - there's no other way to get a new handle to an existing asset. The only other way to send a HandleEvent::new is with AssetHandleProvider::reserve_handle, but that only gives you handles to a new asset, so we can't have accidentally dropped it yet.

I will have added a comment here to explain that. The existing comment about the lock being transactional is insufficient.

[janis-bhm]

I think it would be nice to find a way to get rid of the weak handle reference in the asset info and use the Asset<A>s new "strong count"; the weak handle is still used in is_path_alive and get_index_handle for checking liveness and getting the asset's handle, which will fail if the original handle was dropped and a new handle was created by the handle provider.

The modified test below fails in this PR:

Details

    #[test]
    fn asset_is_live_after_reacquired() {
        let (mut app, dir) = create_app();

        dir.insert_asset_text(Path::new("dep.cool.ron"), SIMPLE_TEXT);

        app.init_asset::<CoolText>()
            .init_asset::<SubText>()
            .register_asset_loader(CoolTextLoader);

        let asset_server = app.world().resource::<AssetServer>().clone();

        let handle = asset_server.load::<CoolText>("dep.cool.ron");
        run_app_until(&mut app, |_| asset_server.is_loaded(&handle).then_some(()));

        // Dropping the handle and then reacquiring it through a load should **not** result in the
        // asset being dropped. Note: we won't reload the asset in any case since we only try to
        // drop on the next update.
        let new_handle = app
            .world_mut()
            .resource_mut::<Assets<CoolText>>()
            .get_strong_handle(handle.id())
            .expect("should yield strong handle");
        drop(handle);

        app.update();

        assert!(app
            .world()
            .resource::<Assets<CoolText>>()
            .contains(&new_handle));

        let asset_server = app.world().resource::<AssetServer>().clone();
        assert!(asset_server.read_infos().is_path_alive("dep.cool.ron"));
        let index =
            ErasedAssetIndex::try_from(new_handle.id().untyped()).expect("should yield index");
        assert!(asset_server.read_infos().get_index_handle(index).is_some());
    }

[andriyDev]

@janis-bhm That was a good catch! I didn't know these methods existed. Now when you call AssetServer::get_id_handle, it will allocate a new handle if possible. We now ignore the weak_count entirely and just say if the entry still exists, you can get a handle to it. I also entirely removed the is_path_alive, inlined it in the only place its used, and simplified it to assume that if the entry exists, the path still exists.

@kristoff3r I've added some not entirely trivial changes, so I'd like another review just in case.

[cart]

Haven't done a review yet, but leaning in to multiple "handle lifetimes" doesn't feel very good to me personally. Adding Assets::get_strong_handle was a mistake I think. I think being able to store and access canonical state in the strong handle would provide significant utility.

Rather than rock the boat too much here, perhaps we should start thinking about the assets-as-entities future, and how handles will behave there.

[cart]

Although maybe fixing the current state of things, at the cost of the ideal future state, is worth it. 🤷 i'll do a review.

[andriyDev]

I am of the opposite opinion - handles should store almost no state IMO. That's because if in the future we want to support no_std, it may be the case that we allow users to entirely disable the AssetServer and allow users to "bring their own asset server", in which case, relying too much on state in the Handle won't work (unless we allow adding arbitrary state to a handle which is a little gross).

Also: the prior attempt at this was #20874 which just used a Mutex to avoid creating duplicate handles. Alice was not excited by the extra locking, though I don't think that's a big deal since the AssetServer already locks anyway.

[cart]

In an "assets as entities" world I do see a path where Handles are "just" handles to a specific Entity, which is decoupled from the asset system entirely (I've built that before actually). But even in that world, a single-shared canonical handle is valuable, as it allows us to easily count references (useful for debugging), and it allows us to treat the dropping of the handle as the "end" of the Entity. It also allows us to maximize the lifetime of Weak handles: if there is only ever one handle lifetime, then we can always successfully upgrade the Weak handle, provided there is still one active reference somewhere. In multi-handle, Weak::upgrade failing doesn't necessarily mean that the asset (or entity) is dropped. Plus multi-lifetime just requires more infrastructure / complexity / overhead and introduces more surface area for weirdness.

[andriyDev]

@cart That is indeed a simple policy but loses out on a nice benefit of the current approach: if you drop the last handle to an asset, but then call load in the same frame, you can "revive" that asset (it won't trigger another load and will just keep the existing asset). Maybe that's not that big of a win? For some reason I was really worried about losing this, but I don't remember why...

If that's a benefit we don't care about, then going to a simpler model seems fine. But I think it's worth making sure we don't care about this (or that there aren't obvious pitfalls).

[cart]

I was really worried about losing this, but I don't remember why...

I do remember something in this category coming up. You might be thinking of AssetLoader implementations that choose to call load() to kick off the load of an asset dependency, but skip holding the handle for "ergonomics", and then later call get_handle?

I believe we resolved those cases by holding a strong handle in the loader context / LoadedAsset (ensuring dependencies stay alive at least until the load is ingested into the world). But also, that pattern should be discouraged :)

Maybe that's not that big of a win

I don't personally see that as a big win. The odds of the drop and load occurring in the same frame are slim, and cases that actually care about this should write logic to keep the handle alive until the point where it is no longer needed, rather than relying on the much less clear and firm implementation detail that there is one frame to "catch" it if you drop it.

However, the actual "drop" of the asset will still need to be deferred, as it requires world access (also true for assets as entities), so we still have the chance to "revive" the asset / entity with a new handle (if we really want it to work that way). The current "should I create a new handle lifetime" code in get_or_create_path_handle_internal does exactly that, and was built with the "single canonical handle" case in mind. That type of "revival" only works if the state stored on the handle can be initialized without World access. But that needs to be true anyway!

In that world, if we choose to store "canonical" state on the handle itself (ex: make LoadState an atomic and store it in the Arc), then the drop-and-revive would lose that state. So it would either need to be recoverable (ex: the handle writes that state to a shared concurrent data structure on drop, the shared state has its own Arc that the AssetServer holds separately, etc) or it needs to be default-able / initializable only from the AssetServer context.

That being said, it probably makes sense to start without storing canonical state on the handle, especially if we're building a generic "entity handle" abstraction.

[kristoff3r]

Small nit for the inner loop, but other than that the PR still seems good

[kristoff3r]

Suggested change

	if !assets.try_remove_dropped(index) {
	// We didn't actually drop the asset entry, so there's nothing more to do.
	continue;
	}
	// Notify the AssetServer of the asset entry drop.
	infos.process_asset_entry_drop(ErasedAssetIndex {
	index,
	type_id: TypeId::of::<A>(),
	});
	dropped_entry = true;
	if assets.try_remove_dropped(index) {
	// Notify the AssetServer of the asset entry drop.
	infos.process_asset_entry_drop(ErasedAssetIndex {
	index,
	type_id: TypeId::of::<A>(),
	});
	dropped_entry = true;
	}

[janis-bhm]

LGTM.

I've added a few comments mainly just documenting my thought process and a few nits, I guess.

[janis-bhm]

Am I reading this correctly as get_path_indices inlined because of lifetime issues?

[janis-bhm]

this is a bit annoying, but if Assets as Entities changes all of this (asset handles) I think it's definitely acceptable and shouldn't stand in the way of fixing the original issue since nothing bad happens, just something redundant.

[janis-bhm]

Maybe this could get a comment explaining why it's &mut now? get_index_handle sounds like some immutable operation, but since the asset behind the index may no longer have a handle, this could actually create a new handle.
Publicly facing, this I think only results in AssetServer::get_handle and friends making an asset live-again if the method is called on a "dangling" asset, which I think is actually the desired behaviour.