Skip to content

Commit

Permalink
Fix AssetServer::get_asset_loader deadlock (#2395)
Browse files Browse the repository at this point in the history
# 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.
  • Loading branch information
vgel committed Jul 6, 2021
1 parent 337e6d5 commit 85a10ec
Showing 1 changed file with 7 additions and 5 deletions.
12 changes: 7 additions & 5 deletions crates/bevy_asset/src/asset_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,11 +134,13 @@ impl AssetServer {
&self,
extension: &str,
) -> Result<Arc<Box<dyn AssetLoader>>, 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()],
})
Expand Down

0 comments on commit 85a10ec

Please sign in to comment.