[Merged by Bors] - Fix AssetServer::get_asset_loader deadlock #2395

vgel · 2021-06-25T21:33:21Z

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:

bevy/crates/bevy_asset/src/asset_server.rs

Lines 133 to 145 in 3a1867a

    
           fn get_asset_loader( 
        
               &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()) 
        
                   .ok_or_else(|| AssetServerError::MissingAssetLoader { 
        
                       extensions: vec![extension.to_string()], 
        
                   }) 
        
           }

Step 2: Thread 2 grabs the `server.loader` write lock on line 107:

bevy/crates/bevy_asset/src/asset_server.rs

Lines 103 to 116 in 3a1867a

    
           pub fn add_loader<T>(&self, loader: T) 
        
           where 
        
               T: AssetLoader, 
        
           { 
        
               let mut loaders = self.server.loaders.write(); 
        
               let loader_index = loaders.len(); 
        
               for extension in loader.extensions().iter() { 
        
                   self.server 
        
                       .extension_to_loader_index 
        
                       .write() 
        
                       .insert(extension.to_string(), loader_index); 
        
               } 
        
               loaders.push(Arc::new(Box::new(loader))); 
        
           }

Step 3: Deadlock, since Thread 1 wants to grab `server.loader` on line 141...:

bevy/crates/bevy_asset/src/asset_server.rs

Lines 133 to 145 in 3a1867a

    
           fn get_asset_loader( 
        
               &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()) 
        
                   .ok_or_else(|| AssetServerError::MissingAssetLoader { 
        
                       extensions: vec![extension.to_string()], 
        
                   }) 
        
           }

... and Thread 2 wants to grab 'extension_to_loader_index` on lines 111..112:

bevy/crates/bevy_asset/src/asset_server.rs

Lines 103 to 116 in 3a1867a

    
           pub fn add_loader<T>(&self, loader: T) 
        
           where 
        
               T: AssetLoader, 
        
           { 
        
               let mut loaders = self.server.loaders.write(); 
        
               let loader_index = loaders.len(); 
        
               for extension in loader.extensions().iter() { 
        
                   self.server 
        
                       .extension_to_loader_index 
        
                       .write() 
        
                       .insert(extension.to_string(), loader_index); 
        
               } 
        
               loaders.push(Arc::new(Box::new(loader))); 
        
           }

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.

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. Fixed by descoping the extension_to_loader_index lock, since get_asset_loader doesn't need to hold the lock for the duration, just to get a copiable 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.

NathanSWard

This is a great catch!! Nice job 😄
This issue also probably hints at us needed to run some sort of better testing for parallel code.
E.g. some sort of fuzz testing or thread-sanitizer.

crates/bevy_asset/src/asset_server.rs

mockersf · 2021-06-25T23:26:07Z

bors try

bors · 2021-06-25T23:48:25Z

try

Build succeeded:

bjorn3 · 2021-06-26T07:46:25Z

This issue also probably hints at us needed to run some sort of better testing for parallel code.
E.g. some sort of fuzz testing or thread-sanitizer.

Loom can test all possible interleavings of synchronization primitives like mutexes and atomics.

NathanSWard · 2021-06-29T15:21:07Z

(@mockersf I'd say this PR is safe to merge)

mockersf · 2021-07-05T01:57:59Z

could you mention in #2373 if you're ok with changing the license for your contribution?

vgel · 2021-07-06T17:17:38Z

could you mention in #2373 if you're ok with changing the license for your contribution?

Done!

mockersf · 2021-07-06T17:35:00Z

bors r+

# 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.

bors · 2021-07-06T17:53:36Z

Pull request successfully merged into main.

Build succeeded:

# 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.

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jun 25, 2021

NathanSWard added A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior and removed S-Needs-Triage This issue needs to be labelled labels Jun 25, 2021

NathanSWard suggested changes Jun 25, 2021

View reviewed changes

crates/bevy_asset/src/asset_server.rs Show resolved Hide resolved

NathanSWard approved these changes Jun 25, 2021

View reviewed changes

bors bot added a commit that referenced this pull request Jun 25, 2021

Try #2395:

77443ee

mockersf approved these changes Jun 25, 2021

View reviewed changes

NathanSWard added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 26, 2021

cart added this to the Bevy 0.6 milestone Jul 1, 2021

bors bot changed the title ~~Fix AssetServer::get_asset_loader deadlock~~ [Merged by Bors] - Fix AssetServer::get_asset_loader deadlock Jul 6, 2021

bors bot closed this Jul 6, 2021

dman-os mentioned this pull request Aug 15, 2021

Double xforms feature flag & type aliasing approach dman-os/bevy#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Fix AssetServer::get_asset_loader deadlock #2395

[Merged by Bors] - Fix AssetServer::get_asset_loader deadlock #2395

vgel commented Jun 25, 2021

NathanSWard left a comment •

edited

Loading

mockersf commented Jun 25, 2021

bors bot commented Jun 25, 2021

bjorn3 commented Jun 26, 2021

NathanSWard commented Jun 29, 2021

mockersf commented Jul 5, 2021

vgel commented Jul 6, 2021

mockersf commented Jul 6, 2021

bors bot commented Jul 6, 2021

	fn get_asset_loader(
	&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())
	.ok_or_else(\|\| AssetServerError::MissingAssetLoader {
	extensions: vec![extension.to_string()],
	})
	}

	pub fn add_loader<T>(&self, loader: T)
	where
	T: AssetLoader,
	{
	let mut loaders = self.server.loaders.write();
	let loader_index = loaders.len();
	for extension in loader.extensions().iter() {
	self.server
	.extension_to_loader_index
	.write()
	.insert(extension.to_string(), loader_index);
	}
	loaders.push(Arc::new(Box::new(loader)));
	}

[Merged by Bors] - Fix AssetServer::get_asset_loader deadlock #2395

[Merged by Bors] - Fix AssetServer::get_asset_loader deadlock #2395

Conversation

vgel commented Jun 25, 2021

Objective

Step 1: Thread 1 grabs the extension_to_loader_index lock on lines 138..139:

Step 2: Thread 2 grabs the server.loader write lock on line 107:

Step 3: Deadlock, since Thread 1 wants to grab server.loader on line 141...:

Solution

NathanSWard left a comment • edited Loading

Choose a reason for hiding this comment

mockersf commented Jun 25, 2021

bors bot commented Jun 25, 2021

try

bjorn3 commented Jun 26, 2021

NathanSWard commented Jun 29, 2021

mockersf commented Jul 5, 2021

vgel commented Jul 6, 2021

mockersf commented Jul 6, 2021

bors bot commented Jul 6, 2021

Step 1: Thread 1 grabs the `extension_to_loader_index` lock on lines 138..139:

Step 2: Thread 2 grabs the `server.loader` write lock on line 107:

Step 3: Deadlock, since Thread 1 wants to grab `server.loader` on line 141...:

NathanSWard left a comment •

edited

Loading