Fix hot reloads of a folder resulting in deadlocks. (#23980)

# Objective

- Fixes #23954.

## Solution

- Don't lock the infos RwLock inside load_folder_internal. Instead at
every callsite (2 callsites), count the number of started loads with
whatever locks we have.
- Move the load_folder_internal calls until after we drop the `infos`.

## Testing

- Added a test for this.

Author: andriyDev

crates/bevy_asset/src/lib.rs

@@ -750,6 +750,7 @@ mod tests {
     use bevy_tasks::block_on;
     use core::{any::TypeId, time::Duration};
     use futures_lite::AsyncReadExt;
+    use ron::ser::PrettyConfig;
     use serde::{Deserialize, Serialize};
     use std::path::{Path, PathBuf};
     use thiserror::Error;
@@ -993,6 +994,20 @@ mod tests {
         storage.0.extend(reader.read().cloned());
     }
 
+    /// Serializes `text` into a `CoolText` that can be loaded.
+    ///
+    /// This doesn't support all the features of `CoolText`, so more complex scenarios may require
+    /// doing this manually.
+    pub(crate) fn serialize_as_cool_text(text: &str) -> String {
+        let cool_text_ron = CoolTextRon {
+            text: text.into(),
+            dependencies: vec![],
+            embedded_dependencies: vec![],
+            sub_texts: vec![],
+        };
+        ron::ser::to_string_pretty(&cool_text_ron, PrettyConfig::new().new_line("\n")).unwrap()
+    }
+
     #[test]
     fn load_dependencies() {
         let dir = Dir::default();
@@ -3217,4 +3232,102 @@ mod tests {
             asset_server.are_direct_dependencies_loaded(app.world().resource::<MyAssetHolder>())
         );
     }
+
+    #[test]
+    fn hot_reload_folder() {
+        let (mut app, dir, event_sender) = create_app_with_source_event_sender();
+
+        app.init_asset::<CoolText>()
+            .init_asset::<SubText>()
+            .register_asset_loader(CoolTextLoader);
+
+        let abc_path = Path::new("dir/abc.cool.ron");
+        let def_path = Path::new("dir/def.cool.ron");
+        dir.insert_asset_text(abc_path, &serialize_as_cool_text("abc"));
+        dir.insert_asset_text(def_path, &serialize_as_cool_text("def"));
+
+        let asset_server = app.world().resource::<AssetServer>().clone();
+
+        let folder_handle = asset_server.load_folder("dir");
+        run_app_until(&mut app, |_| {
+            asset_server
+                .is_loaded_with_dependencies(&folder_handle)
+                .then_some(())
+        });
+
+        let folder = app
+            .world()
+            .resource::<Assets<LoadedFolder>>()
+            .get(&folder_handle)
+            .unwrap();
+        assert_eq!(folder.handles.len(), 2);
+        let mut handles = folder
+            .handles
+            .iter()
+            .cloned()
+            .map(UntypedHandle::typed::<CoolText>)
+            .collect::<Vec<_>>();
+        // Sort the handles so we know abc is first and def is second.
+        handles.sort_by_key(|handle| handle.path().unwrap().path().to_path_buf());
+
+        let abc_handle = handles[0].clone();
+        let def_handle = handles[1].clone();
+
+        let cool_texts = app.world().resource::<Assets<CoolText>>();
+        assert_eq!(cool_texts.get(&abc_handle).unwrap().text, "abc");
+        assert_eq!(cool_texts.get(&def_handle).unwrap().text, "def");
+
+        // Before doing any hot reloading stuff, clear out any AssetEvent messages.
+        app.world_mut()
+            .resource_mut::<Messages<AssetEvent<LoadedFolder>>>()
+            .clear();
+
+        // Add a new asset to the folder, and send an event to trigger hot-reloading.
+        let ghi_path = Path::new("dir/ghi.cool.ron");
+        dir.insert_asset_text(ghi_path, &serialize_as_cool_text("ghi"));
+        event_sender
+            .send_blocking(AssetSourceEvent::AddedAsset(ghi_path.to_path_buf()))
+            .unwrap();
+
+        run_app_until(&mut app, |world| {
+            for event in world
+                .resource_mut::<Messages<AssetEvent<LoadedFolder>>>()
+                .drain()
+            {
+                if let AssetEvent::LoadedWithDependencies { id } = event
+                    && id == folder_handle.id()
+                {
+                    return Some(());
+                }
+            }
+            None
+        });
+
+        let folder = app
+            .world()
+            .resource::<Assets<LoadedFolder>>()
+            .get(&folder_handle)
+            .unwrap();
+        assert_eq!(folder.handles.len(), 3);
+        let mut handles = folder
+            .handles
+            .iter()
+            .cloned()
+            .map(UntypedHandle::typed::<CoolText>)
+            .collect::<Vec<_>>();
+        // Sort the handles so we know the order is abc, def, and ghi.
+        handles.sort_by_key(|handle| handle.path().unwrap().path().to_path_buf());
+
+        let new_abc_handle = handles[0].clone();
+        let new_def_handle = handles[1].clone();
+        let new_ghi_handle = handles[2].clone();
+
+        assert_eq!(new_abc_handle, abc_handle);
+        assert_eq!(new_def_handle, def_handle);
+
+        let cool_texts = app.world().resource::<Assets<CoolText>>();
+        assert_eq!(cool_texts.get(&new_abc_handle).unwrap().text, "abc");
+        assert_eq!(cool_texts.get(&new_def_handle).unwrap().text, "def");
+        assert_eq!(cool_texts.get(&new_ghi_handle).unwrap().text, "ghi");
+    }
 }

crates/bevy_asset/src/processor/tests.rs

@@ -34,8 +34,8 @@ use crate::{
     },
     saver::{tests::CoolTextSaver, AssetSaver},
     tests::{
-        read_asset_as_string, read_meta_as_string, run_app_until, CoolText, CoolTextLoader,
-        CoolTextRon, SubText,
+        read_asset_as_string, read_meta_as_string, run_app_until, serialize_as_cool_text, CoolText,
+        CoolTextLoader, CoolTextRon, SubText,
     },
     transformer::{AssetTransformer, TransformedAsset},
     Asset, AssetApp, AssetLoader, AssetMode, AssetPath, AssetPlugin, LoadContext,
@@ -223,20 +223,6 @@ impl<R: AssetReader> AssetReader for LockGatedReader<R> {
     }
 }
 
-/// Serializes `text` into a `CoolText` that can be loaded.
-///
-/// This doesn't support all the features of `CoolText`, so more complex scenarios may require doing
-/// this manually.
-fn serialize_as_cool_text(text: &str) -> String {
-    let cool_text_ron = CoolTextRon {
-        text: text.into(),
-        dependencies: vec![],
-        embedded_dependencies: vec![],
-        sub_texts: vec![],
-    };
-    ron::ser::to_string_pretty(&cool_text_ron, PrettyConfig::new().new_line("\n")).unwrap()
-}
-
 /// Sets the transaction log for the app to a fake one to prevent touching the filesystem.
 fn set_fake_transaction_log(app: &mut App) {
     /// A dummy transaction log factory that just creates [`FakeTransactionLog`].

crates/bevy_asset/src/server/mod.rs

@@ -1142,6 +1142,7 @@ impl AssetServer {
         }
         // `get_or_create_path_handle` always returns a Strong variant, so this is safe.
         let index = (&handle).try_into().unwrap();
+        self.write_infos().stats.started_load_tasks += 1;
         self.load_folder_internal(index, path);
 
         handle
@@ -1186,8 +1187,6 @@ impl AssetServer {
             Ok(())
         }
 
-        self.write_infos().stats.started_load_tasks += 1;
-
         let path = path.into_owned();
         let server = self.clone();
         IoTaskPool::get()
@@ -2155,45 +2154,51 @@ pub fn handle_internal_asset_events(world: &mut World) {
             }
         }
 
-        let reload_parent_folders = |path: &PathBuf, source: &AssetSourceId<'static>| {
-            for parent in path.ancestors().skip(1) {
-                let parent_asset_path =
-                    AssetPath::from(parent.to_path_buf()).with_source(source.clone());
-                for folder_handle in infos.get_path_handles(&parent_asset_path) {
-                    info!("Reloading folder {parent_asset_path} because the content has changed");
-                    // `get_path_handles` only returns Strong variants, so this is safe.
-                    let index = (&folder_handle).try_into().unwrap();
-                    server.load_folder_internal(index, parent_asset_path.clone());
+        let mut folders_to_reload = Vec::default();
+        let mut reload_parent_folders =
+            |path: &PathBuf, source: &AssetSourceId<'static>, infos: &mut AssetInfos| {
+                let mut new_loads = 0;
+                for parent in path.ancestors().skip(1) {
+                    let parent_asset_path =
+                        AssetPath::from(parent.to_path_buf()).with_source(source.clone());
+                    for folder_handle in infos.get_path_handles(&parent_asset_path) {
+                        info!(
+                            "Reloading folder {parent_asset_path} because the content has changed"
+                        );
+                        new_loads += 1;
+                        folders_to_reload.push((folder_handle, parent_asset_path.clone()));
+                    }
                 }
-            }
-        };
+                infos.stats.started_load_tasks += new_loads;
+            };
 
         let mut paths_to_reload = <HashSet<_>>::default();
-        let mut reload_path = |path: PathBuf, source: &AssetSourceId<'static>| {
-            let path = AssetPath::from(path).with_source(source);
-            queue_ancestors(&path, &infos, &mut paths_to_reload);
-            paths_to_reload.insert(path);
-        };
+        let mut reload_path =
+            |path: PathBuf, source: &AssetSourceId<'static>, infos: &AssetInfos| {
+                let path = AssetPath::from(path).with_source(source);
+                queue_ancestors(&path, infos, &mut paths_to_reload);
+                paths_to_reload.insert(path);
+            };
 
         let mut handle_event = |source: AssetSourceId<'static>, event: AssetSourceEvent| {
             match event {
                 AssetSourceEvent::AddedAsset(path) => {
-                    reload_parent_folders(&path, &source);
-                    reload_path(path, &source);
+                    reload_parent_folders(&path, &source, &mut infos);
+                    reload_path(path, &source, &infos);
                 }
                 // TODO: if the asset was processed and the processed file was changed, the first modified event
                 // should be skipped?
                 AssetSourceEvent::ModifiedAsset(path) | AssetSourceEvent::ModifiedMeta(path) => {
-                    reload_path(path, &source);
+                    reload_path(path, &source, &infos);
                 }
                 AssetSourceEvent::RenamedFolder { old, new } => {
-                    reload_parent_folders(&old, &source);
-                    reload_parent_folders(&new, &source);
+                    reload_parent_folders(&old, &source, &mut infos);
+                    reload_parent_folders(&new, &source, &mut infos);
                 }
                 AssetSourceEvent::RemovedAsset(path)
                 | AssetSourceEvent::RemovedFolder(path)
                 | AssetSourceEvent::AddedFolder(path) => {
-                    reload_parent_folders(&path, &source);
+                    reload_parent_folders(&path, &source, &mut infos);
                 }
                 _ => {}
             }
@@ -2223,6 +2228,11 @@ pub fn handle_internal_asset_events(world: &mut World) {
         #[cfg(any(target_arch = "wasm32", not(feature = "multi_threaded")))]
         drop(infos);
 
+        for (handle, path) in folders_to_reload {
+            // `get_path_handles` only returns Strong variants, so this is safe.
+            let index = (&handle).try_into().unwrap();
+            server.load_folder_internal(index, path);
+        }
         for path in paths_to_reload {
             server.reload_internal(path, true);
         }