Release the asset infos lock before acquiring the file transaction lock.

[andriyDev]

Objective

• Fix the first flake related to #22001.
• We had a double lock problem. We would first lock asset_infos, then lock one of the assets within asset_infos and then dropped the asset_infos lock. This means however that if a process needs to access asset_infos (which is necessary for loading nested assets during processing), we'll have one thread trying to lock 1) asset_infos, 2) per-asset lock, and we'll have another thread trying to lock 1) per-asset lock (we've already dropped the asset_infos lock), 2) asset_infos. A classic deadlock!

Solution

• Before locking the per-asset lock, we clone the Arc<RwLock> out of the asset_infos, drop the asset_infos, and only then lock the per-asset lock. This ensures that we never hang on the asset_infos lock when just trying to fetch a single asset.
• Make all the access to asset_infos "short" - they get what they need out of asset_infos and then drop the lock as soon as possible.
• I also used ? to cleanup some methods in ProcessorAssetInfos, where previously it was "remove the asset info, then have one big if let after it". Now we just return early if the value is none.
• I also happened to fix a weird case where the new path in a rename wasn't guarded by a transaction lock. Now it is!

Testing

• Running without this fix I get "Ran out of loops" from the only_reprocesses_wrong_hash_on_startup test quite quickly (after a minute or so). With this fix, I now only get the assertion failure problem. If I also skip that assertion, the test hasn't flaked for a while! Yay, no more deadlock!

[andriyDev]

@kfc35

Is it ok to not make the two “operations" [...] in handle_removed_asset and handle_renamed_asset atomic with respect to the locking

In theory we could have specifically the remove and rename still lock asset_infos and then also lock the files. However this means blocking all other access to the files while we wait for access to the old and new assets. I'd prefer not blocking *all access just for that.

my computer never came up with the deadlock on main

If I just run on main, the deadlock is very rare. Once I enabled logging, the deadlock was much more common. As an aside, we should probably just enable logging for bevy_asset tests in general - it makes it impossible to debug these issues otherwise.

Is the 4th step that the same thread eventually wants to lock asset_infos again, causing an inversion of the locking order...

Yes exactly! "Inversion of the locking order" is a much better way to describe it too haha. This happens through nested immediate asset loading. The dep_changed asset loader needs to nested-immedate asset load the source_changed asset, and processed asset reads are gated by ProcessorGatedReader which acquires the file transaction lock (which we need to acquire the asset_infos lock to look up).

[andriyDev]

I spent 5 days staring at this code and putting traces everywhere I could think of, so I deeply understand being confused here haha. Thank you for taking the time to understand it!

I noticed in the function wait_until_processed in this file, asset_infos is being locked for a write Line 1353, but it’s only being read from.

Yeah I noticed this too, but decided not to mess with it for now. I also suspect it's a mistake. I'll leave that for another PR though.

[kfc35]

Actually, sorry, I’m just thinking about the ramifications of separating out the two locks. Is it now possible to read an asset that’s been removed? For example: Thread 1

• tries to get_transaction_lock to read an asset (like a là ProcessorGatedReader maybe?)
• locks asset_infos, gets a ProcessorAssetInfo for a path, unlocks asset_infos

Before Thread 1 can lock the file_transaction_lock to start reading…

Thread 2

• starts executing handle_removed_asset
• locks asset_infos, removes the info from the same path, unlocks asset_infos
• also locks file_transaction_lock!
• does further removal via remove_processed_asset_and_meta
• unlocks file_transaction_lock

Thread 1

• locks file_transaction_lock
• attempts to read bytes
• boom?

Having the operations lock both locks would prevent this from happening afaict.

(But also if this is totally off-base because this is not how the code would actually be run, or this situation is not a big deal, or the situation is too rare to justify addressing please disregard!!)

[andriyDev]

At the end of the day, the "source of truth" is the asset source, not the transaction lock. So it's totally valid for us to lock the transaction lock, and for the AssetReader::read to return a file-not-found error. We just have a slight optimization that if there is no transaction lock, we assume the file doesn't exist. So whether the transaction lock is missing or the AssetReader::read fails, either way we return a file-not-found error, and then it's no problem! We have a failed process, but I think that's fair when we're racing between the delete of the source asset and the processing of an asset. We also have problems if someone deletes a source asset half way through processing an asset.

This did make me realize we should probably also delete the processed asset path if we fail so we don't leave a corrupted file around. But that's a future PR.