birkeh/o3de
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

[LYN-3411] Editor freezes when saving/reloading slice due to deadlock between main thread which is blocked waiting on a loading asset, and the loading thread, which is blocked on a mutex held by the main thread.

Browse Source 
The specific mutex is "m_assetMutex" in the AssetManager.  AssetManager::ReloadAssetFromData was holding this lock while calling AssignAssetData, which goes on to call OnAssetReloaded.  This is dangerous, because any arbitrary logic can be executed during that call, including blocking asset loads, which is what happens in this specific deadlock case.  The fix is to move the call to AssignAssetData outside of the mutex lock.  AssignAssetData itself already grabs the assetMutex to protect the parts of its logic that are necessary, and then specifically releases the lock before calling OnAssetReloaded, so this change shouldn't introduce any dangerous threading conditions.

(cherry picked from commit 1581a28339f12d521955edef890acfb678a0df6f)
This commit is contained in:
mbalfour
2021-04-29 10:12:54 -05:00
parent 1d4c483e73
commit 2cfb7d536e
1 changed files with 36 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Code/Framework/AzCore/AzCore/Asset/AssetManager.cpp
+36 -20
View File
@@ -1454,32 +1454,48 @@ namespace AZ
//=========================================================================
void AssetManager::ReloadAssetFromData(const Asset<AssetData>& asset)
{
AZ_Assert(asset.Get(), "Asset data for reload is missing.");
AZStd::scoped_lock<AZStd::recursive_mutex> assetLock(m_assetMutex);
AZ_Assert(m_assets.find(asset.GetId()) != m_assets.end(), "Unable to reload asset %s because its not in the AssetManager's asset list.", asset.ToString<AZStd::string>().c_str());
AZ_Assert(m_assets.find(asset.GetId()) == m_assets.end() || asset->RTTI_GetType() == m_assets.find(asset.GetId())->second->RTTI_GetType(),
"New and old data types are mismatched!");
bool shouldAssignAssetData = false;
auto found = m_assets.find(asset.GetId());
if ((found == m_assets.end()) || (asset->RTTI_GetType() != found->second->RTTI_GetType()))
{
return; // this will just lead to crashes down the line and the above asserts cover this.
}
AZ_Assert(asset.Get(), "Asset data for reload is missing.");
AZStd::scoped_lock<AZStd::recursive_mutex> assetLock(m_assetMutex);
AZ_Assert(
m_assets.find(asset.GetId()) != m_assets.end(),
"Unable to reload asset %s because it's not in the AssetManager's asset list.", asset.ToString<AZStd::string>().c_str());
AZ_Assert(
m_assets.find(asset.GetId()) == m_assets.end() ||
asset->RTTI_GetType() == m_assets.find(asset.GetId())->second->RTTI_GetType(),
"New and old data types are mismatched!");
AssetData* newData = asset.Get();
if (found->second != newData)
{
// Notify users that we are about to change asset
AssetBus::Event(asset.GetId(), &AssetBus::Events::OnAssetPreReload, asset);
// Resolve the asset handler and account for the new asset instance.
auto found = m_assets.find(asset.GetId());
if ((found == m_assets.end()) || (asset->RTTI_GetType() != found->second->RTTI_GetType()))
{
AssetHandlerMap::iterator handlerIt = m_handlers.find(newData->GetType());
AZ_Assert(handlerIt != m_handlers.end(), "No handler was registered for this asset [type:%s id:%s]!",
newData->GetType().ToString<AZ::OSString>().c_str(), newData->GetId().ToString<AZ::OSString>().c_str());
return; // this will just lead to crashes down the line and the above asserts cover this.
}
AssetData* newData = asset.Get();
if (found->second != newData)
{
// Notify users that we are about to change asset
AssetBus::Event(asset.GetId(), &AssetBus::Events::OnAssetPreReload, asset);
// Resolve the asset handler and account for the new asset instance.
{
AssetHandlerMap::iterator handlerIt = m_handlers.find(newData->GetType());
AZ_Assert(
handlerIt != m_handlers.end(), "No handler was registered for this asset [type:%s id:%s]!",
newData->GetType().ToString<AZ::OSString>().c_str(), newData->GetId().ToString<AZ::OSString>().c_str());
}
shouldAssignAssetData = true;
}
}
// We specifically perform this outside of the m_assetMutex lock so that the lock isn't held at the point that
// OnAssetReload is triggered inside of AssignAssetData. Otherwise, we open up a high potential for deadlocks.
if (shouldAssignAssetData)
{
AssignAssetData(asset);
}
}