Asset locking

[andriyDev]

Objective

• Implements Asset locking #15346.

Solution

• This is pretty much the same as the design proposed in Asset locking #15346.

Testing

• A basic test in bevy_asset.

---

Showcase

fn my_task_spawning_system(my_mesh_handle: Res<MyMeshHandle>, mut meshes: ResMut<Assets<Mesh>>) {
    let handle: Handle<Mesh> = my_mesh_handle.0.clone();
    let mesh_arc: Arc<Mesh> = meshes.lock(&handle);

    AsyncComputeTaskPool::get().spawn(move async || {
        // Do something with the mesh_arc.
        std::thread::sleep(std::time::Duration::from_secs(mesh_arc.count_vertices()));
        println!("Done processing!");
    });
}

The primary new parts of the API are:

• Assets::lock - Locks the asset if it exists
• Assets::try_unlock - Attempts to unlock the asset if there are no more outstanding handles (otherwise the asset remains locked).

Migration Guide

• Assets::get_or_insert_with may now return an error if the asset is locked.
• Assets::get_mut may now return an error if the asset is locked (in addition to if the asset is missing). Consider using Assets::get_mut_or_clone if your type is Clone to avoid handling locking.
• Assets::remove and Assets::remove_untracked now return an AssetPresence<A>. This requires you to handle the case where an asset is locked.
• Assets::iter_mut now returns an (AssetId<A>, AssetPresenceMut<'a, A>) instead of (AssetId<A>, &'a mut A). This allows you to handle the locked case.
• ReflectAsset::get_mut and ReflectAsset::get_unchecked_mut may now return an error if the asset is locked (in addition to if the asset is missing).
• ReflectAsset::remove now returns a ReflectedAssetPresence to store both the locked and unlocked assets.

[andriyDev]

Just rebased onto main.

[BenjaminBrienen]

Very clean code and easy to follow!

[alice-i-cecile]

The code is really well-made: commented, clearly engineered, great use of enums. Can you add an example demonstrating how (and especially why) a user might use this API?

Once that's in place, I'm on board and am willing to merge.

[cart]

Left some thoughts in the issue: #15346 (comment)

[andriyDev]

I rebased (#15281 had a big blast radius), and also included a new example to show off how you can use asset locking.

I still need to figure out how to implement get_mut_or_clone as mentioned in #15346. I'm running into borrow checker issues (something something polonius something). I'll take another stab at that tomorrow.

[andriyDev]

Ok, I've implemented get_mut_or_clone, and all the examples now use it. I've updated the migration guide to point users to get_mut_or_clone as well.

One thing that's kinda rough here is that iter_mut doesn't allow you to "clone to unlock". I'm not sure if this is worthwhile to solve, it may require a whole new struct to wrap around AssetPresenceMut, and that seems a little overcomplicated.

[bushrat011899]

I think this is a very interesting concept to add to Assets and opens up further discussion around other systems. I've left some notes with possible future work, but I think as-is this is good to merge.

[bushrat011899]

Future PR: a more fundamental version of the function could be

pub fn get_mut_or_replace_with(
    &mut self,
    id: impl Into<AssetId<A>>,
    f: impl FnOnce(Option<&A>) -> A
) -> Option<&mut A>;

Which would allow the user to decide how to replace a locked asset, remove the A: Clone requirement, and handling the Missing case.

[bushrat011899]

Future PR: since the only way to lock an Asset is via this method, using #[track_caller] and std::panic::Location::caller(), we could store a Vec<Location> in the Locked variant, allowing the try_unlock method to return an error indicating where the Asset has been locked from. I believe the #[track_caller] attribute would need to be added to all the methods that forward to lock as well (e.g., Assets::lock)

I imagine we'd want such a thing in debug builds only, since it adds more weight to the AssetPresence type, but I imagine it would be quite helpful for diagnosing possibly frustrating deadlocks.

[andriyDev]

This is an interesting idea, but it's not clear to me how big a win this is. For one, the list of lock locations can only grow (until an asset is finally unlocked). I suppose if we assume locking is infrequent and short-lived this isn't a big deal, but I can totally see an asset being locked for a long time and constantly getting more and more lock locations. For another, that only tracks the initial lock location, but you're free to clone the Arc as much as you want. I suspect the more dangerous situation is something storing the Arc without knowing it's a locked asset and blocking mutations that way (and there's no real way to solve this AFAICT).

Regardless, this definitely seems worthy of a discussion and I could be convinced!

[bushrat011899]

Yeah it's definitely not perfect, but it could help with at least giving the user a starting point for debugging how an asset deadlock started. Potentially, we could even wrap the returned Arc to give it better debugging functionality. Storing a HashMap of Location's on the heap and counting down each location as the wrapped Arc is dropped, and counting up as the Arc is cloned.

Definitely half-baked and up for debate, and absolutely out of scope for this PR!

[alice-i-cecile]

Going to wait on @cart to take another look at this design: he had opinions in the linked thread and I don't feel comfortable merging this without him :)