From 85a10eccc5bee7e15643888975da86bb3f6a0ff6 Mon Sep 17 00:00:00 2001 From: Theia Vogel Date: Tue, 6 Jul 2021 17:35:16 +0000 Subject: [PATCH] Fix AssetServer::get_asset_loader deadlock (#2395) # Objective Fixes a possible deadlock between `AssetServer::get_asset_loader` / `AssetServer::add_loader` A thread could take the `extension_to_loader_index` read lock, and then have the `server.loader` write lock taken in add_loader before it can. Then add_loader can't take the extension_to_loader_index lock, and the program deadlocks. To be more precise: ## Step 1: Thread 1 grabs the `extension_to_loader_index` lock on lines 138..139: https://github.com/bevyengine/bevy/blob/3a1867a92edc571b8f842bb1a96112dcbdceae4b/crates/bevy_asset/src/asset_server.rs#L133-L145 ## Step 2: Thread 2 grabs the `server.loader` write lock on line 107: https://github.com/bevyengine/bevy/blob/3a1867a92edc571b8f842bb1a96112dcbdceae4b/crates/bevy_asset/src/asset_server.rs#L103-L116 ## Step 3: Deadlock, since Thread 1 wants to grab `server.loader` on line 141...: https://github.com/bevyengine/bevy/blob/3a1867a92edc571b8f842bb1a96112dcbdceae4b/crates/bevy_asset/src/asset_server.rs#L133-L145 ... and Thread 2 wants to grab 'extension_to_loader_index` on lines 111..112: https://github.com/bevyengine/bevy/blob/3a1867a92edc571b8f842bb1a96112dcbdceae4b/crates/bevy_asset/src/asset_server.rs#L103-L116 ## Solution Fixed by descoping the extension_to_loader_index lock, since `get_asset_loader` doesn't need to hold the read lock on the extensions map for the duration, just to get a copyable usize. The block might not be needed, I think I could have gotten away with just inserting a `copied()` call into the chain, but I wanted to make the reasoning clear for future maintainers. --- crates/bevy_asset/src/asset_server.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/crates/bevy_asset/src/asset_server.rs b/crates/bevy_asset/src/asset_server.rs index 2810b03b86..d5a331909d 100644 --- a/crates/bevy_asset/src/asset_server.rs +++ b/crates/bevy_asset/src/asset_server.rs @@ -134,11 +134,13 @@ impl AssetServer { &self, extension: &str, ) -> Result>, AssetServerError> { - self.server - .extension_to_loader_index - .read() - .get(extension) - .map(|index| self.server.loaders.read()[*index].clone()) + let index = { + // scope map to drop lock as soon as possible + let map = self.server.extension_to_loader_index.read(); + map.get(extension).copied() + }; + index + .map(|index| self.server.loaders.read()[index].clone()) .ok_or_else(|| AssetServerError::MissingAssetLoader { extensions: vec![extension.to_string()], })